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

Prevent downgrading platforms (#3071) #3076

Merged
merged 2 commits into from
Mar 31, 2022
Merged

Prevent downgrading platforms (#3071) #3076

merged 2 commits into from
Mar 31, 2022

Conversation

cpsauer
Copy link
Contributor

@cpsauer cpsauer commented Feb 25, 2022

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

It proposes one good way of preventing silent, problematic platform downgrade. Please see #3071 for context and other options. (But since it's had been a week without hearing back and others were having the issue, I figured I'd propose the fix that seemed the simplest and best.)

Note: Platforms already long guaranteed to be bundled with Bazel given the minimum Bazel version check of 4.2.1, and declaring an explicit version causes problems.

Which issues(s) does this PR fix?

Fixes #3071

Proposing one good way of preventing silent, problematic platform downgrade (see #3071).
Platforms already long guaranteed to be bundled with Bazel given the minimum bazel version check of 4.2.1.
@sluongng
Copy link
Contributor

sluongng commented Mar 8, 2022

I don't think deleting this would be the right fix.

Instead, the dependencies is loaded with _maybe() which will not load if there is an http_archive with the same named defined prior to the loading of go_rules_dependencies.

Have you tried defining a custom http_archive before go_rules_dependencies declaration?

@cpsauer
Copy link
Contributor Author

cpsauer commented Mar 8, 2022

Hi, @sluongng! Thanks for taking a look.

Well aware of being able to manually override the problematic _maybe with a custom http_archive :)

Manually importing is explicitly contemplated in the background issue (#3071), along with (in the days since) a stream of other users stumbling over it. The issue is instead that we almost certainly don't want to rely on users having to manually fight a problematic default that silently causes hard to debug problems. Lots more good discussion of that and other options in #3071.

Are there things we actually need the _maybe(platforms) for that outweigh its problems--or the complexity of a different fix?

[No substitute for reading #3071, but recall that _maybe() fails to detect the @platforms already bundled with Bazel, so it most often results in a downgrade for up-to-date users...and likely a fairly severe downgrade if ancient rules_go is being pulled in as a transitive dependency from, e.g., gRPC, as it often is. This is really quite hard to debug, because there's no reason users would expect an external repository to subtly break unrelated behavior. Others don't.]

All that said, definitely happy if you guys want to fix #3071 another way! As above, I just tossed this up as the simplest option that fixes things and because the user is guaranteed to have a pretty recent version of platforms anyway. (Not doing this because I need it for myself, but rather because I'd like to help others who will just keep running into this, leaving things better than I found them.)

Thanks so much for your consideration!
Chris
(ex-Googler)

@seh
Copy link
Contributor

seh commented Mar 9, 2022

I had tried pulling in the "platforms" repository myself (per #3071 (comment)) and it didn't solve this problem, but perhaps I pulled it in too late, after rules_go had set up its own version. At minimum that shows that it's possible to try and still fail. I agree with @cpsauer that users of rules_go shouldn't have to bear this burden. I expect rules_go should work fine on its own without me needing to patch it in the right way.

@cpsauer
Copy link
Contributor Author

cpsauer commented Mar 18, 2022

Friendly bump :)

Would love to get another reply--or approval--from someone with merge powers, looking at this with the additional context!

Not wedded to this fix, certainly, just trying to solve this before too many other people get tripped up by it. That said, this does seem like the most direct, sufficient fix.

Tagging @achew22 and @robfig, since it looks like they've most recently merged in non-release changes here. Hope that's okay!

@cpsauer
Copy link
Contributor Author

cpsauer commented Mar 29, 2022

Another friendly bump :)

@linzhp linzhp self-assigned this Mar 31, 2022
Copy link
Contributor

@linzhp linzhp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Bazel already has it, it's one less thing for rules_go to worry about. The only issues is when a new Bazel version is bundled with a @platforms not compatible with rules_go, we will have to update rules_go, but I guess that's inevitable anyways.

@linzhp linzhp merged commit def9dfb into bazel-contrib:master Mar 31, 2022
@cpsauer
Copy link
Contributor Author

cpsauer commented Mar 31, 2022

Sweet! Thanks so much @linzhp!

@cpsauer cpsauer deleted the patch-1 branch March 31, 2022 23:02
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.

Not bundling @platforms, or at least not downgrading it?
4 participants