rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.72k stars 2.42k forks source link

Multiple rustflags configs should be cumulative #5376

Open SimonSapin opened 6 years ago

SimonSapin commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

Boscop commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 4 years ago

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 commented 3 years ago

6338 contains some additional discussion of this issue.

m-ou-se commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

This also potentially interacts with https://github.com/rust-lang/cargo/issues/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 commented 2 years ago

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 commented 2 years ago

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).

Heliozoa commented 2 years ago

I hit this issue due to having a global $CARGO_HOME/config.toml to enable using mold as the linker

[target.x86_64-unknown-linux-gnu]
linker = "clang"
rustflags = ["-C", "link-arg=-fuse-ld=/usr/bin/mold"]

and trying to follow tokio_console's instructions for enabling the tokio_unstable flag for my project at $CARGO_MANIFEST_DIR/.cargo/config.toml

[build]
rustflags = ["--cfg", "tokio_unstable"]

I tried swapping the two configs, and it seems that Cargo will always prefer the target flags over the build ones, which in cases like this leads to a confusing situation where global flags override local ones. I didn't see this mentioned in the discussion so far so I thought it'd be good to bring it up.

My current solution is to add the global rustflags to the local config and to comment out the rustflags line from the global config when working on this project, which is a pretty awkward workflow.

danlapid commented 2 years ago

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 commented 11 months ago

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 commented 10 months ago

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 commented 9 months ago

@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 commented 7 months ago

Interestingly, target.runner seems to be appended, not overwriting? At least that's what https://github.com/rust-lang/cargo/issues/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 commented 7 months ago

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 commented 7 months ago

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

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 commented 4 months ago

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.