Skip to content

add rustc setting use_default_shell_env#4009

Open
oliverlee wants to merge 1 commit intobazelbuild:mainfrom
oliverlee:rustc-use-default-shell-env
Open

add rustc setting use_default_shell_env#4009
oliverlee wants to merge 1 commit intobazelbuild:mainfrom
oliverlee:rustc-use-default-shell-env

Conversation

@oliverlee
Copy link
Copy Markdown

//rust/settings:use_default_shell_env is a bool flag that controls whether rustc actions inherit the shell environment. The default is False to maintain existing behavior.

This allows use in environments where PATH is set outside of Bazel's action environment, e.g. when using a Nix-based CcToolchain that relies on a Nix shell to populate PATH.

`//rust/settings:use_default_shell_env` is a bool flag that controls
whether rustc actions inherit the shell environment. The default is
`False` to maintain existing behavior.

This allows use in environments where PATH is set outside of Bazel's
action environment, e.g. when using a Nix-based CcToolchain that relies
on a Nix shell to populate PATH.
@oliverlee
Copy link
Copy Markdown
Author

Related:
#2177
#3482
#3556
#3535

Copy link
Copy Markdown
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Thanks! This seems like a reasonable compromise 😄

Just one question.

Comment thread rust/private/rust.bzl
"_rustc_output_diagnostics": attr.label(
default = Label("//rust/settings:rustc_output_diagnostics"),
),
"_use_default_shell_env": attr.label(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would it make more sense to have this on rust_toolchain?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm fine with either a toolchain flag or a global setting, although it seems unlikely a project would have multiple Rust toolchains where only some need the default shell env.

However, I don't have deep rules_rust experience, so feel free to disregard — at my day job I use Rust to build some tooling, though only ever with a single Rust toolchain.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We currently have a big number of flags on the toolchain and feel it's much easier to manage having it there

# Experimental and incompatible flags
_rename_first_party_crates = rename_first_party_crates,
_third_party_dir = third_party_dir,
_pipelined_compilation = pipelined_compilation,
_experimental_link_std_dylib = _experimental_link_std_dylib(ctx),
_experimental_use_cc_common_link = _experimental_use_cc_common_link(ctx),
_experimental_use_global_allocator = experimental_use_global_allocator,
_experimental_use_coverage_metadata_files = ctx.attr._experimental_use_coverage_metadata_files[BuildSettingInfo].value,
_toolchain_generated_sysroot = ctx.attr._toolchain_generated_sysroot[BuildSettingInfo].value,
_incompatible_do_not_include_data_in_compile_data = ctx.attr._incompatible_do_not_include_data_in_compile_data[IncompatibleFlagInfo].enabled,
_incompatible_do_not_include_transitive_data_in_compile_inputs = ctx.attr._incompatible_do_not_include_transitive_data_in_compile_inputs[IncompatibleFlagInfo].enabled,
_no_std = no_std,
_codegen_units = ctx.attr._codegen_units[BuildSettingInfo].value,
_experimental_use_allocator_libraries_with_mangled_symbols = ctx.attr.experimental_use_allocator_libraries_with_mangled_symbols,
_experimental_use_allocator_libraries_with_mangled_symbols_setting = ctx.attr._experimental_use_allocator_libraries_with_mangled_symbols_setting[BuildSettingInfo].value,

Both work but if there's no string feelings or technical rationale (which I'm amenable to here) I'd prefer to have it on the toolchain

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure I can take a look at it next week.

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