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

Cannot override build scripts via environment variables #11041

Closed
glandium opened this issue Aug 31, 2022 · 3 comments · Fixed by #11139
Closed

Cannot override build scripts via environment variables #11041

glandium opened this issue Aug 31, 2022 · 3 comments · Fixed by #11139
Labels
A-configuration Area: cargo config files and env vars A-documenting-cargo-itself Area: Cargo's documentation C-bug Category: bug

Comments

@glandium
Copy link
Contributor

Problem

https://doc.rust-lang.org/cargo/reference/config.html#environment-variables says:

Cargo can also be configured through environment variables in addition to the TOML configuration files. For each configuration key of the form foo.bar the environment variable CARGO_FOO_BAR can also be used to define the value.

This unfortunately isn't true for e.g. target.x86_64-unknown-linux-gnu.foo.rustc_flags.

Cc: @thomcc

Steps

  1. cargo new testcase --lib
  2. cd testcase
  3. Edit Cargo.toml to add links = "foo"
  4. echo "fn main() { panic!(); }" > build.rs
  5. env CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_FOO_RUSTC_FLAGS="" cargo build (assuming you're on x86_64-unknown-linux-gnu)

The build script will still be executed.

The environment variable is recognized by cargo config, which could be a source of confusion.

$ env CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_FOO_RUSTC_FLAGS="" cargo +nightly -Z unstable-options config get target.x86_64-unknown-linux-gnu.foo.rustc_flags
target.x86_64-unknown-linux-gnu.foo.rustc_flags = ""

Obviously, setting the same variable via .cargo/config.toml works.

Possible Solution(s)

cargo::util::config::load_config_table calls cargo::util::config::Config::get_table, which explicitly leaves taking environment variables into account to its callers, but it doesn't. Making either of those handle environment variables would solve the problem. If I didn't miss anything, the only two other uses of Config::get_table are ConfigMapAccess::new_map and ConfigMapAccess::new_struct, and the former handles environment variables. I'm not sure why the latter doesn't, but maybe it should, at which point it would seem like get_table should be doing it.

Notes

No response

Version

No response

@glandium glandium added the C-bug Category: bug label Aug 31, 2022
@ehuss
Copy link
Contributor

ehuss commented Sep 1, 2022

Unfortunately tables with open-ended keys cannot be easily fetched via environment variables. #5416 contains a deeper discussion of the problem.

This is generally a duplicate of #5416. However, I think it would be good to make the documentation a little clearer that there are some limitations. Any entry in the config page that doesn't explicitly list the the Environment: field don't support environment variables. The Environment variables section should probably have a small note that not all config fields are supported.

@ehuss ehuss added A-documenting-cargo-itself Area: Cargo's documentation A-configuration Area: cargo config files and env vars labels Sep 1, 2022
@ehuss
Copy link
Contributor

ehuss commented Sep 1, 2022

And #9301 discusses the issues with cargo config. Currently it just blindly looks for env vars starting with CARGO_. It can't know which environment variables will actually get used.

@glandium
Copy link
Contributor Author

glandium commented Sep 1, 2022

#5416 has a discussion about lists, and I can see how that wouldn't work, but I don't see open-ended keys being a huge problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-documenting-cargo-itself Area: Cargo's documentation C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants