Read project environment vars from config

[rib]

I've recently been struggling with being able to use Visual Studio Code with rust-analyzer while I'm trying to use a dependency (ffmpeg-sys) that needs to be configured via an FFMPEG_DIR environment variable for it to build.

Looking around I found a bunch of different, related issues where people have basically been struggling with this same issue:

• Passing environment variables from down-stream to up-stream library #4121 <- notably OP here was facing difficulty with the exact same ffmpeg-sys package as me
• a way to give RLS environment settings rls#915
• Error squiggles for Cargo.toml when build.rs requires environment variable vscode-rust#791
• Add rust-analyzer.serverEnv to set the environment variables of the server rust-analyzer#6099
• Custom environment variables for running cargo intellij-rust/intellij-rust#1569 <- notably maintainers asserted this would best be solved lower in the stack - presumably in cargo.

Since I saw that #4121 had been open for over three years I figured there's little point waiting for progress with that and I instead looked at modifying ffmpeg-sys so I could remove its dependence on an environment variable for configuration. My original plan was to implement something very similar to this pull request except that I found cargo didn't expose enough information to build scripts for them to locate the workspace root which meant I could only find a config that would be adjacent to the Cargo.toml for ffmpeg-sys. This wasn't very practical since it meant I'd have to fork ffmpeg-sys in order to configure it.

This is the PR for reference: zmwangx/rust-ffmpeg-sys#7
Notably the maintainer was clearly very much against the idea too, of the oppinion that it should either be solved within the tooling integrations or lower in the stack in cargo.

This PR brings the same idea directly into Cargo, with the advantage that it knows where the workspace root is and it can also solve the same problem for all crates that depend on environment variables for configuration where users need to be able ensure a consistent environment between command line builds and IDE or editor-driven builds.

In summary this introduces support for reading an (optional) environment.json found at the root of the current workspace that may contain a map of environment variable key, value pairs. These variables will be exported to all build scripts run under the workspace. The removes any need to configure tools and editors independently.

The configuration is separate from any Config.toml since it's likely that the state shouldn't be under version control in many situations (generally locating resources for the project within a specific user's development environment).

A few open questions

1. Would it be worth having more structure, such as being able to have a 'default' section and also override sections for specific packages? I think keeping things simple might be best to start with since there's nothing ruling out the possibility of extending it later if that looks like it would be useful.
2. I guess it would be better if the environment config were read once when constructing the Workspace instead of repeatedly before invoking each build script.
3. I guess somehow the environment config should be added as a build dependency so any changes to it would trigger a rebuild

In principle does this seem like a reasonable way of addressing issues like #4121?

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (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.

[rib]

A few other things I'd probably change with an update of this:

1. Any environment variables that have actually been set should take precedence - basically, the user knows best.

2. I can see that it would be helpful if environment variables could be overridden according to the target being built for. In my case where I'm using ffmpeg-sys then I may need to configure a VCPKG_ROOT variable for Windows builds or may need to configure FFMPEG_DIR differently between windows builds and when cross-compiling for Android. Notably though it can be backwards compatible to add something like this.

Can imagine a structure like this working:

{
    // defaults...
    "FOO": "bar"
    "FOO2": "baz"
    // target overrides...
    "aarch64-linux-android": {
        "FOO": "cross_bar"
    }
}

3. A format like JSON with comments, or toml might be better than pure JSON

[joshtriplett]

We talked about this in the Cargo meeting today. Summary of the consensus from that meeting:

• We'd like this to come from .cargo/config.toml, not from a separate configuration file. That will allow it to come from user configuration, or from a crate's configuration (since you can check in .cargo/config.toml), and there's already an established hierarchy of overrides.
• This can then use a TOML table:

[env]
FFMPEG_DIR = "/some/path"

• By default, this should not override an existing environment variable; it should only set an environment variable if not set. If necessary, we can support var = { value = "value", force = true }, but only if necessary.
• We talked about whether this should apply to the binary run with cargo run, and we felt like perhaps it should. (Perhaps there could be a scope = ["run", "build", ...] key, if there's a need to distinguish, but hopefully that's not needed for the common case so perhaps we can do without it.)
• This should start out unstable, and get stabilized later.

One more thought from that meeting: This is, effectively, adding a channel for a crate to communicate down to its dependencies. Previously, the only such channel we've had was feature flags. In an ideal world, we'd have a more general structured channel for that. In particular, we've talked about cargo "capabilities", which would be a key-value way to specify parameters for dependencies; that would work for things like "use this backend", for which things depending on a crate directly or indirectly could set the value, with capability higher in the dependency tree (ultimately the final binary crate) overriding those set lower in the dependency tree. Such a mechanism would allow ffmpeg-sys to have a structured and namespaced way to pass in a path, rather than using global unstructured unnamespaced environment variables.

So, long-term, we'd like to see a cargo key-value capability mechanism, which would allow Cargo.toml files to specify values for capabilities. But in the interests of providing a short-term solution, we'd also be open to allowing .cargo/config.toml to set environment variables. Our primary concern is that we don't want that used as the primary general-purpose channel for passing arbitrary information down to dependencies; that's why we want it in .cargo/config.toml where it can only be used in top-level crates and not at arbitrary points in the dependency tree.

[rib]

Right, it looks like these .cargo/config.toml files could be a good place for this.

It's an asside really but looking at the docs for these config files it makes me wonder whether there could potentially be a /project/.cargo/user/config.toml read after /project/.cargo/config.toml with a higher precedence within the documented heirachy and would never be expected to be version controlled. (I think these env vars will almost always be user and project specific) This sounds like a seperate issue though, and dealing with a conflict between version-controlled state and user state might be a little annoying in those cases but it's better than nothing.

Was a bit surprised to realise that cargo will apparently keep searching up to the root of the filesystem for config.toml overrides! I guess that's safe most of the time - seems odd to be searching well outside the bounds of a project though. Anyway totally unrelated.

Although I was initially focused on build script overrides for my use case, I forgot to mention that I'd been wondering if actually it could make sense for it to be more global, so the env state would be re-exported much earlier and then be visible across other commands like run too. Maybe that would be a better default - more consistent with the non-discriminatory nature of environment variables generally - and command scoping could be added in the future if needed - as suggested. I suppose this brings up the question whether the environment state should also be re-evaluated on a per-crate basis or maybe just once with respect to the top-level project/workspace?

Assuming re-evaluation on a per-crate basis for now and thinking about being carefull to not trample environment variables set by the user (I agree that if the user had exported the environment variable themselves cargo shouldn't touch them) I suppose there will need to be a central record of which env vars have only been set by cargo so it always remains possible differentiate user-set and cargo-set environment variables. Guess that sounds fine - just a little bit more fiddly.

Just thinking about the ability to have different overrides for different targets, such as when cross-compiling for Android in my case. I guess the way that might be able to work here is that [env] would contain defaults and a table like `[env.aarch64-linux-android]' could be used to override for a specific toolchain?

About the downsteam communication - this doesn't necessarily need to open up that possibility - it seems like it really comes down to the question of whether this environment state should be re-evaluated on a per-crate basis or just once for the top-level project. Perhaps environment state should be kept more global (which could be more in keeping with how environment variables are generally not a precision tool for configuring things). It could be somewhat simpler to implement and conceptually reasonable that this state would be evaluated very early on within cargo only with respect to the top level of your project/workspace and with that constraint I think it avoids introducing general downstream communication (which can be left to "capabilities" or something similar later)? Certainly if there was a feature like capabilities in the future then it would be good if projects like ffmpeg-sys could migrate to using that instead of environment variables.

Oh, haha, sorry, I clearly hadn't digested your last paragraph :)

that's why we want it in .cargo/config.toml where it can only be used in top-level crates and not at arbitrary points in the dependency tree

Right, so this pretty much answers the question I had about what you were imagining - this state would only be evaluated once.

Thanks for the follow up!

[joshtriplett]

@rib Are you up for updating the PR to get the environment variables from the Cargo config? I recently found a use case for this myself (setting CFLAGS for crates building C libraries), and I'd be interested.

[rib]

Hey, it's been in the back of my mind to follow up at some point on this, but just been really focused on seasonal startup work recently. I'm happy either way if you go ahead and put something together or otherwise I guess some time in the new year I may get a chance to look at following up on this.

[joshtriplett]

@rib Not a problem, and no rush. Just wanted to see if you were still interested.

[matklad]

From an IDE point of view, I'd say that "global" env-vars should be configured by the shell (and IDE should be started from the shell) while project-specific config should either be handled by https://direnv.net (IDEs integrate with that somewhat) or by build-in IDE config.

I personally feel that "convenient way to set env vars" is a feature creep for Cargo, as that's clearly the shell's turf. OTOH, using shell on windows is a pain, so we might as well support this.

[ehuss]

Closing as implemented by #9175.