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.

Sign up for GitHub

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
Open

Allow explicitly specifying Linux distribution #327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions toolchain/internal/common.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,14 @@ def os_version_arch(rctx):
_os = os(rctx)
_arch = arch(rctx)

if _os == "linux" and not rctx.attr.exec_os:
(distname, version) = _linux_dist(rctx)
return distname, version, _arch
if _os == "linux":
if (rctx.attr.exec_linux_distribution == "") != (rctx.attr.exec_linux_distribution_version == ""):
fail("Either both or neither of linux_distribution and linux_distribution_version must be set")
if rctx.attr.exec_linux_distribution and rctx.attr.exec_linux_distribution_version:
return rctx.attr.exec_linux_distribution, rctx.attr.exec_linux_distribution_version, _arch
if not rctx.attr.exec_os:
(distname, version) = _linux_dist(rctx)
return distname, version, _arch
Comment on lines +101 to +103
Copy link
Collaborator

@rrbutani rrbutani May 10, 2024 •
edited
Loading

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.


return _os, "", _arch

Expand Down
16 changes: 15 additions & 1 deletion toolchain/internal/repo.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ load(

_target_pairs = ", ".join(_supported_os_arch_keys())

# Atributes common to both `llvm` and `toolchain` repository rules.
# Attributes common to both `llvm` and `toolchain` repository rules.
common_attrs = {
"llvm_versions": attr.string_dict(
mandatory = False,
Expand All @@ -43,6 +43,20 @@ common_attrs = {
mandatory = False,
doc = "Execution platform architecture, if different from host arch.",
),
"exec_linux_distribution": attr.string(
mandatory = False,
doc = ("Linux distribution the exec platform is running (or can compatibly run binaries for). " +
"This attribute is ignored if exec_os != 'linux', " +
"and if it's set so must exec_linux_distribution_verison be. " +
"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."),
Comment on lines +51 to +52
Copy link
Collaborator

@rrbutani rrbutani May 10, 2024 •
edited
Loading

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

),
"exec_linux_distribution_version": attr.string(
mandatory = False,
doc = ("The version number corresponding to exec_linux_distribution, " +
"in whatever format the distribution uses " +
"(e.g. for ubuntu this may be 20.04, for rhel this may be 8.4, etc)."),
),
}

llvm_repo_attrs = dict(common_attrs)
Expand Down
Loading