Skip to content
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

Multiple rustflags configs should be cumulative #5376

Open
SimonSapin opened this issue Apr 17, 2018 · 24 comments
Open

Multiple rustflags configs should be cumulative #5376

SimonSapin opened this issue Apr 17, 2018 · 24 comments
Labels
A-configuration Area: cargo config files and env vars A-rustflags Area: rustflags S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@SimonSapin
Copy link
Contributor

Example .cargo/config:

[target.'cfg(any(target_arch="arm", target_arch="aarch64"))']
rustflags = ["-C", "target-feature=+neon"]

[build]
rustflags = ["-W", "unused-extern-crates"]

When running for example cargo build --target armv7-linux-androideabi I’d expect both sets of flags to be passed to rustc, but only the former is.

When running RUSTFLAGS="-C link-args=-fuse-ld=gold" cargo build --target armv7-linux-androideabi I’d expect all three sets of flags, but only the environment variable one is used and both configs are ignored.

The intra-config-file case can be worked around by duplicating some config, but the environment variable is still useful for wrapper scripts like Servo’s mach to dynamically choose to use some flags or not. (In this case depending on testing the presence of a ld.gold executable.) Setting some flags through a config file is useful when cargo build is used directly without going through the wrapper script. (CC https://www.mail-archive.com/dev-servo@lists.mozilla.org/msg02703.html.)

@alexcrichton
Copy link
Member

This seems like a reasonable interpretation of RUSTFLAGS to me, @matklad do you think we should be worried about breakage if we implemented these semantics?

@matklad
Copy link
Member

matklad commented Apr 17, 2018

Hm, I am actually unsure about semantics... It seems to me that sometimes you'd want to override rustflangs? For example, for an hierarchy of .cargo/config at various directory levels, I certainly expect the overriding behavior. It also seems reasonable and sometimes useful that env-var overrides config...

Implicit merging is also not a great UX because it's not clear that merging is happening somewhere.

What do you think about some cryptic opt-in syntax for merging?

RUSTFLAGS="+ -C link-args=-fuse-ld=gold"

perhaps?

As for breakage, I think the path of "0. feature flag 1. irlo announcement 2. flip default on nightly and see if anything breaks too terribly" is a reasonable one in this circumstances.

@SimonSapin
Copy link
Contributor Author

For example, for an hierarchy of .cargo/config at various directory levels, I certainly expect the overriding behavior.

For what it’s worth I would also expect accumulation in this case, not overriding.

RUSTFLAGS="+ …"

That sounds remarkably obscure.

@alexcrichton
Copy link
Member

@matklad true yeah, in any case that sequence of events sounds reasonable to me!

@Boscop
Copy link

Boscop commented Jul 16, 2018

In my use case I have a workspace whose toplevel crate has a .cargo/config of

[build]
rustflags = ["-Ctarget-feature=+crt-static", "-Ctarget-cpu=haswell"]

But in one of the subcrates (my app's yew wasm frontend), I have

[build]
rustflags = "" # clear the rustflags from the parent config
target = "wasm32-unknown-unknown"

Is this the correct way to prevent the parent config from affecting the subcrate?
I wish there was a better way that wouldn't require me to manually overwrite all the keys I set in the parent config in every subcrate's config where I don't want to inherit the config..

@SimonSapin
Copy link
Contributor Author

@Boscop I believe that Cargo’s "config" is determined once per run of the cargo command, you cannot have a different config in different crates that are compiled together.

@Boscop
Copy link

Boscop commented Jul 17, 2018

The frontend crate is not compiled together with the toplevel (backend) crate. But they are both in the same workspace because they are part of the same project and they both depend on some shared crates for communication (that are also part of this workspace).

@jac-cbi
Copy link

jac-cbi commented Mar 3, 2020

In the interim, would it be possible just to warn the user that if both RUSTFLAGS is set and .cargo/config exists, "WARN: $RUSTFLAGS overrides anything set in .cargo/config when it exists"?

This would've save me and the #rust-embedded Matrix room a good eight hours today due to my footgun.

@ehuss
Copy link
Contributor

ehuss commented Apr 13, 2021

#6338 contains some additional discussion of this issue.

@m-ou-se
Copy link
Member

m-ou-se commented Sep 23, 2021

I ran into this problem again just now.

I have a ~/.cargo/config containing:

[target.aarch64-unknown-linux-musl]
linker = "aarch64-linux-musl-gcc"
rustflags = ["-Lnative=/usr/aarch64-linux-musl/lib"]

And everything built fine except in a CI environment with RUSTFLAGS=-Dwarnings set. It turns out that overrides the rustflags setting from the target settings, resulting in missing libraries.


While hacking around it, I noticed I hacked around a similar problem before where I had a $project/.cargo/config containing:

[build]
target = "thumbv7em-none-eabihf"

[target.thumbv7em-none-eabihf]
rustflags = ['-Clink-arg=-Tlink.x', '-Clink-arg=-s']

which was also ignored in CI because of RUSTFLAGS=-Dwarnings.

@flip1995
Copy link
Member

A Clippy contributor also just hit this issue, where their ~/.cargo/config file overrides the Clippy .cargo/config file, where we define rustflags.

I can see the overriding the build.rustflags with the env var RUSTFLAGS might be debatable. But from the Configuration documentation:

If a key is specified in multiple config files, the values will get merged together.

This seems not to be the case if the config value is specified under two different keys (target.<taget>.rustflags in ~/.cargo/config and build.rustflags in $PROJECT/.cargo/config)

So would a patch that merges the rustflags config no matter the origin be acceptable as a first step?

@alexcrichton
Copy link
Member

I think at the very least it's pretty reasonable to merge target-specific rustflags and "bland rustflags". I suspect that can be mostly just done through a PR.

For build.rustflags and RUSTFLAGS we now also have CARGO_ENCODED_RUSTFLAGS so it's a bit tricky. We have precedent for env vars overriding what's in the config file but that's typically for one-off settings rather than settings that are lists like RUSTFLAGS is. Personally I feel like the use cases for not appending are few and far between and would be ok taking a PR to implement it, but I'd want others to weigh in first before a PR is actually sent.

@jonhoo
Copy link
Contributor

jonhoo commented Nov 23, 2021

This also potentially interacts with #7722 — if I specify --config build.rustflags, should that be merged with [build] rustflags = ".." (and if so, with which ones if [build] rustflags is set in multiple config files) or should it completely override it?

@jonhoo
Copy link
Contributor

jonhoo commented Nov 23, 2021

From a basic experiment it looks like --config works as expected (as in, it merges) with rustflags:

#!/bin/bash

set -euo pipefail

rm -rf cargo-rustflags
mkdir cargo-rustflags
cd cargo-rustflags

mkdir cargo-home
cat >cargo-home/config.toml <<EOF
[build]
rustflags = ["--cfg=cargo_home"]
EOF

cargo new --lib project
cd project

mkdir .cargo
cat >.cargo/config.toml <<EOF
[build]
rustflags = ["--cfg=dot_cargo_config"]
EOF

cat >build.rs <<EOF
fn main() {
    let dash_cfgs: Vec<_> = std::env::var("CARGO_ENCODED_RUSTFLAGS")
      .unwrap()
      .split('\\x1f')
      .filter_map(|flag| flag.starts_with("--cfg").then(|| flag))
      .map(String::from)
      .collect();
    assert!(false, "\n{}\n", dash_cfgs.join("\n"));
}
EOF

function cfgs() {
	(env CARGO_HOME="$PWD/../cargo-home" cargo "$@" check 2>&1 || true) | sed 's/^ *//' | grep -E '^--cfg'
}

echo "rustc cfg flags with straight build"
cfgs

echo "rustc cfg flags with --config"
cfgs +nightly -Z unstable-options --config='build.rustflags = ["--cfg=cli"]'

echo "rustc cfg flags with RUSTFLAGS"
(
	export RUSTFLAGS="--cfg=rustflags"
	cfgs
)
rustc cfg flags with straight build
--cfg=dot_cargo_config
--cfg=cargo_home
rustc cfg flags with --config
--cfg=dot_cargo_config
--cfg=cargo_home
--cfg=cli
rustc cfg flags with RUSTFLAGS
--cfg=rustflags

@marieell
Copy link

marieell commented Feb 8, 2022

I've just been hit by RUSTFLAGS overwriting .cargo/config.toml's build.rustflags, too. I assumed RUSTFLAGS was modeled after CFLAGS, which tools usually just concatenate, too. I suppose intentional disabling of flags configured elsewhere is pretty rare, but even these use-cases can be solved by explicitly overwriting with the desired values higher up (i. e. RUSTFLAGS).

@danlapid
Copy link

This is indeed very confusing behavior
Attached is a test case and a possible solution in case a decision is made for it to be changed:

#[cargo_test]
fn build_flags_and_target_flags() {
    let config = format!(
        r#"
            [build]
            rustflags = ["-Wunused"]
            [target.'cfg(target_arch = "{}")']
            rustflags = ["-Wunsafe-code"]
        "#,
        std::env::consts::ARCH
    );

    let p = project()
        .file(".cargo/config.toml", config.as_str())
        .file("src/lib.rs", r#""#)
        .build();

    p.cargo("build -vv")
        .with_stderr_contains("[..]-Wunsafe-code[..]")
        .with_stderr_contains("[..]-Wunused[..]")
        .run();
}

A possible solution would be changing core/compiler/build_context/target_info.rs:env_args
end of function to:

    let mut all_rustflags = Vec::new();
    if let Some(rustflags) = rustflags_from_env(flags) {
        all_rustflags.extend(rustflags);
    }
    if let Some(rustflags) = rustflags_from_target(config, host_triple, target_cfg, kind, flags)? {
        all_rustflags.extend(rustflags);
    }
    if let Some(rustflags) = rustflags_from_build(config, flags)? {
        all_rustflags.extend(rustflags);
    }
    Ok(all_rustflags)

@kirawi
Copy link

kirawi commented Nov 6, 2023

I believe that project-local config.toml should override the global config. Currently this isn't the case, and there is no way for me to get around it without creating a project-local config for every other project just to handle issues with this one project.

# ~/.cargo/config.toml
[target.x86_64-unknown-linux-gnu]
rustflags = ["-C", "link-arg=-fuse-ld=mold", "-Zshare-generics=y"]
# $PROJECT/ .cargo/config.toml
[target.x86_64-unknown-linux-gnu]
rustflags = []

@Veetaha
Copy link

Veetaha commented Dec 3, 2023

This is a big pain point. I have a docker image that I use for CI runs. In that docker image I have mold linker installed, but I can't just configure cargo to use that linker by default in my image, because all config files get overridden by occasional RUSTFLAGS env var setup in the CI .yml template (e.g. to set --deny warnings). So I have to copy-paste the mold configs everywhere where RUSTFLAGS are set.

@hauleth
Copy link

hauleth commented Jan 14, 2024

@kirawi I think that could be configurable via additional option like:

# $PROJECT/ .cargo/config.toml
[target.x86_64-unknown-linux-gnu]
clean_env = true
rustflags = []

Or

# $PROJECT/ .cargo/config.toml
[target.x86_64-unknown-linux-gnu]
clean_env = [ "rustflags" ]
rustflags = []

Or setting flags to [] could have special meaning (though I do not know whether TOML supports repeated keys, so that could be tricky). Systemd uses that approach, but they use custom configuration format.

@RalfJung
Copy link
Member

Interestingly, target.runner seems to be appended, not overwriting? At least that's what #10893 looks like. But maybe there's some other relevant factor here.

This is the exact opposite of what you'd want, I think: target.runner is usually a binary with some flags, concatenating multiple of these basically never makes sense. rustflags is a list of flags passed to rustc, concatenating them is usually what you want.

@weihanglo
Copy link
Member

From the doc:

Arrays will be joined together with higher precedence items being placed later in the merged array.

Most of the arrays in Cargo configuration are cumulative. Even [alias] is, which is a bit odd. However, rustflags is one of them not.

@garyttierney
Copy link

garyttierney commented Mar 30, 2024

Hi all! Would this be a correct summary of the current behaviour?

  • RUSTFLAGS defined on the environment completely overrides any rustflags derived by Cargo config
  • rustflags defined under the same TOML section from multiple Cargo config.toml files will be concatenated
  • rustflags defined under different TOML sections from multiple Cargo config.toml files will take the one with highest precedence, see example below.

When we define rustflags under a target section globally (i.e. $HOME/.cargo/config.toml)

[target.x86_64-pc-windows-msvc]
linker = "lld"
rustflags = [
  "-Lnative=$HOME/.xwin/crt/lib/x86_64",
  "-Lnative=$HOME/.xwin/sdk/lib/um/x86_64",
  "-Lnative=$HOME/.xwin/sdk/lib/ucrt/x86_64"
]

and in <project>/.cargo/config.toml at the target level:

[build]
target = "x86_64-pc-windows-msvc"

[target.x86_64-pc-windows-msvc]
rustflags = ["-C", "target-feature=+crt-static"]

We get:

-Lnative=/home/gtierney/.xwin/crt/lib/x86_64 
-Lnative=/home/gtierney/.xwin/sdk/lib/um/x86_64
-Lnative=/home/gtierney/.xwin/sdk/lib/ucrt/x86_64 
-C target-feature=+crt-static

However, if we change <project>/.cargo/config.toml to specify rustflags under [build]

We get:

-Lnative=/home/gtierney/.xwin/crt/lib/x86_64
-Lnative=/home/gtierney/.xwin/sdk/lib/um/x86_64
-Lnative=/home/gtierney/.xwin/sdk/lib/ucrt/x86_64

If both specify rustflags under [build] then as in the [target] case they get concatenated.

@svix-jplatte
Copy link

Since it hasn't been mentioned in this thread yet, as a workaround you can do this instead:

[target.'cfg(all())']
rustflags = ["--cfg", "tokio_unstable"]

since target-specific rustflags are cumulative already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-rustflags Area: rustflags S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

No branches or pull requests