-
-
Notifications
You must be signed in to change notification settings - Fork 664
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
Transition on edges not self #3116
Transition on edges not self #3116
Conversation
ba9d8a8
to
f811aa0
Compare
/cc @fmeum I think this is good to go! |
@illicitonion Looks great and removes a great deal of complexity. The maintainers would likely appreciate a small test (or just a regular target under |
Test added! I verified it failed (at least on Linux) before this change, too. |
Should the transition be added to all attributes? This would result in a undesirable behaviour change for any attributes which don't include the transition. It's possible to transition values for almost all attribute types. |
Sorry, I realise I should have added some more context to my previous comment. It's possible for the transition rule to change the target platform. As such, attributes which don't use this transition will then potentially select on the wrong platform. Does this make sense? |
True, I forgot about the possible platform change when I proposed this in #3103 (comment). Adding the transition to every |
Don't forget attr.string! They are also selectable. I don't know if other attributes are too. |
You are right, messed up again. It's possible that someone selects essentially any attribute based on the current platform. That's bad. I'll have to think about an alternative, as fixing #3103 also seems desirable. But maybe the wrapper has to be brought back. Sorry @illicitonion for leading you in the wrong direction. |
Makes sense - I've re-opened #3130 which adds a wrapper target to do the transitioning, PTAL instead! |
@illicitonion Sorry again for the long wait and back and forth on this. If you feel like you don't have the cycles to make the change I mentioned in #3130 (comment), please let me know and I can take over that part. |
381a257
to
f848748
Compare
Glad to hear that, relaxed contributors are our favorite contributors :-) This looks really good overall, only left a few comments. Glad to see the deletion of quite a bit of complex code. |
e0d7300
to
5770f3c
Compare
As per bazelbuild/bazel#15157 currently we can't configure any transition-relevant attributes using select. This is because we use self-transitions on targets, and when this happens, configurable attributes aren't passed to the transition, so we make incorrect transition decisions. We only actually consult configuration data in `_go_context_data`, so we only actually need to transition on the edges which (transitively) reach a `_go_context_data`, which is `_go_context_data` itself and `deps`.
5770f3c
to
c92a034
Compare
1ee423e
to
f19c329
Compare
Thanks for the contribution @illicitonion! @linzhp Could you test and merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tests passed in Uber
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | minor | `v0.33.0` -> `v0.34.0` | --- ### Release Notes <details> <summary>bazelbuild/rules_go</summary> ### [`v0.34.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.34.0) [Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.33.0...v0.34.0) #### What's Changed - releaser: fix scrubbing timestamp from patch files by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3180](https://togithub.com/bazelbuild/rules_go/pull/3180) - Replace Starlark JSON parser with json.decode by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3184](https://togithub.com/bazelbuild/rules_go/pull/3184) - gopackagesdriver: separates "s" files in pkg info by [@​iamricard](https://togithub.com/iamricard) in [https://github.com/bazelbuild/rules_go/pull/3165](https://togithub.com/bazelbuild/rules_go/pull/3165) - Refactor away references to @​io_bazel_rules_go by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3185](https://togithub.com/bazelbuild/rules_go/pull/3185) - Do not print to stderr if cgo linking succeeds after retry by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3187](https://togithub.com/bazelbuild/rules_go/pull/3187) - Use param files with go-protoc by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3190](https://togithub.com/bazelbuild/rules_go/pull/3190) - Don't include non-executable go_binary in dependent's runfiles by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3151](https://togithub.com/bazelbuild/rules_go/pull/3151) - Link in native libraries of transitive dependencies in archive mode by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3186](https://togithub.com/bazelbuild/rules_go/pull/3186) - runfiles: remove deprecated api by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3198](https://togithub.com/bazelbuild/rules_go/pull/3198) - Fix failing open hermeticity test by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3206](https://togithub.com/bazelbuild/rules_go/pull/3206) - Fix go_googleapis Gazelle patch by [@​nickgooding](https://togithub.com/nickgooding) in [https://github.com/bazelbuild/rules_go/pull/3193](https://togithub.com/bazelbuild/rules_go/pull/3193) - Exclude unsupported C/C++ features by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3189](https://togithub.com/bazelbuild/rules_go/pull/3189) - Allow gomock to take Bazel common attributes by [@​linzhp](https://togithub.com/linzhp) in [https://github.com/bazelbuild/rules_go/pull/3207](https://togithub.com/bazelbuild/rules_go/pull/3207) - Transition on edges not self by [@​illicitonion](https://togithub.com/illicitonion) in [https://github.com/bazelbuild/rules_go/pull/3116](https://togithub.com/bazelbuild/rules_go/pull/3116) - Include go_transition_test in bazel aspect by [@​ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) in [https://github.com/bazelbuild/rules_go/pull/3160](https://togithub.com/bazelbuild/rules_go/pull/3160) - Add an example for go_download_sdk.sdks by [@​fishy](https://togithub.com/fishy) in [https://github.com/bazelbuild/rules_go/pull/3139](https://togithub.com/bazelbuild/rules_go/pull/3139) - tests: nogo over generated code by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3214](https://togithub.com/bazelbuild/rules_go/pull/3214) - test nogo/coverage: test generated code by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3213](https://togithub.com/bazelbuild/rules_go/pull/3213) - Remove references to go_transition_test by [@​linzhp](https://togithub.com/linzhp) in [https://github.com/bazelbuild/rules_go/pull/3215](https://togithub.com/bazelbuild/rules_go/pull/3215) - Basic bzlmod setup by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3047](https://togithub.com/bazelbuild/rules_go/pull/3047) - Run BCR tests against Bazel 6.0.0-pre.20220608.2 by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3223](https://togithub.com/bazelbuild/rules_go/pull/3223) - Use repo-relative labels in MODULE.bazel by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3226](https://togithub.com/bazelbuild/rules_go/pull/3226) - upkeep: upgrade to go 1.18.3 and gazelle v0.26.0 by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3220](https://togithub.com/bazelbuild/rules_go/pull/3220) - nogo: ignore generated source files by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3216](https://togithub.com/bazelbuild/rules_go/pull/3216) - asm: Pass package path with -p by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3231](https://togithub.com/bazelbuild/rules_go/pull/3231) - bzlmod: Add support for gomock by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3232](https://togithub.com/bazelbuild/rules_go/pull/3232) - test cgo: ensure helper script works by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3236](https://togithub.com/bazelbuild/rules_go/pull/3236) - Fix //tests/legacy/examples/cgo:cgo_lib_test on M1 Macs by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3237](https://togithub.com/bazelbuild/rules_go/pull/3237) - gopackagesdriver: Descend into go_proto_compiler's deps by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3240](https://togithub.com/bazelbuild/rules_go/pull/3240) - new_library: remove unused resolver by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3219](https://togithub.com/bazelbuild/rules_go/pull/3219) - nogo: instantiate type info for generic types when running under Go >=1.18 by [@​farhaven](https://togithub.com/farhaven) in [https://github.com/bazelbuild/rules_go/pull/3212](https://togithub.com/bazelbuild/rules_go/pull/3212) #### New Contributors - [@​iamricard](https://togithub.com/iamricard) made their first contribution in [https://github.com/bazelbuild/rules_go/pull/3165](https://togithub.com/bazelbuild/rules_go/pull/3165) - [@​ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) made their first contribution in [https://github.com/bazelbuild/rules_go/pull/3160](https://togithub.com/bazelbuild/rules_go/pull/3160) - [@​fishy](https://togithub.com/fishy) made their first contribution in [https://github.com/bazelbuild/rules_go/pull/3139](https://togithub.com/bazelbuild/rules_go/pull/3139) - [@​farhaven](https://togithub.com/farhaven) made their first contribution in [https://github.com/bazelbuild/rules_go/pull/3212](https://togithub.com/bazelbuild/rules_go/pull/3212) **Full Changelog**: bazel-contrib/rules_go@v0.33.0...v0.34.0 #### `WORKSPACE` code load("@​bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") http_archive( name = "io_bazel_rules_go", sha256 = "16e9fca53ed6bd4ff4ad76facc9b7b651a89db1689a2877d6fd7b82aa824e366", urls = [ "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip", "https://github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip", ], ) load("@​io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") go_rules_dependencies() go_register_toolchains(version = "1.18.4") </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/gapic-generator-go). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMTcuNCIsInVwZGF0ZWRJblZlciI6IjMyLjExNy40In0=-->
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | minor | `v0.33.0` -> `v0.34.0` | --- ### Release Notes <details> <summary>bazelbuild/rules_go</summary> ### [`v0.34.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.34.0) [Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.33.0...v0.34.0) #### What's Changed - releaser: fix scrubbing timestamp from patch files by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3180](https://togithub.com/bazelbuild/rules_go/pull/3180) - Replace Starlark JSON parser with json.decode by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3184](https://togithub.com/bazelbuild/rules_go/pull/3184) - gopackagesdriver: separates "s" files in pkg info by [@​iamricard](https://togithub.com/iamricard) in [https://github.com/bazelbuild/rules_go/pull/3165](https://togithub.com/bazelbuild/rules_go/pull/3165) - Refactor away references to @​io_bazel_rules_go by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3185](https://togithub.com/bazelbuild/rules_go/pull/3185) - Do not print to stderr if cgo linking succeeds after retry by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3187](https://togithub.com/bazelbuild/rules_go/pull/3187) - Use param files with go-protoc by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3190](https://togithub.com/bazelbuild/rules_go/pull/3190) - Don't include non-executable go_binary in dependent's runfiles by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3151](https://togithub.com/bazelbuild/rules_go/pull/3151) - Link in native libraries of transitive dependencies in archive mode by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3186](https://togithub.com/bazelbuild/rules_go/pull/3186) - runfiles: remove deprecated api by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3198](https://togithub.com/bazelbuild/rules_go/pull/3198) - Fix failing open hermeticity test by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3206](https://togithub.com/bazelbuild/rules_go/pull/3206) - Fix go_googleapis Gazelle patch by [@​nickgooding](https://togithub.com/nickgooding) in [https://github.com/bazelbuild/rules_go/pull/3193](https://togithub.com/bazelbuild/rules_go/pull/3193) - Exclude unsupported C/C++ features by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3189](https://togithub.com/bazelbuild/rules_go/pull/3189) - Allow gomock to take Bazel common attributes by [@​linzhp](https://togithub.com/linzhp) in [https://github.com/bazelbuild/rules_go/pull/3207](https://togithub.com/bazelbuild/rules_go/pull/3207) - Transition on edges not self by [@​illicitonion](https://togithub.com/illicitonion) in [https://github.com/bazelbuild/rules_go/pull/3116](https://togithub.com/bazelbuild/rules_go/pull/3116) - Include go_transition_test in bazel aspect by [@​ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) in [https://github.com/bazelbuild/rules_go/pull/3160](https://togithub.com/bazelbuild/rules_go/pull/3160) - Add an example for go_download_sdk.sdks by [@​fishy](https://togithub.com/fishy) in [https://github.com/bazelbuild/rules_go/pull/3139](https://togithub.com/bazelbuild/rules_go/pull/3139) - tests: nogo over generated code by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3214](https://togithub.com/bazelbuild/rules_go/pull/3214) - test nogo/coverage: test generated code by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3213](https://togithub.com/bazelbuild/rules_go/pull/3213) - Remove references to go_transition_test by [@​linzhp](https://togithub.com/linzhp) in [https://github.com/bazelbuild/rules_go/pull/3215](https://togithub.com/bazelbuild/rules_go/pull/3215) - Basic bzlmod setup by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3047](https://togithub.com/bazelbuild/rules_go/pull/3047) - Run BCR tests against Bazel 6.0.0-pre.20220608.2 by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3223](https://togithub.com/bazelbuild/rules_go/pull/3223) - Use repo-relative labels in MODULE.bazel by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3226](https://togithub.com/bazelbuild/rules_go/pull/3226) - upkeep: upgrade to go 1.18.3 and gazelle v0.26.0 by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3220](https://togithub.com/bazelbuild/rules_go/pull/3220) - nogo: ignore generated source files by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3216](https://togithub.com/bazelbuild/rules_go/pull/3216) - asm: Pass package path with -p by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3231](https://togithub.com/bazelbuild/rules_go/pull/3231) - bzlmod: Add support for gomock by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3232](https://togithub.com/bazelbuild/rules_go/pull/3232) - test cgo: ensure helper script works by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3236](https://togithub.com/bazelbuild/rules_go/pull/3236) - Fix //tests/legacy/examples/cgo:cgo_lib_test on M1 Macs by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3237](https://togithub.com/bazelbuild/rules_go/pull/3237) - gopackagesdriver: Descend into go_proto_compiler's deps by [@​fmeum](https://togithub.com/fmeum) in [https://github.com/bazelbuild/rules_go/pull/3240](https://togithub.com/bazelbuild/rules_go/pull/3240) - new_library: remove unused resolver by [@​sluongng](https://togithub.com/sluongng) in [https://github.com/bazelbuild/rules_go/pull/3219](https://togithub.com/bazelbuild/rules_go/pull/3219) - nogo: instantiate type info for generic types when running under Go >=1.18 by [@​farhaven](https://togithub.com/farhaven) in [https://github.com/bazelbuild/rules_go/pull/3212](https://togithub.com/bazelbuild/rules_go/pull/3212) #### New Contributors - [@​iamricard](https://togithub.com/iamricard) made their first contribution in [https://github.com/bazelbuild/rules_go/pull/3165](https://togithub.com/bazelbuild/rules_go/pull/3165) - [@​ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) made their first contribution in [https://github.com/bazelbuild/rules_go/pull/3160](https://togithub.com/bazelbuild/rules_go/pull/3160) - [@​fishy](https://togithub.com/fishy) made their first contribution in [https://github.com/bazelbuild/rules_go/pull/3139](https://togithub.com/bazelbuild/rules_go/pull/3139) - [@​farhaven](https://togithub.com/farhaven) made their first contribution in [https://github.com/bazelbuild/rules_go/pull/3212](https://togithub.com/bazelbuild/rules_go/pull/3212) **Full Changelog**: bazel-contrib/rules_go@v0.33.0...v0.34.0 #### `WORKSPACE` code load("@​bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") http_archive( name = "io_bazel_rules_go", sha256 = "16e9fca53ed6bd4ff4ad76facc9b7b651a89db1689a2877d6fd7b82aa824e366", urls = [ "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip", "https://github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip", ], ) load("@​io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") go_rules_dependencies() go_register_toolchains(version = "1.18.4") </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/gapic-config-validator). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMTcuNCIsInVwZGF0ZWRJblZlciI6IjMyLjExNy40In0=-->
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
As per bazelbuild/bazel#15157 currently we
can't configure any transition-relevant attributes using select. This is
because we use self-transitions on targets, and when this happens,
configurable attributes aren't passed to the transition, so we make
incorrect transition decisions.
We only actually consult configuration data in
_go_context_data
, so weonly actually need to transition on the edges which (transitively) reach
a
_go_context_data
, which is_go_context_data
itself anddeps
.Which issues(s) does this PR fix?
Fixes #3103
Fixes #3015