Reset Go settings for protoc dependencies #3005
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
No worries, let me know if you need any additional explanations. |
fmeum
force-pushed
the
hermeticity-tests
branch
from
cd757ef
to
6f570a7
Compare
|
I rebased the PR onto master after my other changes were merged. |
|
@blico Friendly ping as the required commit has landed in Bazel: bazelbuild/bazel@52d1d4a |
|
@robfig I just submitted #3108, which is based on this PR and finished the leftover work. Along the way, it fixes a rules_go correctness bug reported in bazelbuild/bazel#14626. Do you think one of the maintainers will have time to review either of those PRs in the near future? |
achew22
approved these changes
May 4, 2022
linzhp
requested changes
May 4, 2022
go/private/rules/transition.bzl
Outdated
|
|
||
| go_reset_target depends on a single target, built using go_reset_transition. | ||
| go_exec_reset_target depends on a single target, built using go_exec_reset_transition. |
There was a problem hiding this comment.
I took the chance to improve the docs, but am not sure I understood this particular point.
This transition should be applied to all non-Go tools (e.g. protoc) to prevent unnecessary rebuilds.
The test verifies that Go Starlark settings such as bootstrap_nogo and pure are reset before they reach protoc, which is a dependency of go_proto_library in multiple ways.
|
@linzhp Thanks for the review. I pushed a commit that should address your comments. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
Reduces the number of essentially distinct configurations in which
protocas a dependency ofgo_proto_libraryis analyzed.Note that this may not (yet) result in a significant reduction of rebuilds since Bazel is not particular friendly to reset transitions at the moment: It encodes some information about the path of transitions taken into the build configuration, which e.g. prevents transitions from ever returning to the top-level target configuration. But this will be solved by bazelbuild/bazel#14023, which is very actively being worked on. In the meantime, this change should at least not regress the current behavior further.
Which issues(s) does this PR fix?
Fixes a part of #2999.
Other notes for review
@robfig If you think that this PR is useful, I would also look into a follow-up that resets the non-Go attrs, e.g.
dataandembedsrcs, of rules such asgo_libraryto their values beforego_transition. Ideally,rules_gosettings set by transitions would only ever propagate to rules that provide a Go provider. Since this requires memoizing the state of settings before a transition, this will be a bit more complicated, which is why I would like to get your feedback on this simpler case first.