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

Added a build setting for toolchain channels #1671

Merged
merged 6 commits into from
Dec 6, 2022

Conversation

UebelAndre
Copy link
Collaborator

@UebelAndre UebelAndre commented Nov 25, 2022

This pull request introduces the @rules_rust//rust/toolchain/channel build setting which is used to support transitioning targets to use a toolchain of a different Rust toolchain channels (e.g. "nightly").

By default rust_register_toolchains will now register a stable and nightly toolchain. The stable toolchain will be the default but users can switch to using the nightly toolchain by passing --@rules_rust//rust/toolchain/channel=nightly with their Bazel invocation.

This pull-request is intended to unlock future changes where transitions might be incorporated into existing rules to make accessing the right Rust tools more convenient.

@UebelAndre UebelAndre linked an issue Nov 25, 2022 that may be closed by this pull request
@UebelAndre
Copy link
Collaborator Author

This PR introduces select statements to toolchain definitions:

toolchain(
    name = "toolchain",
    exec_compatible_with = ["@platforms//cpu:x86_64","@platforms//os:osx"],
    target_compatible_with = ["@platforms//cpu:x86_64","@platforms//os:osx"],
    toolchain = select({"@rules_rust//rust/toolchain/channel:nightly":"@rust_darwin_x86_64__x86_64-apple-darwin_nightly_tools//:rust_toolchain","@rules_rust//rust/toolchain/channel:stable":"@rust_darwin_x86_64__x86_64-apple-darwin_stable_tools//:rust_toolchain"}),
    toolchain_type = "@rules_rust//rust:toolchain",
)

The conditions for these are defined as config settings that match on a build_setting rule I wrote in the //rust/toolchain/channel package:

config_setting(
    name = "nightly",
    flag_values = {
        ":channel": "nightly",
    },
    visibility = ["//visibility:public"],
)

config_setting(
    name = "stable",
    flag_values = {
        ":channel": "stable",
    },
    visibility = ["//visibility:public"],
)

I'm now confused as to why my test is failing at //test/unit/channel_transitions

exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //test/unit/channel_transitions:nightly_transition_test
-----------------------------------------------------------------------------
In test _nightly_transition_test_impl from //test/unit/channel_transitions:channel_transiitons_test.bzl: Expected ["bazel-out/k8-opt-exec-2B5CBBC6-ST-54f9b509cdab/bin/util/process_wrapper/process_wrapper", "--subst", "pwd=${pwd}", "--", "bazel-out/k8-fastbuild-ST-444e700f6c5e/bin/external/rust_linux_x86_64__x86_64-unknown-linux-gnu_stable_tools/rust_toolchain/bin/rustc", "test/unit/channel_transitions/lib.rs", "--crate-name=lib", "--crate-type=rlib", "--error-format=human", "--codegen=metadata=-2309820253", "--codegen=extra-filename=-2309820253", "--out-dir=bazel-out/k8-fastbuild-ST-444e700f6c5e/bin/test/unit/channel_transitions", "--codegen=opt-level=0", "--codegen=debuginfo=0", "--remap-path-prefix=${pwd}=", "--emit=dep-info,link", "--color=always", "--target=x86_64-unknown-linux-gnu", "-L", "bazel-out/k8-fastbuild-ST-444e700f6c5e/bin/external/rust_linux_x86_64__x86_64-unknown-linux-gnu_stable_tools/rust_toolchain/lib/rustlib/x86_64-unknown-linux-gnu/lib", "--edition=2018"] to contain -Zunstable-options

@illicitonion @scentini @krasimirgg Would any of you have an idea as to why the toolchain is not being transitioned?

@UebelAndre
Copy link
Collaborator Author

re: The issue was was resolved with the following diff.

diff --git a/test/unit/channel_transitions/channel_transiitons_test.bzl b/test/unit/channel_transitions/channel_transiitons_test.bzl
index 748b454a..668f2ca6 100644
--- a/test/unit/channel_transitions/channel_transiitons_test.bzl
+++ b/test/unit/channel_transitions/channel_transiitons_test.bzl
@@ -30,7 +30,7 @@ nightly_transition_test = analysistest.make(
     _nightly_transition_test_impl,
     doc = "Test that targets can be forced to use a nightly toolchain",
     config_settings = {
-        "@rules_rust//rust/toolchain/channel:channel": "nightly",
+        "@//rust/toolchain/channel:channel": "nightly",
     },
 )

@@ -41,7 +41,7 @@ stable_transition_test = analysistest.make(
     _stable_transition_test_impl,
     doc = "Test that targets can be forced to use a stable toolchain",
     config_settings = {
-        "@rules_rust//rust/toolchain/channel:channel": "stable",
+        "@//rust/toolchain/channel:channel": "stable",
     },
 )

The behavior around labels has been frustratingly inconsistent. I wish it was clearer what style should be used where or if wrapping a string in Label could be "correct" in all cases.

@katre
Copy link
Member

katre commented Nov 29, 2022

The label syntax is confusing. When you specify the label as @rules_rust//something, that means "a package in the rules_rust repo, which might be the main repo if it was named with workspace(name = "rules_rust") in WORKSPACE". On the other hand, the @//something syntax means "a package in the main repo", which might be what you mean in a test but also might not be what users want when they import your rules.

Can you use repo-relative labels like //something instead? Those assume that they refer to the current repo, whether that's the main or an imported repo, and might work better for you.

@UebelAndre
Copy link
Collaborator Author

Can you use repo-relative labels like //something instead? Those assume that they refer to the current repo, whether that's the main or an imported repo, and might work better for you.

I tried, repo-relative labels didn't work 😞

@katre
Copy link
Member

katre commented Nov 29, 2022

With respect to select and toolchain, I actually prefer to use the (poorly documented) target_setting attribute instead.

You currently have this toolchain:

toolchain(
    name = "toolchain",
    exec_compatible_with = ["@platforms//cpu:x86_64","@platforms//os:osx"],
    target_compatible_with = ["@platforms//cpu:x86_64","@platforms//os:osx"],
    toolchain = select({"@rules_rust//rust/toolchain/channel:nightly":"@rust_darwin_x86_64__x86_64-apple-darwin_nightly_tools//:rust_toolchain","@rules_rust//rust/toolchain/channel:stable":"@rust_darwin_x86_64__x86_64-apple-darwin_stable_tools//:rust_toolchain"}),
    toolchain_type = "@rules_rust//rust:toolchain",
)

This is equivalent to the following:

toolchain(
    name = "toolchain-nightly",
    exec_compatible_with = ["@platforms//cpu:x86_64","@platforms//os:osx"],
    target_compatible_with = ["@platforms//cpu:x86_64","@platforms//os:osx"],
    toolchain = "@rust_darwin_x86_64__x86_64-apple-darwin_nightly_tools//:rust_toolchain",
    target_settings = ["@rules_rust//rust/toolchain/channel:nightly"],
    toolchain_type = "@rules_rust//rust:toolchain",
)

toolchain(
    name = "toolchain-stable",
    exec_compatible_with = ["@platforms//cpu:x86_64","@platforms//os:osx"],
    target_compatible_with = ["@platforms//cpu:x86_64","@platforms//os:osx"],
    toolchain = "@rust_darwin_x86_64__x86_64-apple-darwin_stable_tools//:rust_toolchain",
    target_settings = ["@rules_rust//rust/toolchain/channel:stable"],
    toolchain_type = "@rules_rust//rust:toolchain",
)

The benefits of the second form is that it's clearer to users when they read it, it's clearer to rule authors when they want to edit it, and the two different toolchains are listed properly in toolchain debugging.

I need to document target_settings better, but the upshot is that it takes a list of config_setting targets, and the toolchain will only be used if all of them are true. You can, of course, use the expanded config logic from skylib to make more complicated structures.

@UebelAndre UebelAndre marked this pull request as ready for review November 29, 2022 16:17
@UebelAndre UebelAndre requested review from illicitonion and krasimirgg and removed request for illicitonion November 29, 2022 16:17
@UebelAndre
Copy link
Collaborator Author

UebelAndre commented Nov 29, 2022

re #1671 (comment)

Thanks for the suggestion @katre! I was able to cleanup some things to come up with a much nicer implementation. I had no idea toolchain.target_settings existed.

@scentini
Copy link
Collaborator

TIL about target_settings too. @katre, do you expect it to work well for selects where //conditions:default is involved? In other words, if we have a toolchain with target_settings and another one without, will Bazel choose the one with target_settings (assuming the underlying config_setting is True), or will there be ambiguity?

@katre
Copy link
Member

katre commented Nov 29, 2022

If you have a toolchain with a target setting that matches the config, and one without, then the normal toolchain mechanism of registration order will apply. Note that if you register toolchains using //package:all they are registered in the order they are declared in the BUILD file.

@UebelAndre
Copy link
Collaborator Author

Anyone have time to review this this week? 😅

@krasimirgg
Copy link
Collaborator

Anyone have time to review this this week? 😅

Thank you for the ping! I will review this tomorrow.

Copy link
Collaborator

@krasimirgg krasimirgg left a comment

Choose a reason for hiding this comment

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

Looks good! I've got a few nits.

crate_universe/private/common_utils.bzl Show resolved Hide resolved
rust/repositories.bzl Show resolved Hide resolved
rust/repositories.bzl Show resolved Hide resolved
Copy link
Collaborator

@krasimirgg krasimirgg left a comment

Choose a reason for hiding this comment

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

Thank you!

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.

Transitioning on toolchain channels
4 participants