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

Getting architecture from JRE #3282

Merged
merged 2 commits into from
Sep 10, 2022
Merged

Getting architecture from JRE #3282

merged 2 commits into from
Sep 10, 2022

Conversation

linzhp
Copy link
Contributor

@linzhp linzhp commented Sep 8, 2022

What type of PR is this?
Bug fix

What does this PR do? Why is it needed?
This PR makes rules_go gets the host architecture repository_ctx, which in turn get it from JVM. This is because Bazel determines default host platform from its JVM https://github.com/bazelbuild/bazel/blob/a380a157f658cc392f66f86b6aba340c461dc408/src/main/java/com/google/devtools/build/lib/bazel/repository/LocalConfigPlatformFunction.java#L63 and https://github.com/bazelbuild/bazel/blob/577cfbc8a2b0841cd67a918febe9e3c5fcb3882e/src/main/java/com/google/devtools/build/lib/util/CPU.java#L56

If rules_go determines the host platform by calling uname, we may run into an interesting situation when users accidentally runs an ARM64 version of Bazel binary, with comes with ARM64 JVM, in a terminal with Rosetta translation:

  • rules_go downloads an AMD64 version of Go SDK, because uname returns "amd64"
  • Bazel tries to find a Go toolchain for ARM64, because JVM tells Bazel that it's running in ARM64

Leading to:

no matching toolchains found for types @io_bazel_rules_go//go:toolchain

Which issues(s) does this PR fix?

Fixes #3086

Other notes for review
Linux and Windows shouldn't have this problem, but getting arch from repository_ctx is much simpler than their current implementation. So I change all of them together. I don't have access to all these OS/Arch combination to verify. So I just optimistically assume that they would work. We can always override the arch names as needed in the future.

@linzhp linzhp requested a review from fmeum September 8, 2022 05:14
@linzhp linzhp marked this pull request as draft September 8, 2022 05:16
Copy link
Collaborator

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

I'm all for this change, but it would require us to raise the minimum Bazel version requirement (I believe to 5.2.0 or 5.3.0). Since 5.3.0 would buy us many other nice things, that could be worth it, but I can't tell whether it's too early for that change.

@linzhp
Copy link
Contributor Author

linzhp commented Sep 8, 2022

5.1 is sufficient for this to work: https://docs.bazel.build/versions/5.1.0/skylark/lib/repository_os.html

I am still debating with myself whether to bump the minimal version (given that 5.1.0 is half year old), or check the existence of repository_os.arch and keep the old logic if the attribute is not available.

@fmeum
Copy link
Collaborator

fmeum commented Sep 8, 2022

Should have known that since I added it myself ;)

I'm personally in favor of bumping the minimum version as we don't know of any other important bugs that would require fixing for people stuck on Bazel 4. But I also never maintained a large monorepo.

@linzhp
Copy link
Contributor Author

linzhp commented Sep 8, 2022

Uber's Go monorepo has been using Bazel 5.2 for quite a while. So it won't help discover any recent rules_go bug with Bazel 4 either.

@linzhp linzhp marked this pull request as ready for review September 9, 2022 23:37
@linzhp linzhp merged commit cf20167 into master Sep 10, 2022
@linzhp linzhp deleted the arch branch September 10, 2022 14:28
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.

no matching toolchains found for types @io_bazel_rules_go//go:toolchain on DARWIN ARM64
2 participants