Check for FFMPEG_DIR in environment.json

[rib]

While I've been looking to use this project I found it surprisingly difficult to find any practical way of configuring Visual Studio
Code so that that FFMPEG_DIR could be set in the environment when it runs cargo as part of its rust-analyzer/RLE integration
for editor diagnostics.

Looking into this further I found others had been struggling for a long time with similar issues:

rust-lang/cargo#97
rust-lang/cargo#4121
rust-lang/rls#915
rust-lang/vscode-rust#791
https://stackoverflow.com/questions/57017066/how-do-i-set-environment-variables-with-cargo

Notably rust-lang/cargo#4121 was filed back in 2017 for essentially this same problem but in all that time there's been no traction in finding a good solution for this.

Considering this impracticality, this adds support for an alternative environment.json file that can be created like:

{
    "FFMPEG_DIR": "/path/to/ffmpeg-build/"
}

The change is backwards compatible, since the file isn't required and it will also still check for the environment variable

Addresses: rust-lang/cargo#4121

[rib]

hmmm, I've just realised an issue with this which is that CARGO_MANIFEST_DIR is always the directory for this project whereas I had been thinking it was the directory of the top level manifest which would be more appropiate for this use case I think. So this works ok for testing building this directly but not for other projects that directly or indirectly depend on ffmpeg-sys.

Maybe it can be changed to just refer to the current working directory when looking for the environment.json

[zmwangx]

You linked to quite a few long discussion threads and I didn't have time to check them all. Anyway, adding support for a nonstandard, rather random configuration file is definitely not the way to go.

I just checked, setting the env var before launching VSCode seems to work totally fine for me:

$ $env:FFMPEG_DIR="C:\path\to\ffmpeg"
$ code .

(rust-analyzer then picks up FFMPEG_DIR and builds without problem.)

I guess this way you'll need to relaunch Code every time you want to change FFMPEG_DIR, but doesn't sound like a huge pain to me?

I also recommend following a solution like rust-lang/rust-analyzer#6099 which solves the problem of setting env vars in rust-analyzer. I'd say tooling inconveniences should be solved in tooling, not by polluting code that tooling's supposed to support. Every project coming up with their own ad hoc hacky solution is untenable.

I'll leave this open for a while in case I'm misunderstanding the issue somehow, and you can enlighten me.

[zmwangx]

(I assumed you're on Windows since I never needed FFMPEG_DIR on *nixes -- which I now realize might not be the case -- so the test above was done in PowerShell, but the equivalent should definitely work on macOS/Linux.)

[rib]

Thanks for taking a look and also digging up another relevant issue.

Although I'm using Windows currently, the same would apply for my use case on Linux too since I'm also using FFMPEG_DIR to locate cross-compiled Android binaries.

I appreciate the trade offs of solving this in tools vs here but the reality is that this hasn't been solved elegantly yet in tools and this is something that's been causing people trouble for a long time. I just implemented this to be pragmatic and figured I'd share it in case it helps others.

I'm not sure it's that constructiving to call it "hacky" since I think a reasonable argument could be made that a more-dedicated config file is actually less hacky by being structured, clearly visible and closely coupled with the thing being configured. Environment variables are often an unrelaible and inflexible mechanism for configuring software and highly discouraged within many different build systems due to their lack of specificity and how easy it is to lose track of or accidentally change what environment you're running in. There aren't that many crates that require similar environment variables for configuration and if you look around you can find that actually lots of people are having trouble with dealing with them and they probably aren't a good long term approach for crates to use. I'd be pretty happy if cargo even adopted a similar config-file approach for solving rust-lang/cargo#4121

Considering for a moment "Every project coming up with their own ad hoc hacky solution is untenable."... and putting asside whether we judge this as "hacky" I think it's acually a good think to try and solve this once in connection with the crate where it's relevant and not need to configure N different tools and workflows that might also spawn cargo and require a specific environment. So another way of looking at it is that "Every project" currently needs "their own ad hoc" solution for a problem that stems from one place and we can just change this project so we don't have the problem. Similarly for other the small hand full of other crates that expect environment variables to be configured, they could be changed to use appropriate config files instead and then everyone wouldn't have to figure out how to configure all their various developer tools to control the environment (in the current case you're very likely going to have to duplicate the same configuration state to account for how different tools work and it'll be easy to lose track of the configuration too if it's buried in various editor-specific config files).

So considering the assertion that "tooling inconveniences should be solved in tooling" - although that sounds fine at first, the point here is that this isn't fundamentally a "tooling inconvenience" - the problem here "how can I configure ffmpeg-sys reliably" and that really shouldn't involve touching tooling configs at all, it only involves tooling due to the current choice to use environment variables to configure ffmpeg-sys and if we can avoid that then it's no longer a tooling issue at all.

I could spawn vscode within a shell with the environment I want certainly, and I could also just set the environment variable to be available across all shells/applications but in my view these are only workarounds and not convenient. For example, most of the time I'm using VSCode in conjunction with Unity3D and Unity is configured to spawn VSCode so then I'm chasing down a rabbit hole having to configure the environment of another tool.

I think the main things about the proposals I'm not entirely satisfied with are:

1. I would have rather been able to place an environment.json config file at the top of my project instead of having to place it next to the Cargo.toml for ffmeg-sys (since I'm now basically forced to point my project at a custom fork)
2. It would be nice if the exact same thing was supported generally by cargo instead. In the mean time I think it's still preferable to configure one project via a config file than it is to configure the environment for N different tools in N different places.

Hope that clarifies my pov a bit. I undertand it's not perfect, but at least for me I think this is better than dealing with environment variables and I'm happy to have a solution that works now instead of waiting another three years for a more general solution in cargo.

[rib]

Here's a relevent example of push back at the idea of solving this kind of problem at the IDE/tooling level:

intellij-rust/intellij-rust#1569

[rib]

Just for reference, I've also made a proposal that Cargo could perhaps do something similar:

rust-lang/cargo#4121 (comment)

(but as mentioned before I'd rather not hold my breath for a solution in Cargo since people have already been waiting for over three years for help with solving this there)

[zmwangx]

So I understood the issue just fine, we just disagree on what's an acceptable solution. If we were to hash this out in person / in chat I imagine the conversation would go down like this:

A: Using environment variable for build configuration is no good.
B: Why? It's a cross-platform convention, and it's basically the only option that works.
A: But I can't use env var with rust-analyzer (or some other tool).
B: Why not? Just start VSCode (or some other encompassing tool) with the env var.
A: I don't want to restart VSCode to add/change an env var.
B: Okay, then it's something to be solved in VSCode/rust-analyzer/whatever, here's someone else's patch to solve it.
A: It's not merged. / What about some other tool that I'll possibly use?
B: Okay... Back to "it's the basically the only option that works". Solve it in cargo instead.
A: It doesn't have enough traction. How about an ad hoc solution right here.
B: Okay... What's that?
A: An ad hoc config file that has to live in tree to work. I know, it doesn't work when the lib is used as a dependency (99.9% of use cases) unless the dependency is patched.
B: ...

I guess I'll give it another consideration if and when there's an ad-hoc working solution that does not require unpacking this crate into filesystem a working tree?

Realistically though, I'll probably reject the ad-hoc working solution as well. A non-standard config file is just too arbitrary. environment.json you say? Never heard of it, and JSON is a shitty config format (it really is). What about JSONC (JSON with comments, and equally importantly, trailing commas)? YAML? TOML? .env? I'm usually not one to settle on something random, but at the same time, I'm not willing to have a debate about config formats when it's a debate that should happen elsewhere (cargo).

Parting thought:

Environment variables are often an unrelaible and inflexible mechanism for configuring software... There aren't that many crates that require similar environment variables for configuration

Of course you hardly ever need env vars for configuring pure Rust crates. But when it comes to system/C interfacing, using env vars is pretty common. bindgen, clang-sys, cc, etc. these cornerstone crates all use env vars. Probably need to convince them first before asking me to depart from tradition.

[rib]

Honestly this conclusion sounds kinda fine to me - though I don't agree exactly with how the conversation would go :) For one there's no question of being cross platform here - config files are as cross platform as environment variables, I suppose more so since environment variables are literally down to the OS to support, and I guess there's a larger volume of stuff configured via config files than via environment variables in general if that was even in question.

I think your stance feels somewhat idealistic which is perhaps a little frustrating (and I think that's kinda why in >3years of people trying to find a solution here everyone is just passing the buck and saying it should be solved elsewhere), but the solution posted here also isn't what I had originally hoped for either and it's not as practical as I would have hoped from being able to discover a top level config file instead.

The choice of json vs toml vs your favourite format is pretty much the last thing anyone wants to bikeshed - changing that according to whoever has the loudest opinion is easy :)

I generally agree it would be better to have a solution from Cargo.

Since I anyway basically need to keep a fork of ffmpeg-sys with the current limitaiton of this approach then I'm pretty much fine with doing that and it doesn't really affect me whether this is accepted - as mentioned I figured I'd share this because there are clearly others also facing the same issue.

You're right this is more likely to relate to -sys projects, but the two projects I've seen mentioned most when looking at this are ffmpeg-sys and openssl-sys. For some other cases like configuring toolchains there are sometimes config file alternatives already.

[zmwangx]

Closing as all the technical arguments have been laid out, the rest is opinion.

Summary:

• I'm not convinced there are insurmountable difficulties with env vars, only inconveniences;
• I'm wary with ad hoc solutions to what should be handled by the build system, in this case cargo (is it actually a problem that needs to be solved? Maybe not, see previous point).
• This specific PR requires adding a file under this crate's directory, which basically requires using this crate as a patched dependency. Therefore it's only slightly cleaner than patching build.rs to hardcode FFMPEG_DIR. The solution is less convenient (and I'd wager rather confusing for users) than the tooling that was perceived as inconvenient in the first place, so not acceptable even as an ad hoc solution.