Allow explicitly specifying Linux distribution #327
Conversation
Without this, if you want to do remote execution from a non-Linux (or
different Linux) platform to a Linux platform, you need to explicitly
fill in the URLs attribute.
With this, you can write something like:
```
llvm_toolchain(
name = "llvm_toolchain_linux_x86_64",
llvm_version = "14.0.0",
exec_os = "linux",
exec_arch = "x86_64",
exec_linux_distribution = "ubuntu",
exec_linux_distribution_version = "18.04",
sysroot = {
"linux-x86_64": "@org_chromium_sysroot_linux_x64//:sysroot",
},
)
```
which will work when remote building from macOS, because it won't
attempt to read the Linux distribution from `/etc/os-release` which
doesn't exist on macOS.
There was a problem hiding this comment.
Thanks for the PR! This looks good to me, just a couple of nits.
A comment on the new attrs: in retrospect I think it would have been nicer if we'd bundled the exec_* override attrs into a single list attr:
llvm_toolchain(
...,
exec_platform = ["linux", "x86_64", "ubuntu", "18.04"],
)llvm_toolchain(
...,
exec_platform = ["darwin", "aarch64"],
)^ would let us sidestep some of the awkward permutations that are allowed (i.e. setting linux_distribution and linux_distribution_version w/o setting exec_os to linux explicitly)
(as an aside, @illicitonion: thanks for covering these in the doc strings)
But at this point it's probably not worth making a breaking change over.
| "If not set, and both the exec_os and the host platform are linux, " + | ||
| "an attempt will be made to discover and use the local host platform."), |
There was a problem hiding this comment.
Unfortunately I think we actually only try to autodetect the host distro + version if the host platform is Linux and exec_os is not specified.
What you've described seems preferable though, IMO (see below).
| if not rctx.attr.exec_os: | ||
| (distname, version) = _linux_dist(rctx) | ||
| return distname, version, _arch |
There was a problem hiding this comment.
To read /etc/os-release when exec_os = "linux" + the host platform is Linux (not just when exec_os = None and the host is Linux) I think we'd want:
| if not rctx.attr.exec_os: | |
| (distname, version) = _linux_dist(rctx) | |
| return distname, version, _arch | |
| if rctx.os.name == "linux": | |
| (distname, version) = _linux_dist(rctx) | |
| return distname, version, _arch |
I don't feel strongly about whether we should do the above or change the docs on exec_linux_distribution to describe the existing behavior but we should probably do one or the other.
|
Having the same issue with remote builds right now :( Any idea about priority on this one? |
3 participants
Without this, if you want to do remote execution from a non-Linux (or different Linux) platform to a Linux platform, you need to explicitly fill in the URLs attribute.
With this, you can write something like:
which will work when remote building from macOS, because it won't attempt to read the Linux distribution from
/etc/os-releasewhich doesn't exist on macOS.