-
-
Notifications
You must be signed in to change notification settings - Fork 723
Fix breakages with Bazel@HEAD and incompatible flags #4368
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
Conversation
The `local_config_platform` repo is no longer available with Bazel@HEAD.
44bc6fb to
91402a1
Compare
@local_config_platform with @platforms//host|
FYI @jayconrod @linzhp I'm going to merge this now to unblock dependent projects, but please take a look and let me know what I should change in a follow-up PR. |
| go_register_toolchains(version = "%[3]s")`, version, shasum, goVersion) | ||
| go_register_toolchains(version = "%[3]s") | ||
| # Create the host platform repository transitively required by rules_go. |
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.
Changing the boilerplate is a pretty substantial change that will cause friction for WORKSPACE users. Is there a way to roll this into go_rules_dependencies? I guess not if we need this host_platform_repo.
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.
I agree, but I don't see another way. We would need to add a new macro and at that point it's much more transparent to have the user instantiate the repo directly.
I would consider this the cost of staying with a pure WORKSPACE setup - if you get at least rules_go via Bzlmod, this is not an issue for you.
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.
Hmm, will this actually affect Bazel WORKSPACE users? If the Bazel authors indeed remove WORKSPACE support in 9, then WORKSPACE users will be on older versions, not affected by this change.
Maybe it's best to keep the boilerplate simple for them and disable the WORKSPACE-mode tests for newer versions of Bazel?
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.
The change in go/private/rules/go_bin_for_host.bzl does affect WORKSPACE users as it requires the new repo to be defined. We could indirect the load through a repo rule of our own in which we dynamically generate this label based on the Bazel version.
But this is a dep of many other rulesets and users only have to call this once, so that additional complexity may very well not be worth it. It's not just the load after all, we would also need to make sure that stardoc deps are declared correctly for each variant.
What type of PR is this?
Adapt to a Bazel breaking change
What does this PR do? Why is it needed?
The
local_config_platformrepo is no longer available with Bazel@HEAD, so the host constraints have to be loaded from@platforms//host.Also prepare the BCR test repo for Bazel@HEAD with
--incompatible_autoload_externally=by applying buildifier lint fixes.Which issues(s) does this PR fix?
Other notes for review