Add support for EnvironmentVariables in Rust targets

[dcbaker]

Idiomatic usage of Rust requires the use of environment variables at a compile time. This can be seen in the common idiom:

include!(concat!(env!("OUT_DIR"), "/generated.rs"))

Which is a very common thing to do. While I think the structured_sources proposal is more elegant and generally a better idea, both for the purpose of building cargo based projects with meson, and for the purpose of including incomplete snippets, we should support environment variables for Rust based targets.

This series adds that support by adding the env keyword to build_targets (usage in non-rust targets is considered an error), and generates two different rust rules, one for env wrapping and one for non wrapping.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7700618) 70.24% compared to head (041b2d5) 61.89%.
Report is 11 commits behind head on master.

❗ Current head 041b2d5 differs from pull request most recent head e7ce752. Consider uploading reports for the commit e7ce752 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10030      +/-   ##
==========================================
- Coverage   70.24%   61.89%   -8.36%     
==========================================
  Files         219      200      -19     
  Lines       48453    42558    -5895     
  Branches    11488     9292    -2196     
==========================================
- Hits        34037    26340    -7697     
- Misses      11988    14124    +2136     
+ Partials     2428     2094     -334     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tristan957]

This is also necessary for GResources in Rust. Thanks for adding it.

[eli-schwartz]

Idiomatic usage of Rust requires the use of environment variables at a compile time. This can be seen in the common idiom:

include!(concat!(env!("OUT_DIR"), "/generated.rs"))

This topic has come up before, and it might be "idiomatic" usage of Rust but it's not "idiomatic compiler design". Previous discussion on the topic I'm pretty sure was targeting getting a rustc fix for this.

It's basically terrible to do this for including generated files, and it's outright horrible to do this for any other reason -- what are people doing, using env variables to select target features? To define constants used for switching out runtime logic?

What's the use case for this other than describing the current_build_dir() and why do we need a generic solution to do frankly bad stuff? What's the solution to not requiring a "wrapped due to env" design for all sources involved in this target, rather than just the one source that cats out a generated file into the current file by absolute path underneath a pile of macros?

Given that structured_sources is such a better more elegant solution to this problem, why merge a worse solution at all?

w.r.t mention of crate subprojects, I'd consider that to be best solved by handling this inside crate conversion to meson rules, not something that requires an extension to the Meson DSL (which crate subprojects wouldn't use... right?)

[tristan957]

This is the documentation: https://doc.rust-lang.org/std/macro.env.html. @eli-schwartz, I think you are taking the example posted too literally. While including a generated file is a solved problem in Meson, the env!() macro is useful for other things too.

I don't think Meson should forbid someone from using a feature of their language effectively.

[eli-schwartz]

Including a generated file in rust is a solved problem using meson?

...

Thanks for the rust docs link, but it doesn't tell me anything I didn't know and acknowledge in my original comment. I was never in doubt that rust lets you do this, I simply challenge the notion that it's a good "compiler design" (i.e. rust was wrong to allow it) and that people should use it when it is allowed.

The original PR asserted that it is useful, but didn't say why. Now you assert that it is useful too, but also don't say why. I, meanwhile, am asking why this thing that seems to be a terrible idea is in fact not only good, but so good that we should extend the meson DSL in a way that no other compiler supports and which slows down compilation by wrapping rust compile rules in a python script for something that is, of necessity, hardcoded into meson.build and always known at buildsystem generation time.

So that source code files can process environment variables.

Environment variables. Which meson loves so much the lead developer of meson would prefer to ban them entirely.

I think it's not unreasonable of me to try to understand why this new feature is needed and what the practical use cases are.

I'm perfectly okay with meson "forbidding someone from using a feature of their language effectively" when that feature is exclusively of use to move build system logic into source code files because cargo isn't a real build system.

[tristan957]

I mean it really is just an alternative to configure_file(). It is just crappier because the Rust community decided that Cargo is an amazing build system, which it isn't. Because Cargo is so bad, Rust has env!(). That is pretty much all there is to it.

I thought this comment was interesting:

w.r.t mention of crate subprojects, I'd consider that to be best solved by handling this inside crate conversion to meson rules, not something that requires an extension to the Meson DSL (which crate subprojects wouldn't use... right?)

Maybe the problem could be solved by using configure_file() for a .env file, and then after meson setup someone could source the env file, and then hopefully rustc just picks up the environment variables that way?

[dcbaker]

w.r.t mention of crate subprojects, I'd consider that to be best solved by handling this inside crate conversion to meson rules, not something that requires an extension to the Meson DSL (which crate subprojects wouldn't use... right?)

What are you proposing? that I write a Rust parser in Meson and process every Rust file with a heuristic to determine when we could replace it with use or mod instead?

Maybe the problem could be solved by using configure_file() for a .env file, and then after meson setup someone could source the env file, and then hopefully rustc just picks up the environment variables that way?

That requires altering the source files, which isn't allowed in a wrap.

Among other things though, bindgen recommends this as the way to handle generated sources: https://rust-lang.github.io/rust-bindgen/tutorial-4.html

This may not be the way I'd design a compiler, but it is how Rust works, and refusing to make Rust work because it's not ideal isn't practical, we might as well just remove Rust support if we're going to refuse to implement common use cases because we don't find them pretty enough.

[eli-schwartz]

What are you proposing? that I write a Rust parser in Meson and process every Rust file with a heuristic to determine when we could replace it with use or mod instead?

No, I just want people writing meson.build for their own rust code to do it properly. :p

Do we actually need it for wraps? I thought the idea was to inspect a crate and convert cargo's output to build.ninja rules somehow. That doesn't require having people literally add an env kwarg to an actual meson.build for the crate.

[dcbaker]

No, but it likely requires the converted crate to add the keyword arguments because the rust files in the crate use include!(env!(...)) because that's idiomatically how you include generated sources in Cargo, and thus how you generate them in Rust.

Otherwise the wrap will have to patch the rust files, which we don't allow.

[tristan957]

Was not thinking of the wrap use case when I made my comment. Thanks for reminding me!

[eli-schwartz]

Again, I thought the idea for crates was not to just add meson.build patches and add a bajillion things to the wrapdb, but to actually introspect cargo's output and use it to generate build.ninja rules similar to how cmake.subproject works from a high-level perspective, no meson.build involved.

[dcbaker]

Again, I thought the idea for crates was not to just add meson.build patches and add a bajillion things to the wrapdb, but to actually introspect cargo's output and use it to generate build.ninja rules similar to how cmake.subproject works from a high-level perspective, no meson.build involved.

Those rust files may already be using env!(), which we need to support, otherwise we have to patch the .rs files. There's no other solutions than:

1. support a feature of cargo + rust, (ie, this PR) so that meson works seamlessly
2. patch Rust sources to remove any use of env!() (assuming that's always possible, since Rust doesn't have a pre-processor)

[xclaesse]

You should use type_checking._env_convertor()

[xclaesse]

0.63

[xclaesse]

I'm not so sure, that's going to split C:\foo on Windows. Also you should not assume user wants to append to current env, they could want to set() or prepend(). I think for that you should pickle an ExecutableSerialisation() that contains the original EnvironmentVariables() object, instead of adding --env in the command line.

[dcbaker]

At this point I'd like to wait on this until the build.py typing stuff lands, as it will be much easier to ensure everything is correct here once that happens.

[dcbaker]

This has been updated to use the nightly --set-env option, though the fallback to use a wrapper is currently not implemented. That will need to be reimplemented

[xclaesse]

@dcbaker Wondering if we should instead pretend Rust uses -DFOO=BAR like every sane compilers, and make CompilerArgs.to_native() translate them to --env-set FOO=BAR. That would make stuff like add_project_arguments() useful for Rust as well.

[xclaesse]

As far as I know, only Rust is crazy enough to use env at compile time, this should at least be renamed to rust_env.

[tristan957]

@dcbaker Wondering if we should instead pretend Rust uses -DFOO=BAR like every sane compilers, and make CompilerArgs.to_native() translate them to --env-set FOO=BAR. That would make stuff like add_project_arguments() useful for Rust as well.

I don't quite understand what you are saying, but it doesn't sound great.

add_project_arguments('--set-env=Hello=World', language: 'rust')

[xclaesse]

the fallback to use a wrapper is currently not implemented

Not sure we need to implement it. We could just make env usage requiring recent rustc version. Rust devs are used to always use latest version, that's not a problem.

[eli-schwartz]

Yes, I think it's extremely reasonable to say that we support this but only with a recent enough rustc version that it can be done sanely.

I've objected all along to adding a python wrapper which juggles os.environ -- the --set-env argument to rustc brings this more in line with -D from C/C++, which is fantastic news and makes this sane to implement. We should leave it at that.

[xclaesse]

I don't quite understand what you are saying, but it doesn't sound great.

add_project_arguments('--set-env=Hello=World', language: 'rust')

That's fair enough. In that case we don't need env kwarg at all, we can do:

executable(
  'main'
  'main.rs',
  rust_args : ['--env-set=OUT_DIR=@0@'.format(meson.current_build_dir()],
)

EDIT: What I meant is we could also allow rust_args : ['-DOUT_DIR=@0@'.format(meson.current_build_dir()], and Meson could translate that to --env-set. Just like we translate -L to /LIBPATH for MSVC. But I agree that could be a bit crazy idea :P

[dcbaker]

I've objected all along to adding a python wrapper which juggles os.environ -- the --set-env argument to rustc brings this more in line with -D from C/C++, which is fantastic news and makes this sane to implement. We should leave it at that.

Unfortunately it's not yet stable, so we're basically telling people "you have to use an unstable feature in a nightly compiler, sorry. That's not great user experience. I was going to propose that the env wrapper be added with an explicit deprecation and remove in 1.6 or 1.7, with the argument that if it's stabilized soon, that there would be a least a few rustc versions with it stabilized and we're no longer telling people "hey this thing that you really need to do idiomatic rust (ie, crates) isn't available, sorry", but we're also being clear that this is a stop gap and we're not going to provide long term fallback support since it's slow and bad. It also doesn't change anything from a language point of view so the cost from our point of view is fairly small. I might be convinced to do away with it entirely, but that assumes that it's stabilized in the next rust release.

We certainly could drop the env keyword argument, I had mostly used it because originally this was only implemnted as a wrapper (it's obviously been in the works a long time). I think my preference would be to drop env entirely. I'm nervious about the -D thing, since what do we do if rust adds support for -D to mean somethign else in the future?

[dcbaker]

This push is ugly, it needs some patches split, doc changes, etc. However, it does away with the env keyword, and just reads the --set-env command directly in the backend to decide to use the wrapper. The result is that we have barely any code required for the wrapper (and I can probably tuck it all up in one comment for easy removal later), to get a useful feature, and it's still only used if the feature is enabled.