Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

go_test: change directory before use init() functions run #2696

Merged
merged 6 commits into from
Oct 30, 2020

Conversation

dragonsinth
Copy link
Contributor

Fixes #1918 go_test: data not available in init()

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Forces a new package, go/tools/test_init to have its package initializer run before any Go user code can be initialized. This package sets the correct working directory, PWD, and TMPDIR to match bazel standards.

Which issues(s) does this PR fix?

Fixes #1918

@dragonsinth
Copy link
Contributor Author

Looks like I have some things to fix!

Copy link
Contributor

@jayconrod jayconrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. Couple comments below.

BUILD.bazel Outdated
@@ -75,6 +76,7 @@ go_context_data(
"//go/platform:internal_cgo_off": None,
"//conditions:default": ":cgo_context_data",
}),
test_init = "//go/tools/test_init",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be an implicit dependency of go_test.

coverdata is a little different, because every go_library might need to import it. test_init only needs to be imported by go_test archives.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being so dense, but how do I do that exactly? I tried doing that first, but I could never figure out exactly how and where to add this dep without following the coverdata pattern. I can try hacking at this more and give more detail on exactly what I ran into.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_testmain_additional_srcs is a better pattern to follow. Add an attribute to go_test that starts with _. It should be an attr.label with a default value pointing to that go_library target.

Copy link
Contributor Author

@dragonsinth dragonsinth Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm close, but I'm missing something. I added this:

--- a/go/private/rules/test.bzl
+++ b/go/private/rules/test.bzl
@@ -137,12 +137,14 @@ def _go_test_impl(ctx):
         resolve = None,
     )
     test_deps = external_archive.direct + [external_archive]
-    test_deps.append(go.test_init)
+    print("test_deps[0]:", type(test_deps[0]), test_deps[0])
+    print("_testmain_additional_deps[0]:", type(ctx.files._testmain_additional_deps[0]), ctx.files._testmain_additional_deps[0])
+    print("go.coverdata:", type(go.coverdata), go.coverdata)
     if ctx.configuration.coverage_enabled:
         test_deps.append(go.coverdata)
     test_source = go.library_to_source(go, struct(
         srcs = [struct(files = [main_go] + ctx.files._testmain_additional_srcs)],
-        deps = test_deps,
+        deps = test_deps + ctx.files._testmain_additional_deps,
     ), test_library, False)
     test_archive, executable, runfiles = go.binary(
         go,
@@ -201,6 +203,10 @@ _go_test_kwargs = {
             default = ["@io_bazel_rules_go//go/tools/testwrapper:srcs"],
             allow_files = go_exts,
         ),
+        "_testmain_additional_deps": attr.label_list(
+            providers = [GoLibrary],
+            default = ["@io_bazel_rules_go//go/tools/testinit"],
+        ),
         # Workaround for bazelbuild/bazel#6293. See comment in lcov_merger.sh.
         "_lcov_merger": attr.label(
             executable = True,

But the thing is, when I get to the print statements, the _testmain_additional_deps[0] is a File, whereas go.coverdata is a struct. I can't figure out how to make it be the kind of thing it needs to be. :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved all the feedback except for this-- I'm a little out of my depth. :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright.. I finally stumbled my way through this! I needed ctx.attr._testmain_additional_deps to get the actual target, not ctx.files._testmain_additional_deps which only has the output file. Big oof!

go/tools/builders/generate_test_main.go Outdated Show resolved Hide resolved
go/tools/test_init/test_init.go Outdated Show resolved Hide resolved
go/tools/test_init/test_init.go Outdated Show resolved Hide resolved
@dragonsinth
Copy link
Contributor Author

dragonsinth commented Oct 29, 2020

@jayconrod does this repo not run correctly under OSX? I'm sorry to be asking such an off topic question, but I can't get things to build on my machine due to '@local_config_cc//:toolchain' does not contain a toolchain for cpu 'darwin_x86_64'. I've reinstalled xcode command line tools, bazel clean expunge, bazel sync, and nothing I've tried lets me get this running locally. (This is on master, not even on my branch.)

@jayconrod
Copy link
Contributor

It should work on macOS. I do most of my development there.

Sounds like a C/C++ toolchain configuration problem? Are you able to build a cc_binary? Not sure I can help much there.

@dragonsinth
Copy link
Contributor Author

Sounds like a C/C++ toolchain configuration problem? Are you able to build a cc_binary? Not sure I can help much there.

I was able to get past this-- the problem is only with tests/core/cgo/objc:objc_test. I just commented it out locally and everything else builds fine.

@dragonsinth
Copy link
Contributor Author

All feedback should now be addressed.

@dragonsinth dragonsinth requested a review from jayconrod October 29, 2020 19:45
Copy link
Contributor

@jayconrod jayconrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks again for working on this!

@jayconrod jayconrod merged commit 9c1568a into bazel-contrib:master Oct 30, 2020
@dragonsinth dragonsinth deleted the test_init branch October 31, 2020 04:59
dragonsinth added a commit to dragonsinth/rules_go that referenced this pull request Oct 31, 2020
@dragonsinth
Copy link
Contributor Author

Sweet! Funny enough, I was meaning to add test coverage as well. So I went ahead and opened a new PR.

jayconrod pushed a commit that referenced this pull request Nov 9, 2020
mikedanese added a commit to mikedanese/kubernetes that referenced this pull request Jan 7, 2021
There was a bug with init() but it was resolved in
bazel-contrib/rules_go#2696

Changed to match other fixture methods.

Change-Id: I882b8535e5c5c117fb10c41d34c8eed1ccdb74bb
k8s-publishing-bot pushed a commit to kubernetes/legacy-cloud-providers that referenced this pull request Jan 8, 2021
There was a bug with init() but it was resolved in
bazel-contrib/rules_go#2696

Changed to match other fixture methods.

Change-Id: I882b8535e5c5c117fb10c41d34c8eed1ccdb74bb

Kubernetes-commit: 77261377dec3a22fef6cc7b5eb38322539bddee6
adamzhoul pushed a commit to adamzhoul/kubernetes that referenced this pull request Jan 11, 2021
There was a bug with init() but it was resolved in
bazel-contrib/rules_go#2696

Changed to match other fixture methods.

Change-Id: I882b8535e5c5c117fb10c41d34c8eed1ccdb74bb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

go_test: data not available in init()
2 participants