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

Add "arch" struct field to repository_os #14738

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Feb 7, 2022

This new field provides access to the Java "os.arch" property to
repository rules, which previously had to rely on uname (Unix) or
additional env variables (Windows) to detect the architecture.

This also fixes a small issue in the existing implementation of
repository_ctx.os.name, which should use the root locale when converting
the value of the "os.name" property to lowercase.

Existing sites of manual architecture detection in shipped repository
rules as well as redundant calls to lower() on the value of
repository_ctx.os.name are cleaned up.

Fixes #14685

This new field provides access to the Java "os.arch" property to
repository rules, which previously had to rely on uname (Unix) or
additional env variables (Windows) to detect the architecture.

This also fixes a small issue in the existing implementation of
repository_ctx.os.name, which should use the root locale when converting
the value of the "os.name" property to lowercase.

Existing sites of manual architecture detection in shipped repository
rules as well as redundant calls to lower() on the value of
repository_ctx.os.name are cleaned up.
@Wyverald
Copy link
Member

Wyverald commented Feb 7, 2022

Looks great, thank you! Any more changes planned before you un-draft this?

@fmeum
Copy link
Collaborator Author

fmeum commented Feb 7, 2022

@Wyverald The implementation is complete, I'm just not sure whether it does the right thing: As discussed in #14685, os.arch depends on the architecture of the JDK, which may not agree with the architecture of the system it is running on (think Rosetta). With this PR, it would thus be possible to create repository rules that behave differently depending on the architecture of the Bazel binary. Do you think that's fine to ignore as an edge case?

@Wyverald
Copy link
Member

Wyverald commented Feb 7, 2022

Ah, I see. It seems fine to me to use os.arch since we are running in a JVM after all, but let's wait for consensus on the other thread. cc @meteorcloudy

@meteorcloudy
Copy link
Member

meteorcloudy commented Feb 8, 2022

Nice, thanks for adding this!

@katre With this change, we probably can starlarkify https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/bazel/repository/LocalConfigPlatformFunction.java ? (Although the exact logic of getting OS and CPU is a bit different that just checking os.name and os.arch)

@fmeum fmeum marked this pull request as ready for review February 8, 2022 10:40
@fmeum
Copy link
Collaborator Author

fmeum commented Feb 8, 2022

@meteorcloudy Marked as ready, thanks for the review.

@bazel-io bazel-io closed this in 32d1606 Feb 8, 2022
@katre
Copy link
Member

katre commented Feb 8, 2022

@meteorcloudy Yes, we could possibly use this to rewrite LocalConfigPlatformFunction, although it's at such a low level in the hierarchy of dependencies I'd be very worried about correctness and performance.

brentleyjones pushed a commit to brentleyjones/bazel that referenced this pull request Feb 16, 2022
This new field provides access to the Java "os.arch" property to
repository rules, which previously had to rely on uname (Unix) or
additional env variables (Windows) to detect the architecture.

This also fixes a small issue in the existing implementation of
repository_ctx.os.name, which should use the root locale when converting
the value of the "os.name" property to lowercase.

Existing sites of manual architecture detection in shipped repository
rules as well as redundant calls to lower() on the value of
repository_ctx.os.name are cleaned up.

Fixes bazelbuild#14685

Closes bazelbuild#14738.

PiperOrigin-RevId: 427147225
(cherry picked from commit 32d1606)
Wyverald pushed a commit that referenced this pull request Feb 16, 2022
This new field provides access to the Java "os.arch" property to
repository rules, which previously had to rely on uname (Unix) or
additional env variables (Windows) to detect the architecture.

This also fixes a small issue in the existing implementation of
repository_ctx.os.name, which should use the root locale when converting
the value of the "os.name" property to lowercase.

Existing sites of manual architecture detection in shipped repository
rules as well as redundant calls to lower() on the value of
repository_ctx.os.name are cleaned up.

Fixes #14685

Closes #14738.

PiperOrigin-RevId: 427147225
(cherry picked from commit 32d1606)

Co-authored-by: Fabian Meumertzheim <[email protected]>
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.

Provide the host CPU to repository rules
4 participants