cargo: Pass env vars in the env when --env-set is not available#15837
cargo: Pass env vars in the env when --env-set is not available#15837nirbheek wants to merge 1 commit into
Conversation
This means using the documented method to set env vars[1] when invoking a process on POSIX, and using a tiny Rust wrapper on Windows, where `cmd /c` is used to spawn processes. This Rust wrapper is built during setup (only on Windows with a non-nightly Rust) when a cargo workspace is used. Of course, this means a build-machine Rust toolchain is required in such cases, but that's already a requirement in Cargo due to build.rs 1. https://ninja-build.org/manual.html#ref_rule_command
bonzini
left a comment
There was a problem hiding this comment.
The main change I noted is the use of DataFile, apart from that it doesn't need to be so tightly tied to cargo.
| element.add_dep(deps) | ||
| element.add_item('ARGS', args) | ||
| element.add_item('targetdep', depfile) | ||
| if target.rust_compile_env: |
There was a problem hiding this comment.
Does it need to have "rust" in the field and variable names?
| def ensure_meson_env_exe(environment: Environment) -> None: | ||
| """ | ||
| Build the meson_env wrapper on windows when using stable Rust that lacks | ||
| --env-set, since that's the only way to set a process's environment in |
There was a problem hiding this comment.
We probably want to do that unconditionally and get rid of --env-set, because it does not work in procedural macros.
There was a problem hiding this comment.
IMO no we don't want to do that. We want to use Rust's "Official Way To Do The Thing(tm)", both because it avoids slowdown due to interposition programs (even a compiled binary, faster than a python script, is still going to be slower to an extent on Windows), and because if our every reaction to rust upstream adding an option that doesn't work exactly the way we want it to, is to stop using it and write a wrapper program, then we lose all our ability to interact with them and claim that we would be helped by it. It will certainly never be stabilized if it has no users.
As for it not working in procedural macros... this is the official rust option, if it doesn't work in all use cases then that's a bug in rust? What's their stance on this? It at minimum deserves to be mentioned, because if there's one thing worse than being a career NIH project it's not even bothering to document or link to the reasons behind it.
There was a problem hiding this comment.
It's not the "Official Way" it's the "Unstable Experimental Way" that they have no plan to stabilize.
They also have a Tracking Issue for proc_macro::{tracked_env, tracked_path}, and the tracked_env API would work well with --env-set. But they also have no plan to stabilize that, and it would take years before it propagates to the procedural macro crates.
So, I don't think there's any hope. --env-set and tracked_env are failed experiment by all intents and purposes.
There was a problem hiding this comment.
If we are "doomed to carry a wrapper program forever" then I don't see the validity of making this unconditional for the relatively small subset of projects that need it.
I don't actually like the fact that we're unconditionally using --env-set for loads of junk that makes for bad command line logging in ninja with no upsides for most crates.
If you squint then it's "okay" because it's just more command line arguments to a program we're already exec'ing, plus an overton window shift.
Suddenly that becomes a bigger problem if we're doing it for everything, with neither an opt-in nor an opt-out, and no plans to ever remove it.
This is unsustainable, and it's punishing well behaved users for the sake of badly behaved ones. If there is no exit ramp for needing a wrapper program then I really really think this needs to be a kwarg specifying whether those environment variables need to be defined in the first place. (Or a Cargo.toml setting that we can propose perhaps on the rationale of "defining clear scopes and isolating side effects".)
There was a problem hiding this comment.
I am sympathetic to the idea that the cmdline is getting super polluted. It's also super-redundant to put these in the build.ninja file repeatedly, so I'll move the env vars to a crate-specific file, and have meson_env.rs parse that file for env vars.
Note that this is not really an optional feature. Every crate with a workspace expects to have these available. Any crates that build without env vars are building purely by accident and can break at any point in the future.
There was a problem hiding this comment.
Note that this is not really an optional feature. Every crate with a workspace expects to have these available. Any crates that build without env vars are building purely by accident and can break at any point in the future.
I find this phrasing troubling.
We already knew long ago that cargo sets these environment variables for every crate. This PR was never about setting environment variables that crates might set, it was and is about taking existing code that currently runs only with nightly and making it run all the time (and that code happens to be "set a bunch of environment variables"). We don't need to relitigate whether or not cargo does this thing.
My point was that not all crates use this automatic feature that cargo automatically does, and I don't think you can justify claiming that it is "purely by accident" that source code which doesn't attempt to utilize the environment variables, can build when the environment variables aren't set.
It's just decently written code that doesn't rely on bizarroland magic like the full path to the cargo executable (which we don't set lol).
The idea that it should be possible to explicitly specify that these environment variables are needed, shouldn't be seen as crazy. It's a separate topic from what the default should be. My gut feeling is that most crate libraries (not executable crates) that don't have a build.rs also largely don't have a need for any of this, and name/version/out_dir cover nearly all practical uses anyway.
There was a problem hiding this comment.
I mostly agree. Only the crate major/minor/patch environment variables are relatively common in binaries. but not that much in libraries.
OUT_DIR is by far the most common (perhaps the only one I would include by default if it came down to making env vars opt in).
| # On Windows we need a tiny native wrapper to set the env before | ||
| # running rustc | ||
| from .. import cargo as _cargo | ||
| _cargo.interpreter.ensure_meson_env_exe(state.environment) |
There was a problem hiding this comment.
Likewise I would move the meson_env_exe code out of cargo, even if it is only used there. If can be in ninjabackend.
There was a problem hiding this comment.
Would the intention be to use it in the future for other things that aren't cargo? Because if so, the fact that the source is written in rust is a major blocker.
There was a problem hiding this comment.
There could always be a slow fallback to Python/ExecutableSerialisation. But more in general, neither "work around ninja limitations on Windows" nor "execute this program as fast as possible" are Cargo-related.
There was a problem hiding this comment.
I wanted to do that as a second step because it's not clear if we have other users of this functionality, and I didn't want to plug in the meson_env.c and meson_env.py fallbacks already as dead code.
There was a problem hiding this comment.
I think ou can make it use Rust as long as the only user in tree is cargo. Whoever wants to adopt it will have to deal with adding the fallback, but they'll find the code already in the right directory.
There was a problem hiding this comment.
I wanted to do that as a second step because it's not clear if we have other users of this functionality, and I didn't want to plug in the meson_env.c and meson_env.py fallbacks already as dead code.
Well, we could in theory go whole hog and make meson_env.c / meson_env.rs be suitable for generic custom_target env: usage.
There was a problem hiding this comment.
I was going to suggest that this would be a performance improvement on Windows, so having the option to use env on systems with env or fallback to meson_env.c/meson_env.cpp could yield benefits there. Of course, that also means that we would have 2-3 paths because we'd still need the python version to support instances where there isn't a build machine toolchain available.
| if build_rust is None: | ||
| raise MesonException('Cargo subproject on Windows requires a build-machine Rust toolchain') | ||
|
|
||
| here = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) |
There was a problem hiding this comment.
Use mesondata.DataFile instead.
There was a problem hiding this comment.
Correct. We need mesondata because "parent directory of __file__" is meaningless and therefore broken when using, well, the windows MSI most popularly used by the target userbase of a wrapper program. :)
It's the same issue as also seen in zipapps, so this should be testable on Linux too. Although the CI only does very basic zipapp smoke tests.
The issue is that __file__ tries to hand out a disk filename for something packed into an archive, which doesn't exist, and we need importlib.resources to create a real disk file for it. IIRC the PyInstaller backed MSI does have a disk file for non python files but doesn't have one for the thing that __file__ points at, but I could be misremembering, and either way it's not guaranteed to not be cleaned up as a "tempfile" when meson.exe exits, so yeah, that's fun.
| out = os.path.join(environment.get_scratch_dir(), 'meson_env.exe') | ||
| if not os.path.exists(out) or os.path.getmtime(out) < os.path.getmtime(source): | ||
| mlog.log('Building meson_env wrapper for Cargo subprojects...') | ||
| cmd = build_rust.get_exelist() + ['-O', '-o', out, source] |
There was a problem hiding this comment.
Why do we build this during setup instead of during ninja? The executable could just be a dependency the same way any executable used in the command: of a custom target is a dependency.
| from ..mesonlib import is_windows | ||
| if not is_windows(): |
There was a problem hiding this comment.
if not environment.machines.build.is_windows()?
There was a problem hiding this comment.
Mesonlib is correct here because the question is whether ninja is a unix ninja and generated build rules get executed via unix shell, or whether ninja is a windows ninja.exe and generated build rules get executed via CreateProcess.
While it's technically true that the build machine compiler is the same platform as the OS layer, the usual argument to follow environment.machines is to avoid hardcoding cross-unsafe assumptions especially when something should respect cross but doesn't. So this is either simpler or "doesn't matter" depending on perspective.
Does it really matter to you in this case?
There was a problem hiding this comment.
I would really love to see the mesonlib.is_* functions die because they get used in the wrong place all the time.
But in this case, we're importing a module inside a function to avoid using the object that has the exact same information and is already passed into the function. I feel like this is a clear improvement.
There was a problem hiding this comment.
I was going to separately point out that we already do from-imports of mesonlib at the top of the file and there's no earthly reason to do so as a function-local import either way.
But if you want it switched to an environment lookup it would be a moot point...
This means using the documented method to set env vars[1] when invoking a process on POSIX, and using a tiny Rust wrapper on Windows, where
cmd /cis used to spawn processes.This Rust wrapper is built during setup (only on Windows with a non-nightly Rust) when a cargo workspace is used.
Of course, this means a build-machine Rust toolchain is required in such cases, but that's already a requirement in Cargo due to build.rs