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

Allow explicitly specifying Linux distribution #327

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

illicitonion
Copy link

@illicitonion illicitonion commented May 10, 2024

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.

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.
Copy link
Collaborator

@rrbutani rrbutani left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +51 to +52
"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."),
Copy link
Collaborator

@rrbutani rrbutani May 10, 2024

Choose a reason for hiding this comment

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

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).

Comment on lines +101 to +103
if not rctx.attr.exec_os:
(distname, version) = _linux_dist(rctx)
return distname, version, _arch
Copy link
Collaborator

@rrbutani rrbutani May 10, 2024

Choose a reason for hiding this comment

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

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:

Suggested change
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.

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.

2 participants