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

Tweak how we pass GOEXPERIMENT to actions #4022

Merged
merged 1 commit into from
Aug 11, 2024

Conversation

dzbarsky
Copy link
Contributor

@dzbarsky dzbarsky commented Aug 9, 2024

What type of PR is this?
Small cleanup. We can just encode the string in the repo rule instead of creating it for every go action.

What does this PR do? Why is it needed?

Which issues(s) does this PR fix?

Fixes #

Other notes for review

go/private/BUILD.sdk.bazel Outdated Show resolved Hide resolved
@dzbarsky dzbarsky force-pushed the zbarsky/experiments branch from d1d3fa3 to f39da84 Compare August 11, 2024 15:23
@fmeum fmeum force-pushed the zbarsky/experiments branch from f39da84 to 4fb54e1 Compare August 11, 2024 19:21
@fmeum fmeum enabled auto-merge (squash) August 11, 2024 19:21
@fmeum fmeum merged commit 3b9a4bc into bazel-contrib:master Aug 11, 2024
2 checks passed
@jtacoma
Copy link

jtacoma commented Dec 31, 2024

Is this the right place to ask for suggestions about how to deal with this change? Asking because it is a backwards-incompatible change. I understand this is allowed between releases that share the same major version number if that number is zero, as it was in the previous v0.50.1 and following v0.51.0 releases. For leaf monorepos this might not be a real problem because they can switch from list-of-strings to comma-delimited-string in the same commit that upgrades from v0.50.1 to v0.51.0 (or later). Unfortunately, that's not an option for repos that (like this one) are used in many other repos.

For example, I'm trying out Bazel + Nix + Go in a hobby project where I ran into this issue when using rules_nixpkgs because its toolchains/go/go.bzl uses experiments. I considered sending a patch to update to the new API before realizing that this would force all users of rules_nixpkgs to upgrade rules_go to v0.51.0 or later. Any idea how to bridge this gap with minimal churn?

@fmeum
Copy link
Member

fmeum commented Dec 31, 2024

@jtacoma Thanks for raising this. What's the part of this PR that is backwards incompatible? My understanding is that it only modifies how attributes are passed around internally.

@jtacoma
Copy link

jtacoma commented Dec 31, 2024

I was a little uncertain since I noticed go_sdk is defined and used only within the "private" subdirectory of rules_go. However, it is also public in go/def.bzl. I ran into this because rules_nixpkgs loads go_sdk from @rules_go//go:def.bzl.

@dzbarsky
Copy link
Contributor Author

I was a little uncertain since I noticed go_sdk is defined and used only within the "private" subdirectory of rules_go. However, it is also public in go/def.bzl. I ran into this because rules_nixpkgs loads go_sdk from @rules_go//go:def.bzl.

Ah, we didn't realize that was exported. We should probably turn that export into a macro that rewrites the list into a comma-separated string to keep it backwards compatible.

@fmeum
Copy link
Member

fmeum commented Dec 31, 2024

@dzbarsky We could also handle the conversion in the go_sdk rule and don't change its attributes.

@dzbarsky
Copy link
Contributor Author

Yep, makes sense. I can send a PR Thursday to restore the interface

fmeum pushed a commit that referenced this pull request Jan 2, 2025
**What type of PR is this?**

Bug fix, see
#4022 (comment)

**What does this PR do? Why is it needed?**

Reverts the behavior to the API while keeping the optimization

**Which issues(s) does this PR fix?**

Fixes #

**Other notes for review**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants