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

Add support for [env] section in .cargo/config.toml #9175

Merged
merged 4 commits into from
Feb 23, 2021

Conversation

wickerwaka
Copy link
Contributor

@wickerwaka wickerwaka commented Feb 15, 2021

This adds support for an [env] section in the config.toml files. Environment variables set in this section will be applied to the environment of any processes executed by cargo.

This is implemented to follow the recommendations in #8839 (comment)

Variables have optional force and relative flags. force means the variable can override an existing environment variable. relative means the variable represents a path relative to the location of the directory that contains the .cargo/ directory that contains the config.toml file. A relative variable will have an absolute path prepended to it before setting it in the environment.

[env]
FOOBAR = "Apple"
PATH_TO_SOME_TOOL = { value = "bin/tool", relative = true }
USERNAME = { value = "test_user", force = true }

Fixes #4121

This adds support for an `[env]` section in the config.toml files. Environment variables set in this section will be applied to the environment of any processes executed by cargo.

```
[env]
FOOBAR = "Apple"
PATH_TO_SOME_TOOL = { value = "bin/tool", relative = true }
USERNAME = { value = "test_user", force = true }
```
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 15, 2021
Fix formatting errors
Added the `-Z configurable-env` option which controls whether the data from the [env] section is used. Updated the tests to pass the flag and to masquerade as the nightly cargo.
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Since it's been a few months since I've at least thought about this could you detail your expected use case with this as well? I'm not sure if this should close #4121 since that seems like a different feature request for Cargo

@@ -1244,6 +1248,39 @@ impl Config {
&self.progress_config
}

/// Create an EnvConfigValue hashmap from the "env" table
fn get_env_config(&self) -> CargoResult<EnvConfig> {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to do custom serde deserialization (e.g. via #[serde(untagged)]) instead of having custom deserialization logic here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I tried that originally and could not get it to work. I had something like:

#[derive(Debug, Deserialize)]
#[serde(transparent)]
pub struct EnvConfig {
    vars: HashMap<String, EnvConfigValue>
}

#[derive(Debug, Deserialize)]
#[serde(untagged)]
pub enum EnvConfigValue {
    Simple(String),
    WithOptions{
        value: Value<String>,
        force: bool,
        relative: bool, 
    }
}

The problem was serde would never parse the Value<T> type in the WithOptions variant, it worked if it was a regular String. I'm not sure why, maybe the entire "path" from the root of the deserialization needs to be Value<T> types in order for them to be handle correctly in some inner struct?

Copy link
Member

Choose a reason for hiding this comment

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

I think this structure may work instead?

#[derive(Debug, Deserialize)]
#[serde(untagged)]
pub enum EnvConfigValue {
    Simple(String),
    WithOptions(WithOptions),
}

#[derive(Debug, Deserialize)]
pub struct WithOptions {
        value: Value<String>,
        force: bool,
        relative: bool, 
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that doesn't work either. It seems like untagged and the Value<T> types do not play nicely together, it will never select a variant with that type. Latest commit does contain a struct/enum layout that does work though. So no more custom logic.

@wickerwaka
Copy link
Contributor Author

Thanks for the PR! Since it's been a few months since I've at least thought about this could you detail your expected use case with this as well? I'm not sure if this should close #4121 since that seems like a different feature request for Cargo

My specific use case is that the rust-cpython and pyo3 crates both look for a PYTHON_SYS_EXECUTABLE environment variable to find the python installation it should link against. I work in a large monorepo that contains rust code that is using rust-cpython, a full win64 python installation and a bunch of other stuff. Whenever possible we avoid using environment variables and prefer to use relative paths to find various components. This means the monorepo is easily relocatable to any location on a developers machine and multiple different versions can exist without having to modify/set any environment variables or other system-wide settings. I want to be able to set PYTHON_SYS_EXECUTABLE for a config.toml at or near the root of the monorepo:

[env]
PYTHON_SYS_EXECUTABLE = { value = "tools/lang/python3.7/python.exe", relative = true }

It seems to be a common thread that comes up during any search for "environment variables in cargo", crates people are using need to find libraries or tools (such as openssl, ffmpeg, python) and the crates use environment variables for that. If you have those dependencies at a known relative location you prefer not to have to rely on global environment variables for that. You can set them locally before you run cargo build but then you have all these issues with tools like IDEs that might not be running within that context. @rib calls out several issues related to this from other projects here: #4121 (comment)

You are right, I don't think it solves every aspect of #4121 because there are two different use cases in that issue and the comments. What some would like is a "prebuild" script that runs before dependencies are built that would allow you to set environment variables (and maybe some other settings) that the dependencies would consume. I would love to have a prebuild script myself, but I know there is some reticence about going down that road.

@bgrieder
Copy link

I confirm we have exactly the same use case.

@alexcrichton
Copy link
Member

Thanks for the explanation! That all sounds quite reasonable to me and I'd be happy to r+ this especially given that it's unstable. I think we'll want more team sign-off once a motion is made to stabilize, but for now seems good!

`untagged` enum variants are not recognised by serde if the variant contains a `Value<T>` type.
This change uses an transparent "inner" enum `Value<T>` member, which is handled correctly by serde.
@alexcrichton
Copy link
Member

@bors: r+

Looks great to me, thanks!

@bors
Copy link
Contributor

bors commented Feb 23, 2021

📌 Commit 05acf73 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2021
@bors
Copy link
Contributor

bors commented Feb 23, 2021

⌛ Testing commit 05acf73 with merge 4742e82...

@bors
Copy link
Contributor

bors commented Feb 23, 2021

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 4742e82 to master...

@Flaise
Copy link

Flaise commented Feb 23, 2021

This is going to be very helpful for a number of my projects. Thanks guys.

@rib
Copy link

rib commented Feb 23, 2021

Big thanks to @wickerwaka for implementing this! I guess #8839 can also be closed based on this?

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 27, 2021
Update cargo

11 commits in bf5a5d5e5d3ae842a63bfce6d070dfd438cf6070..572e201536dc2e4920346e28037b63c0f4d88b3c
2021-02-18 15:49:14 +0000 to 2021-02-24 16:51:20 +0000
- Pass the error message format to rustdoc (rust-lang/cargo#9128)
- Fix test target_in_environment_contains_lower_case (rust-lang/cargo#9203)
- Fix hang on broken stderr. (rust-lang/cargo#9201)
- Make it more clear which module is being tested when running cargo test (rust-lang/cargo#9195)
- Updates to edition handling. (rust-lang/cargo#9184)
- Add --cfg and --rustc-cfg flags to output compiler configuration (rust-lang/cargo#9002)
- Run rustdoc doctests relative to the workspace (rust-lang/cargo#9105)
- Add support for [env] section in .cargo/config.toml (rust-lang/cargo#9175)
- Add schema field and `features2` to the index. (rust-lang/cargo#9161)
- Document the default location where cargo install emitting build artifacts (rust-lang/cargo#9189)
- Do not exit prematurely if anything failed installing. (rust-lang/cargo#9185)
@mandeep
Copy link

mandeep commented Mar 7, 2021

Perhaps there should be a test that shows what happens if a dependency requires an environment variable. I tried testing this change in that manner with an environment variable that points to the path of a shared library. cargo build worked just fine however cargo run ended with an error with the environment variable not being found. I'm not sure if this is the point of the change, but curious to hear some thoughts.

@wickerwaka
Copy link
Contributor Author

Perhaps there should be a test that shows what happens if a dependency requires an environment variable. I tried testing this change in that manner with an environment variable that points to the path of a shared library. cargo build worked just fine however cargo run ended with an error with the environment variable not being found. I'm not sure if this is the point of the change, but curious to hear some thoughts.

Could you provide some more details on this? Environment variables are getting set for both build and run and my own personal use case is handling a situation where a dependency needs an environment variable set. However, I am not using that environment variable during the run step. Do you have a minimal example you can share?

@DieracDelta
Copy link

DieracDelta commented Apr 9, 2021

Is there a way to obtain the same functionality of this Cargo.toml env section from a build.rs script? I am passing in environment variables that are only known at runtime. E.g. I want to build package A that depends on package B. I build package A like ENV_VAR_EXAMPLE="hello world" cargo build. The build.rs script for package A will pick up on this. But I want package B to see ENV_VAR_EXAMPLE as well. The use case here is passing in a library location that is generated during a build.

@mandeep
Copy link

mandeep commented Apr 9, 2021

@wickerwaka Sorry for the late reply. Here's an example I'm using with https://github.com/Twinklebear/oidn-rs:

// config.toml
OIDN_DIR = "path/to/lib/" // contains shared_obj_file.so
LD_LIBRARY_PATH = "path/to/lib/"

cargo build executes just fine, however cargo run will see an error about not finding the shared object file. I'm assuming this is because at runtime these environment variables are not used.

@roblabla
Copy link
Contributor

roblabla commented Jun 3, 2021

Shouldn't this have a tracking issue for the -Z configurable-env gate?

@ehuss
Copy link
Contributor

ehuss commented Jun 3, 2021

Sure, posted #9539 as a tracking issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Passing environment variables from down-stream to up-stream library