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

Support environment variable expansion in config.toml #10789

Open
avdb13 opened this issue Jun 25, 2022 · 9 comments
Open

Support environment variable expansion in config.toml #10789

avdb13 opened this issue Jun 25, 2022 · 9 comments
Labels
A-configuration Area: cargo config files and env vars A-environment-variables Area: environment variables C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@avdb13
Copy link

avdb13 commented Jun 25, 2022

Problem

Currently, the following doesn't seem to work, it gets resolved to a relative path without parsing the environment variable:

[build]
rustc-wrapper = "$HOME/.cargo/bin/sccache"

Proposed Solution

match the filepath strings in Cargo with regex to find the environment variables.

Notes

No response

@avdb13 avdb13 added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Jun 25, 2022
@weihanglo
Copy link
Member

weihanglo commented Jun 25, 2022

AFAIK, Cargo does not support environment variable expansion like shell does in most fields of its configuration file at this moment. Environment variable expansion is a big topic containing lots of different syntax flavours and mechanisms. Not to mention the interaction with [env] config and unstable feature -Z advanced-env might get thing even more complicated.

Alternatively, you may circumvent it with the following approaches:

  • Use RUSTC_WRAPPER directly
    RUSTC_WRAPPER="$HOME/.cargo/bin/sccache" cargo check
  • Use --config flag, which just get stabilized on nightly channel (cargo 1.63.0-nightly (a5e08c470 2022-06-23))
    cargo +nightly --config "build.rustc-wrapper='$HOME/.cargo/bin/sccache'" check

Hope it help :)

@weihanglo weihanglo added A-configuration Area: cargo config files and env vars A-environment-variables Area: environment variables labels Jun 25, 2022
@weihanglo weihanglo changed the title Add environment variable support in config.toml Support environment variable expansion in config.toml Jun 25, 2022
@weihanglo
Copy link
Member

Pardon for my editing the issue title. This should make the intent slightly prominent.

mdaverde added a commit to bpfdeploy-io/bpf-rs that referenced this issue Sep 23, 2022
bpf-rs & bpf-feature tests need capabilities (to read certain directories
or load bpf programs) that are not available to a regular user.

This is strictly possible by just wrapping `cargo test` with `sudo` (such
as with `sudo -E "PATH=$PATH" $(which cargo) test --all-features`) but the
issue here (besides ergonomics) is when cargo tests build & write new
files they are now owned by root (making future non-sudo builds fail). It
might be possible to configure sudo to strictly write new files as a
separate user from root but I haven't seen that.

Ideally, we just run the created test bins with sudo after compilation and
this is possible with specifying a runner in .cargo/config.toml. We can
specify this at the workspace level for the relevant target triple but the
issue with this is that sudo disallows certain libraries to be dynamically
linked which the proc_macro crate (used transitively by bpf-rs-macros) needs.
So we're left with the option of forcing sudo to allow loading of these
libraries or to only using sudo on the crate tests that need it.

The problem is that cargo itself changes LD_LIBRARY_PATH: https://doc.rust-lang.org/cargo/reference/environment-variables.html#dynamic-library-paths
which we don't have access to from the runner's perspective, especially
given that variable shell expansion is not supported: rust-lang/cargo#10789

For now, we're choosing the latter (only using sudo on specific crates)
but we ran into another issue: config.toml is ignored on a per-crate basis when running
cargo test from a workspace. To workaround this, we change our local task
`just test` to cd into each crate and run tests from there. This is not
ideal but gets the job done for now.

We keep our github actions to just run all the tests as the original sudo
cmd for now since we don't care about the owners of the new files after
the vm disappears.

Related to #7

Signed-off-by: Milan <milan@mdaverde.com>
@nmittler
Copy link

nmittler commented Nov 16, 2022

I have another use case for this. I'd like to set the value of an environment variable in .cargo/config.toml to have the target os and arch embedded in it (e.g. some/path/${CARGO_CFG_TARGET_OS}_${CARGO_CFG_TARGET_ARCH}.

I'm not entirely sure, however, if the variables would even be available at the time of evaluation. So this might be a slightly harder problem to solve in that case.

@nmittler
Copy link

@weihanglo this combined with #10273 adds a lot of capability to the configuration system. We'd be able to use environment variables within expressions (this issue) and also set them conditionally (#10273).

@asdf8dfafjk
Copy link

asdf8dfafjk commented Jan 11, 2023

@weihanglo Is there a tracking issue/RFC of sorts regarding this? I ask because I've only ever seen everything decided in a "solid" manner in rust community (except async lol IMHO) and would love to read all the nuances. (And of course i'm also suffering from a lack of env var support in both Cargo.toml and .cargo/config.toml)

Talking specifically about #10789 (comment)

@weihanglo
Copy link
Member

@asdf8dfafjk AFAIK no. This is probably the only place to discuss environment variable expansion I am aware of. There might be place like URLO having such a discussion. If you are going to bake an RFC from your side, kindly recommend following the guide.

@asdf8dfafjk
Copy link

@weihanglo Sadly both my knowledge and rigor wouldn't be up to rust's standard's. My motivation behind asking the question was a curiousity about the nuances of environment variables but upon further thought I suspect there is also an issue of config variable evaluation in a way that is not confusing. On that note, here's a library https://github.com/japgolly/clear-config that seems to be well liked in the scala ecosystem (it can serve as a good inspiration)

@jonhoo
Copy link
Contributor

jonhoo commented Mar 9, 2023

I think this is also related to #8801 in a way

@joshtriplett
Copy link
Member

If we decide to have some kind of expansion (environment variable or otherwise), we should mark things that get expanded, rather than just allowing $VAR in all or some subset of strings by default.

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-environment-variables Area: environment variables C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

No branches or pull requests

6 participants