-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
cargo: Pass env vars in the env when --env-set is not available #15837
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| import os | ||
| import pathlib | ||
| import collections | ||
| import subprocess | ||
| import urllib.parse | ||
| import typing as T | ||
| from pathlib import PurePath | ||
|
|
@@ -47,6 +48,41 @@ def _dependency_name(package_name: str, api: str, suffix: str = '-rs') -> str: | |
| return f'{basename}-{api}{suffix}' | ||
|
|
||
|
|
||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably want to do that unconditionally and get rid of --env-set, because it does not work in procedural macros.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 So, I don't think there's any hope.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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".)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
| ninja. Requires a build-machine Rust toolchain. | ||
| """ | ||
| if environment.meson_env_exe is not None: | ||
| return | ||
| from ..mesonlib import is_windows | ||
| if not is_windows(): | ||
|
Comment on lines
+59
to
+60
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would really love to see the 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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... |
||
| return | ||
| host_rust = environment.coredata.compilers[MachineChoice.HOST].get('rust') | ||
| if host_rust is None: | ||
| return | ||
| if T.cast('RustCompiler', host_rust).enable_env_set_args() is not None: | ||
| # rustc supports --env-set; no wrapper needed. | ||
| return | ||
|
|
||
| build_rust = environment.coredata.compilers[MachineChoice.BUILD].get('rust') | ||
| 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__))) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. We need mesondata because "parent directory of 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 |
||
| source = os.path.join(here, 'scripts', 'meson_env.rs') | ||
| 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] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| try: | ||
| subprocess.run(cmd, check=True) | ||
| except subprocess.CalledProcessError as e: | ||
| raise MesonException(f'Failed to build meson_env wrapper: {e}') | ||
| environment.meson_env_exe = out | ||
|
|
||
|
|
||
| def _extra_args_varname() -> str: | ||
| return 'extra_args' | ||
|
|
||
|
|
@@ -167,7 +203,7 @@ def get_lint_args(self, rustc: RustCompiler) -> T.List[str]: | |
|
|
||
| return args | ||
|
|
||
| def get_env_args(self, rustc: RustCompiler, environment: Environment, subdir: str) -> T.List[str]: | ||
| def get_env_set_args(self, rustc: RustCompiler, environment: Environment, subdir: str) -> T.List[str]: | ||
| """Get environment variable arguments for rustc.""" | ||
| enable_env_set_args = rustc.enable_env_set_args() | ||
| if enable_env_set_args is None: | ||
|
|
@@ -179,6 +215,15 @@ def get_env_args(self, rustc: RustCompiler, environment: Environment, subdir: st | |
| env_args.extend(['--env-set', f'{k}={v}']) | ||
| return env_args | ||
|
|
||
| def get_rustc_env(self, environment: Environment, subdir: str, machine: MachineChoice) -> T.Dict[str, str]: | ||
| """Get environment variables as a dict for rustc.""" | ||
| if not environment.is_cross_build(): | ||
| machine = MachineChoice.HOST | ||
| rustc = T.cast('RustCompiler', environment.coredata.compilers[machine]['rust']) | ||
| if rustc.enable_env_set_args() is not None: | ||
| return {} | ||
| return self.get_env_dict(environment, subdir) | ||
|
|
||
| def get_rustc_args(self, environment: Environment, subdir: str, machine: MachineChoice) -> T.List[str]: | ||
| """Get rustc arguments for this package.""" | ||
| if not environment.is_cross_build(): | ||
|
|
@@ -191,7 +236,7 @@ def get_rustc_args(self, environment: Environment, subdir: str, machine: Machine | |
| args: T.List[str] = [] | ||
| args.extend(self.get_lint_args(rustc)) | ||
| args.extend(cfg.get_features_args()) | ||
| args.extend(self.get_env_args(rustc, environment, subdir)) | ||
| args.extend(self.get_env_set_args(rustc, environment, subdir)) | ||
| return args | ||
|
|
||
| def supported_abis(self) -> T.Set[RUST_ABI]: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -342,6 +342,22 @@ def merge_kw_args(self, state: ModuleState, kwargs: T.Union[RustPackageExecutabl | |
|
|
||
| kwargs['override_options'].setdefault('rust_std', self.package.manifest.package.edition) | ||
|
|
||
| def _apply_rustc_env(self, state: ModuleState, | ||
| native: MachineChoice, | ||
| result: T.Union[BothLibraries, BuildTarget]) -> None: | ||
| env = self.package.get_rustc_env(state.environment, state.subdir, native) | ||
| if not env: | ||
| return | ||
| # 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likewise I would move the meson_env_exe code out of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well, we could in theory go whole hog and make meson_env.c / meson_env.rs be suitable for generic custom_target
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was going to suggest that this would be a performance improvement on Windows, so having the option to use |
||
| if isinstance(result, BothLibraries): | ||
| result.shared.rust_compile_env = dict(env) | ||
| result.static.rust_compile_env = dict(env) | ||
| else: | ||
| result.rust_compile_env = dict(env) | ||
|
|
||
| def _library_method(self, state: ModuleState, args: T.Tuple[ | ||
| T.Optional[T.Union[str, StructuredSources]], | ||
| T.Optional[StructuredSources]], kwargs: RustPackageLibrary, | ||
|
|
@@ -366,22 +382,25 @@ def _library_method(self, state: ModuleState, args: T.Tuple[ | |
|
|
||
| lib_args: T.Tuple[str, SourcesVarargsType] = (tgt_name, [sources]) | ||
| self.merge_kw_args(state, kwargs) | ||
| native = kwargs['native'] | ||
|
|
||
| result: T.Union[BothLibraries, SharedLibrary, StaticLibrary, SharedModule] | ||
| if shared_mod: | ||
| return state._interpreter.build_target(state.current_node, lib_args, | ||
| T.cast('_kwargs.SharedModule', kwargs), | ||
| SharedModule) | ||
|
|
||
| if static and shared: | ||
| return state._interpreter.build_both_libraries(state.current_node, lib_args, kwargs) | ||
| result = state._interpreter.build_target(state.current_node, lib_args, | ||
| T.cast('_kwargs.SharedModule', kwargs), | ||
| SharedModule) | ||
| elif static and shared: | ||
| result = state._interpreter.build_both_libraries(state.current_node, lib_args, kwargs) | ||
| elif shared: | ||
| return state._interpreter.build_target(state.current_node, lib_args, | ||
| T.cast('_kwargs.SharedLibrary', kwargs), | ||
| SharedLibrary) | ||
| result = state._interpreter.build_target(state.current_node, lib_args, | ||
| T.cast('_kwargs.SharedLibrary', kwargs), | ||
| SharedLibrary) | ||
| else: | ||
| return state._interpreter.build_target(state.current_node, lib_args, | ||
| T.cast('_kwargs.StaticLibrary', kwargs), | ||
| StaticLibrary) | ||
| result = state._interpreter.build_target(state.current_node, lib_args, | ||
| T.cast('_kwargs.StaticLibrary', kwargs), | ||
| StaticLibrary) | ||
| self._apply_rustc_env(state, native, result) | ||
| return result | ||
|
|
||
| def _proc_macro_method(self, state: 'ModuleState', args: T.Tuple[ | ||
| T.Optional[T.Union[str, StructuredSources]], | ||
|
|
@@ -519,7 +538,10 @@ def executable_method(self, state: 'ModuleState', args: T.Tuple[ | |
|
|
||
| exe_args: T.Tuple[str, SourcesVarargsType] = (tgt_name, [sources]) | ||
| self.merge_kw_args(state, kwargs) | ||
| return state._interpreter.build_target(state.current_node, exe_args, kwargs, Executable) | ||
| native = kwargs['native'] | ||
| result = state._interpreter.build_target(state.current_node, exe_args, kwargs, Executable) | ||
| self._apply_rustc_env(state, native, result) | ||
| return result | ||
|
|
||
|
|
||
| class RustSubproject(RustCrate): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // Copyright The Meson development team | ||
|
|
||
| // Tiny helper used by Meson on Windows to set environment variables before | ||
| // executing a command, equivalent to `env(1)` on POSIX systems. | ||
| // | ||
| // Usage: meson_env KEY1=VAL1 [KEY2=VAL2 ...] COMMAND [ARG ...] | ||
| // | ||
| // All leading arguments that look like `K=V` (and where K is a valid env-var | ||
| // name) are consumed as assignments; the first non-assignment argument and | ||
| // everything after it becomes the command to execute. The child process' | ||
| // exit code is propagated. | ||
|
|
||
| use std::env; | ||
| use std::process::{exit, Command}; | ||
|
|
||
| fn is_assignment(arg: &str) -> Option<(&str, &str)> { | ||
| let (k, v) = arg.split_once('=')?; | ||
| if k.is_empty() { | ||
| return None; | ||
| } | ||
| let first = k.as_bytes()[0]; | ||
| if !(first.is_ascii_alphabetic() || first == b'_') { | ||
| return None; | ||
| } | ||
| if !k.bytes().all(|b| b.is_ascii_alphanumeric() || b == b'_') { | ||
| return None; | ||
| } | ||
| Some((k, v)) | ||
| } | ||
|
|
||
| fn main() { | ||
| let mut args = env::args().skip(1); | ||
| let mut cmd: Option<String> = None; | ||
| while let Some(arg) = args.next() { | ||
| match is_assignment(&arg) { | ||
| Some((k, v)) => env::set_var(k, v), | ||
| None => { | ||
| cmd = Some(arg); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| let cmd = match cmd { | ||
| Some(c) => c, | ||
| None => { | ||
| eprintln!("meson_env: no command given"); | ||
| exit(2); | ||
| } | ||
| }; | ||
| let rest: Vec<String> = args.collect(); | ||
| let status = match Command::new(&cmd).args(&rest).status() { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| eprintln!("meson_env: failed to execute {cmd}: {e}"); | ||
| exit(127); | ||
| } | ||
| }; | ||
| exit(status.code().unwrap_or(1)); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| int check_envs(void); | ||
|
|
||
| int main(int argc, char *argv[]) { | ||
| return check_envs(); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| project('cargo env vars', 'c') | ||
|
|
||
| envcheck_dep = dependency('envcheck-1') | ||
| exe = executable('app', 'main.c', dependencies: envcheck_dep) | ||
| test('cargo env vars', exe) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| [wrap-file] | ||
| method = cargo | ||
|
|
||
| [provide] | ||
| dependency_names = envcheck-1 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| [package] | ||
| name = "envcheck" | ||
| version = "1.2.3" | ||
| edition = "2021" | ||
|
|
||
| [lib] | ||
| crate-type = ["staticlib"] | ||
| path = "lib.rs" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| const PKG_NAME: &str = env!("CARGO_PKG_NAME"); | ||
| const PKG_VERSION: &str = env!("CARGO_PKG_VERSION"); | ||
| const PKG_VERSION_MAJOR: &str = env!("CARGO_PKG_VERSION_MAJOR"); | ||
| const PKG_VERSION_MINOR: &str = env!("CARGO_PKG_VERSION_MINOR"); | ||
| const PKG_VERSION_PATCH: &str = env!("CARGO_PKG_VERSION_PATCH"); | ||
| const CRATE_NAME: &str = env!("CARGO_CRATE_NAME"); | ||
|
|
||
| #[no_mangle] | ||
| pub extern "C" fn check_envs() -> i32 { | ||
| if PKG_NAME != "envcheck" { return 1; } | ||
| if PKG_VERSION != "1.2.3" { return 2; } | ||
| if PKG_VERSION_MAJOR != "1" { return 3; } | ||
| if PKG_VERSION_MINOR != "2" { return 4; } | ||
| if PKG_VERSION_PATCH != "3" { return 5; } | ||
| if CRATE_NAME != "envcheck" { return 6; } | ||
| 0 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it need to have "rust" in the field and variable names?