Add clarifications to environment-variables.md that will help people not break their builds when targeting specific CPUs

[alexpyattaev]

Added clarification for use of RUSTFLAGS variable (and some minor tweaks around it)

RUSTFLAGS environment variable is too broad and should be avoided when doing any cross-compiling (or targeting specific CPU model). This is now clarified here.

What does this PR try to resolve?

A confusing lack of information regarding the effect of *RUSTFLAGS environment variables. This is a doc change only.

Additional information

In general, the only reason for this issue to be relevant is that Cargo profiles can not set rustflags (there is an open issue for that already). In the meantime, env variables are the way to go.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[weihanglo]

Thanks for the effort!

I got some concerns around the change:

• What does “production code” mean here? Is it --release build or something else?
• If I understand correctly, under some circumstances, proc macros and build scripts also get these rustflags. build.rustflags should have explained this already.
• In build.rustflags doc we already have some "Caution". We'd better consolidating rustflags warning in once place, and build.rustflags looks like a better place to me.
• Continue. Not only platform-specific flags could break the build. Any contradictory to what Cargo passes to rustc could potential break any build.

[weihanglo]

In general, the only reason for this issue to be relevant is that Cargo profiles can not set rustflags (there is an open issue for that already). In the meantime, env variables are the way to go.

Yep but not only an open issue, it is an unstable feature -Zprofile-rustflags people can try today. Feel free to give feedback there :)

[alexpyattaev]

Well yes, an unstable feature exists, but using it is tricky since it requires switching to nightly compiler as well (which is not something users necessarily want to do in production).

The actual use case for these environment variables "in production" is such that when you are producing a binary, you normally want it to be built for specific CPU on which it will run. I am not talking arm vs x86 here, more like AMD Ryzen 3 vs Intel Cascade Lake. --release will only go as far as enabling some basic optimizations, but not CPU-specific stuff. CPU specific features are critical for vectorization and some other optimizer tricks that LLVM can apply, and not making use of them is very wasteful. To enable CPU-specific optimizations, one needs to set rustc flags such as -C target-cpu=znver3. Setting them using RUSTFLAGS (which is the "natural" thing to try) results in builds failing with illegal instruction errors, since cargo starts producing procmacros using the architecture specified in RUSTFLAGS. In fact, what is needed is CARGO_BUILD_RUSTFLAGS to be set instead, which will affect the target code but not any auxillary stuff like procmacros. Setting RUSTFLAGS directly is thus somewhat misleading. Unless the cargo profiles specific compiler flags are stable, setting CARGO_BUILD_RUSTFLAGS is also the only way to produce actually optimized builds.

One other small note - setting target CPU in cargo.toml is very bad since it would need to be changed for each CPU family in the deployment. I currently have 3 CPU families, so having 3 distinct cargo.toml for them would be very silly and prone to bugs. Instead I have a small shell script that runs cargo with different CARGO_BUILD_RUSTFLAGS settings and different CARGO_TARGET_DIR as well.

If you feel there is a better place where this sort of information "belongs" please let me know.

[weihanglo]

As aforementioned, the doc of build.rustflags of .cargo/config.toml is a better place if we want everything to be in one place, and there is already a similar notice:

cargo/src/doc/src/reference/config.md

Lines 479 to 483 in 5a9f611

	> **Caution**: Due to the low-level nature of passing flags directly to the
	> compiler, this may cause a conflict with future versions of Cargo which may
	> issue the same or similar flags on its own which may interfere with the
	> flags you specify. This is an area where Cargo may not always be backwards
	> compatible.

[alexpyattaev]

After reading the doc for build.rustflags (especially without reading the doc for the environment variables) a reader would be even more confused.
The build.rustflags variable is said to be affected by environment variables "CARGO_BUILD_RUSTFLAGS or CARGO_ENCODED_RUSTFLAGS or RUSTFLAGS", but then 1 paragraph down from there it does not mention CARGO_BUILD_RUSTFLAGS at all (even though it clearly does affect compiler flags). I can go over that file and add patches to that too so it is consistent with the environment variables file. I suppose a better way would be to make environment.md file do nothing but contain pointers to the config.md, so that the actual descriptions are not duplicated (and thus are not out of sync w.r.t. each other.) What do you think?

[weihanglo]

Yes. It is a bit confusing. This paragraph explains that CARGO_BUILD_RUSTFLAGS is actually equivalent to build.rustflags but in the disguise of an environment variable. The kind of mapping between env and config is the secret sauce of Cargo configuration system but something people do feel confused. See #12087 for a similar discussion.

Not sure how to write it down well without adding more complexity (more text usually mean more cognitive overhead). From tooling perspective, personally I do want a simpler way to inspect actual rustflags being passed in. (cargo --verbose does that in a "verbose` way though)

[weihanglo]

I feel like we were a bit off rails. There were two issues in the discussion:

1. Incorrect RUSTFLAGS setting may break one's build, and there is no warning in the Environments chapter.
2. The confusion of the interaction and precedence of rustflags related env vars and configs.

For the first one, we have a Caution paragraph in build.rustflags so personally I don't think we need the duplication in environment doc.

For the second one, we also have CARGO_TARGET_<triple>_RUSTFLAGS not mentioned in build.rustflags. And sorry I currently have no idea how to reduce the confusion.

I am going to close this, and we can discuss any idea in #12087, like something this comment said #12087 (comment).

Thank you for the pull request :)

[alexpyattaev]

Fair enough. I suppose one day the proper targets configuration will become a thing and all this silly environment variable business can RIP.