go_test: change directory before use init() functions run by dragonsinth · Pull Request #2696 · bazel-contrib/rules_go

dragonsinth · 2020-10-28T21:07:30Z

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

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

dragonsinth · 2020-10-28T22:19:21Z

Looks like I have some things to fix!

jayconrod

Thanks for working on this. Couple comments below.

jayconrod · 2020-10-29T15:03:53Z

        "//go/platform:internal_cgo_off": None,
        "//conditions:default": ":cgo_context_data",
    }),
+    test_init = "//go/tools/test_init",


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.

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.

_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.

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. :/

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

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!

dragonsinth · 2020-10-29T15:44:26Z

@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 · 2020-10-29T15:56:59Z

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 · 2020-10-29T18:43:44Z

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 · 2020-10-29T19:17:27Z

All feedback should now be addressed.

jayconrod

Looks good. Thanks again for working on this!

dragonsinth · 2020-10-31T05:02:57Z

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

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

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

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

@tomasr8

go_test: change directory before use init() functions run

f0e95fa

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

dragonsinth requested a review from jayconrod as a code owner October 28, 2020 21:07

google-cla Bot added the cla: yes label Oct 28, 2020

dragonsinth mentioned this pull request Oct 28, 2020

go_test: data not available in init() #1918

Closed

fix

583d804

jayconrod reviewed Oct 29, 2020

View reviewed changes

dragonsinth added 2 commits October 29, 2020 13:12

initial review feedback

79417da

fmt

1ac6ee0

dragonsinth added 2 commits October 29, 2020 14:48

fix windows

812db99

wire up _testmain_additional_deps

5e8e10e

dragonsinth requested a review from jayconrod October 29, 2020 19:45

jayconrod approved these changes Oct 30, 2020

View reviewed changes

jayconrod merged commit 9c1568a into bazel-contrib:master Oct 30, 2020

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

go_test: add tests for bazel-contrib#2696

310031f

dragonsinth mentioned this pull request Oct 31, 2020

go_test: add tests for #2696 #2698

Merged

jayconrod pushed a commit that referenced this pull request Nov 9, 2020

go_test: add tests for #2696 (#2698)

bf1b5b9

mikedanese mentioned this pull request Jan 7, 2021

regression in go_test generation breaks unix domain sockets created in the temp directory #2776

Closed

Uh oh!

Conversation

dragonsinth commented Oct 28, 2020

Uh oh!

dragonsinth commented Oct 28, 2020

Uh oh!

jayconrod left a comment

Choose a reason for hiding this comment

Uh oh!

jayconrod Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

dragonsinth Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

jayconrod Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

dragonsinth Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dragonsinth Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

dragonsinth Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dragonsinth commented Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayconrod commented Oct 29, 2020

Uh oh!

dragonsinth commented Oct 29, 2020

Uh oh!

dragonsinth commented Oct 29, 2020

Uh oh!

jayconrod left a comment

Choose a reason for hiding this comment

Uh oh!

dragonsinth commented Oct 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dragonsinth Oct 29, 2020 •

edited

Loading

dragonsinth commented Oct 29, 2020 •

edited

Loading