cc-wrapper: warn if clang's --target option is used on a wrapped compiler

[risicle]

Description of changes

A reaction to #318674. People shouldn't expect using the --target argument of a wrapped compiler to work properly. I'd personally be in favour of a hard fail in this case because it's only an accident if it appears to work ok.

Note this is the first occurrence of cc-wrapper emitting stderr output in non NIX_DEBUG mode, but warnings tend not to have the desired effect if you need to ask for them first..

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[alyssais]

I disagree with this change. Upstreams rightly expect to be able to use Clang with custom targets (at least with some, like bpf). They probably do not expect Clang to have a full cross sysroot set up, but they do expect it to work for targets that don't need that, and it's valid of them to expect that.

How about this? If a custom target is used, we exec the unwrapped compiler, to make it as likely as possible that simple uses with no libraries will work. Additionally, if it's not on a list of targets we consider safe (which should definitely include bpf and probably also wasm but not wasi), we warn before execing?

[risicle]

I don't think passing to an unwrapped compiler is going to work very well, because it won't be able to find requisite headers.

I think in the long run there's a need for a different type of wrapping - one for non-"system" targets, like wasm and gpu kernels, that have a fraction of the concerns that cc-wrapper deals with (though at the same time I probably wouldn't want to start writing a new cc-wrapper-alike without any shared code). See how awkward I found it to cook up a custom platform object for wasm32-unknown-unknown in #318674 (comment)

Perhaps cc-wrapper could take a delegatedTargetCompilers argument that mapped target names to other compilers.

Still, I do think we should be warning people that "this may not work right" until such time as we have a better solution.

[alyssais]

I don't think passing to an unwrapped compiler is going to work very well, because it won't be able to find requisite headers.

Fair enough. I guess even with BPF, you could have header-only libraries you'd want to use, and it's only a warning.

Maybe as we get more experience with this it'll become clearer what a good solution will be…

[emilazy]

This caused a bunch of Rust‐related headaches recently and I believe it’s still broken there, so yeah I approve of this as a general warning. Even if we want this to work properly, it currently doesn’t.

[emilazy]

FWIW my main concern is that this might be spammy with other build tools that pass the hostPlatform --target unconditionally, but it’s probably not worth coding up a workaround for that case until we run into it.

[mschwaig]

This broke ROCm (because of course it did 😂). See: #375745 and #375850

I was wondering if you had some specific mechanism in mind that would work well for the workaround. 🤔

I'm guessing reverting this or adding an environment variable to turn this off in some places would not be a practical fix because it would cause a mass rebuild and therefore have a really long turnaround time?

[emilazy]

Fun.

As discussed in #370180, ROCm’s use of a wrapped compiler seems a bit weird/dodgy in general. But in this case I guess it’s passing the correct target anyway? We could refine this check so that it accepts --target as long as it matches the expected one.

cc-wrapper changes will need to go through staging and therefore take a couple of weeks, yes. In the meantime perhaps you can get the tests to stop passing --target or redirect their standard error or something. Maybe with a really hacky wrapper over the wrapper.

[lzcunt]

We've hit an issue related to this change in the limine package, from what I understand we should be passing the unwrapped clang to use for the UEFI target right? there's #377696 that does this but I haven't reviewed it yet. it would be awesome if we had a UEFI toolchain in nixpkgs to not have to do that workaround

How about this? If a custom target is used, we exec the unwrapped compiler, to make it as likely as possible that simple uses with no libraries will work. Additionally, if it's not on a list of targets we consider safe (which should definitely include bpf and probably also wasm but not wasi), we warn before execing?

This seems to be a perfect solution for packages that target UEFI and only depend on vendored-in dependencies

[th7nder]

We've encountered issue with this solution as well.
It fails for us in an unusual place, e.g. rust library tries to detect whether a given toolchain supports a flag -std=c++17.
https://github.com/brson/wasm-opt-rs/blob/8854bd8a58d7393c7bacd950884aea521ce8e19f/components/wasm-opt-sys/build.rs#L365

Code responsible for that is in the rust ecosystem crate:
https://github.com/rust-lang/cc-rs/blob/c82ea31f88ad999e3b2a287ac7c1039f53e975e1/src/lib.rs#L1365
It basically compiles a file and checks whether exit code was 0 and nothing was on the stderr.
It sets the -target by default.

However something was in on the stderr:

stderr: "Warning: supplying the --target argument to a nix-wrapped compiler may not work correctly - cc-wrapper is currently not designed with multi-target compilers in mind. You may want to use an un-wrapped compiler instead.\n"

I think this warning gives an unintended side effect which doesn't make sense in this case.

@emilazy what do you think?

[risicle]

My (brief) reading of the code is it only uses --target when it's being configured for cross-compiling - and cross-compiling with a compiler wrapped for another platform is one of those things that this warning was supposed to flag up as something that "only works by accident". The silly bit here is wasm-opt-sys taking any warning as a failure.

[th7nder]

The --target is set almost always.

1. is_flag_supported calls is_flag_supported_inner with self.get_target() https://github.com/rust-lang/cc-rs/blob/c82ea31f88ad999e3b2a287ac7c1039f53e975e1/src/lib.rs#L1249
2. self.get_target() sets the target always, either from env variable or from cache: https://github.com/rust-lang/cc-rs/blob/c82ea31f88ad999e3b2a287ac7c1039f53e975e1/src/lib.rs#L3512.
3. try_get_compiler() gets it: https://github.com/rust-lang/cc-rs/blob/c82ea31f88ad999e3b2a287ac7c1039f53e975e1/src/lib.rs#L1913
4. It adds default flags, based on the environment variables by default:
   https://github.com/rust-lang/cc-rs/blob/c82ea31f88ad999e3b2a287ac7c1039f53e975e1/src/lib.rs#L1921

It's set both on Linux and MacOS by default.
We can disable setting those flags by setting CRATE_CC_NO_DEFAULTS=1.
I set it, tested it and it now compiles, however - I'm not sure whether there are any huge implications of using this flag.

I get that the cross-compiling using a wrapped compiler for another platform is not a good idea. However the case here is that they cross-compile for the same platform as compiler, for some reason.
This issue will affects all of the crates in the Rust ecosystem which are using cc-rs and detecting some flags.
I guess that's a lot.

Maybe we should add this warning only if TARGET != HOST_PLATFORM? Would that be possible?

[9999years]

Yup, we see this warning all the time with GHC too.