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

Provide the host CPU to repository rules #14685

Closed
uhthomas opened this issue Feb 2, 2022 · 19 comments
Closed

Provide the host CPU to repository rules #14685

uhthomas opened this issue Feb 2, 2022 · 19 comments
Labels
help wanted Someone outside the Bazel team could own this P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: feature request

Comments

@uhthomas
Copy link
Contributor

uhthomas commented Feb 2, 2022

Description of the problem / feature request:

Repository rules already have access to repository_os which allows rules to change their behaviour based on the exec OS. Repository rules should also know the exec architecture. Repository rules currently work around this limitation by uname -m, which isn't ideal. See rules_go.

Feature requests: what underlying problem are you trying to solve with this feature?

Repository rules create potentially brittle and strange workarounds to get the exec CPU. There is no standardisation and no official guidance on how best to create repository rules which target multiple platforms.

What operating system are you running Bazel on?

Linux x86_64

What's the output of bazel info release?

❯ bazel info release
Starting local Bazel server and connecting to it...
release 5.0.0

Have you found anything relevant by searching the web?

Replace this line with your answer.

Any other information, logs, or outputs that you want to share?

N/A

@brentleyjones
Copy link
Contributor

brentleyjones commented Feb 2, 2022

I've found the need for this as well, when trying to target x86_64 macOS RBE from arm64 macOS. Repository rules incorrectly provide arm64 binaries which fail to run on RBE.

@uhthomas
Copy link
Contributor Author

uhthomas commented Feb 2, 2022

Also see rules_rust.

@gregestren gregestren added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. untriaged type: feature request labels Feb 2, 2022
@Wyverald Wyverald added help wanted Someone outside the Bazel team could own this P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels Feb 7, 2022
@Wyverald
Copy link
Member

Wyverald commented Feb 7, 2022

This seems like a reasonable request. It shouldn't be too hard to add a field to repository_os, so if anyone wants to send a PR, it would be greatly appreciated.

@fmeum
Copy link
Collaborator

fmeum commented Feb 7, 2022

I started working on this in #14738, but then noticed that this is more subtle than expected: The natural implementation would be to just return System.getProperty("os.arch").toLowerCase(Locale.ROOT), but that is just the architecture of the JDK Bazel is running on, which is not necessarily the same as the architecture of the operating system Bazel is running on. For example, which value should this have in the case of an x86_64 JDK executing Bazel using Rosetta on an aarch64/arm64 M1 Mac?

@uhthomas @brentleyjones Could you clarify what exactly you would want to know the architecture of? You also referred to RBE, but I think that repository rules could only ever provide the host CPU, not the exec CPU (which isn't even known during the loading phase since it depends on the result of toolchain resolution). Is that sufficient for your use cases?

@keith
Copy link
Member

keith commented Feb 7, 2022

For a M1 mac running through Rosetta x86_64 is right IMO, since the emulation should affect everything.

@uhthomas
Copy link
Contributor Author

uhthomas commented Feb 7, 2022

The intent of this issue is to solve the multi cpu/os repository problem where rule maintainers are forced to write odd hacks to detect the exec (host?) CPU. For example the Go rules declare a signal repository, go_sdk, which will automatically detect the correct SDK to use. The go_download_sdk() rule uses uname to do this detection.

It's been suggested that multiple repositories for the corresponding cpu and os should be declared (ref), but this can be really messy and weird.

select does not work with repository rules and there is no clear guidance on how to actually do this, which is odd considering this is extremely common.

The M1 situation is a bit weird and I don't really know what the right answer is. I think it should depend on whether the architecture of the Bazel binary, as there is no other way to differentiate between native and rosetta at current.

Does what I've written make sense, and is it clear what the goal of this issue is?

@brentleyjones
Copy link
Contributor

brentleyjones commented Feb 7, 2022

Could you clarify what exactly you would want to know the architecture of? You also referred to RBE, but I think that repository rules could only ever provide the host CPU, not the exec CPU (which isn't even known during the loading phase since it depends on the result of toolchain resolution). Is that sufficient for your use cases?

I would like the know the architecture of --host_platform. If I don't run Bazel through Rosetta, all repository rules will see arm64 on an M1 Mac, even if I set --host_platform to a platform with x86_64 cpu.

@fmeum
Copy link
Collaborator

fmeum commented Feb 7, 2022

The architecture of the Bazel binary, at least assuming it uses the shipped JDK, the default cpu constraint on the auto-detected host platform and the value of the os.arch Java system property are all the same as far as I know, so @uhthomas and @keith requests would be handled by my PR.

@brentleyjones I understand your use case, but this seems difficult to do: Strictly speaking, the platform specified by --host_platform is an opaque collection of constraint values, which may theoretically not even contain one for the constraint setting @platforms//cpu. And even if we come up with logic for this (admittedly very special) case, I fear getting a constraint value of a platform identified by a label from the context of a repository rule is pretty difficult.

@uhthomas
Copy link
Contributor Author

uhthomas commented Feb 7, 2022

Awesome @fmeum, your PR looks great. Thanks for taking a look at this! :)

Is there some reference which lists the possible values of os.arch and os.name?

For my own sanity also, are repository rules executed on the host platform, or the execution platform? More specifically, will the values of os.arch and os.name be of the host platform, or execution platform?

@uhthomas
Copy link
Contributor Author

uhthomas commented Feb 7, 2022

Additionally, how does this all fit into platforms? If at all. I think it might be strange to use the Platforms API in most cases, but the repository_os struct in others. What are your thoughts?

@fmeum
Copy link
Collaborator

fmeum commented Feb 7, 2022

Is there some reference which lists the possible values of os.arch and os.name?

The two sources I usually refer to are the OpenJDK sources and Bazel's CPU and OS classes.

For my own sanity also, are repository rules executed on the host platform, or the execution platform? More specifically, will the values of os.arch and os.name be of the host platform, or execution platform?

Additionally, how does this all fit into platforms? If at all. I think it might be strange to use the Platforms API in most cases, but the repository_os struct in others. What are your thoughts?

My understanding is that repository rules are always executed locally (i.e., on the host), even with RBE, and don't really have a concept of platforms. Rather, platforms could be used to select the correct repository among the instances of a repository rule for the different platforms. Bazel would then only fetch the repository rules that are relevant for the current configuration. In this context, repository_os is more of a shortcut geared towards local execution, whereas sophisticated RBE setups would probably favor the approach described in #11655 (comment).

@meteorcloudy
Copy link
Member

I think #14738 solves exactly the request of this issue. The repository_ctx.os.name and repository_ctx.os.arch return the values for the host platform (which is the one Bazel runs on), this should be enough as in most cases repository rules run locally.

But we do have a way to make repository.execute run commands on RBE, but this is a very much experimental feature and currently unmaintained.

I also agree if Bazel runs in a simulation it should just get the simulated arch. We do provide native arm64 binaries for Linux and macOS, arm64 support for Windows has also been added recently.

@brentleyjones
Copy link
Contributor

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Feb 8, 2022
@brentleyjones
Copy link
Contributor

(I miss-read what that command was for, this isn't a blocker, sorry.)

@meteorcloudy meteorcloudy removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Feb 8, 2022
@meteorcloudy
Copy link
Member

meteorcloudy commented Feb 8, 2022

OK, I removed the label. But I guess we could still cherry-pick this to 5.1? @Wyverald

@brentleyjones
Copy link
Contributor

Yeah, that was what I wanted to do. Per the process, it seems we just open the PR instead of flagging it.

@meteorcloudy
Copy link
Member

meteorcloudy commented Feb 8, 2022

@bazel-io fork 5.1

@meteorcloudy
Copy link
Member

The fork command will create an associate issue to track the cherry-pick, feel free to send a PR!

@uhthomas uhthomas changed the title Provide the exec CPU to repository rules Provide the host CPU to repository rules Feb 8, 2022
@uhthomas
Copy link
Contributor Author

uhthomas commented Feb 8, 2022

Amazing, thank you for the work on this. I look forward to using it! I'll make contributions for the major rules to also take advantage of this new attribute when available.

For clarification; this only provides info for the host OS, correct? What happens in a remote execution context? Are those actions still executed locally? I suppose it depends on the declared toolchains.

Is there any official guidance on how best to write repository rules for compatibility with remote execution? It seems to me like it may still be the case that rule authors must declare every cpu/os combination as repositories and reference them via toolchains.

Maybe I'm missing something 🙂

brentleyjones pushed a commit to brentleyjones/bazel that referenced this issue 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 issue 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
help wanted Someone outside the Bazel team could own this P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants