Build dependency on CARGO_ENCODED_RUSTFLAGS causes needless rebuilds

[GPSnoopy]

In our CI/CD, we do something along the following lines:

cargo build --all-targets
maturin develop --manifest-path src/python_bindings/Cargo.toml
cargo test

The problem is that portable-atomic has a build dependency on CARGO_ENCODED_RUSTFLAGS, see

portable-atomic/build.rs

Line 41 in 0479f5b

println!("cargo:rerun-if-env-changed=CARGO_ENCODED_RUSTFLAGS");

For some reason, portable-atomic is built each time (3 times) rather than once. Maturin passes the exact same CARGO_ENCODED_RUSTFLAGS environment variable as Cargo does, but the rebuild is still triggered by Cargo, saying that the aforementioned environment variable has changed. Adding debug printing (with proper non-ASCII escaping) in the portable-atomic build.rs file confirms that the env var is identical on each command.

This could be very a bug in cargo itself, but since portable-atomic is the only crate I've encountered with this build-time dependency, I thought the would be the right place for an initial issue report.

[taiki-e]

This is probably due to maturin setting as CARGO_ENCODED_RUSTFLAGS what was originally set as RUSTFLAGS.

If so, changing this line to the following would fix the rebuild problem (although that is not a better fix here).

https://github.com/PyO3/maturin/blob/fd85ab4f2af21d6e04abf96de704f6b0a60500c0/src/compile.rs#L362

From:

build_command.env("CARGO_ENCODED_RUSTFLAGS", rustflags.encode()?);

To:

build_command.env("RUSTFLAGS", rustflags.encode_space_separated()?);

I think a better fix here is to add a method like encode_preserve that respects the format of the rustflags source to in cargo-config2 and change maturin to use that.

[GPSnoopy]

I have managed to make a reproducible example: portable-atomic-maturin.zip

Typing:

cargo build --all-targets -vv
maturin develop -vv
cargo test -vv

will show for the last two commands:

       Dirty portable-atomic v1.6.0: the env variable CARGO_ENCODED_RUSTFLAGS changed
   Compiling portable-atomic v1.6.0

This is only reproducible if the following is added in `.cargo/config.toml':

[target.'cfg(target_arch = "x86_64")']
rustflags = "--codegen target-cpu=x86-64-v3"

[taiki-e]

Ah, that's a tricky issue involving interaction with config.

If cargo fixes a bug (rust-lang/cargo#13003), there would be no need for rerun-if-env-changed=*RUSTFLAGSs there in the first place although...

[GPSnoopy]

This is probably due to maturin setting as CARGO_ENCODED_RUSTFLAGS what was originally set as RUSTFLAGS.

I'm not sure I fully understand. Printing the content of the env var in portable-atomic build file shows the string to be completely identical between cargo build and maturin develop, including the weird ASCII separator character used by CARGO_ENCODED_RUSTFLAGS. Surely that should be enough for cargo not to rebuild portable-atomic, or is there a weird interaction with RUSTFLAGS and others that I'm not aware of?

(I've not tried your proposal yet, maturin is non trivial to rebuild from source and test)

[taiki-e]

I'm not sure I fully understand. Printing the content of the env var in portable-atomic build file shows the string to be completely identical between cargo build and maturin develop, including the weird ASCII separator character used by CARGO_ENCODED_RUSTFLAGS. Surely that should be enough for cargo not to rebuild portable-atomic, or is there a weird interaction with RUSTFLAGS and others that I'm not aware of?

(I've not tried your proposal yet, maturin is non trivial to rebuild from source and test)

Thanks to the detailed information provided, this turned out to be an irrelevant issue.

[taiki-e]

In my understanding, what is happening:

• rerun-if-env-changed=CARGO_ENCODED_RUSTFLAGS is not triggered if CARGO_ENCODED_RUSTFLAGS is set by the cargo as described here.

  portable-atomic/build.rs

  Lines 38 to 41 in 0479f5b

  	// Ideally, the build script should be rebuilt when CARGO_ENCODED_RUSTFLAGS
  	// is changed, but since it is an environment variable set by cargo,
  	// as of 1.62.0-nightly, specifying it as rerun-if-env-changed does not work.
  	println!("cargo:rerun-if-env-changed=CARGO_ENCODED_RUSTFLAGS");

  i.e., CARGO_ENCODED_RUSTFLAGS for rerun-if-env-changed is considered unset.

• maturin sets the CARGO_ENCODED_RUSTFLAGS even if it has not changed, so the conditions for rerun-if-env-changed=CARGO_ENCODED_RUSTFLAGS to be triggered are met when building with maturin.

  i.e., CARGO_ENCODED_RUSTFLAGS for rerun-if-env-changed is considered set.

There are several possible fixes here:

• Fix a cargo bug (Build script not rerun when target rustflags change rust-lang/cargo#13003) and remove rerun-if-env-changed=*RUSTFLAGS in the version that contains the fix. This is the best thing to do, and a PR to fix the bug has already been submitted, but it will take months to be available in stable even if it is merged right now.

• Accept the Cargo bug and remove the rerun-if-env-changed=*RUSTFLAGS. I do not want to do this if possible.

• Change maturin to not set CARGO_ENCODED_RUSTFLAGS if rustflags has not been changed. This is probably the most preferred way, because there are no adverse effects whatsoever and the fixes will be available as soon as they are released a new versions.

[taiki-e]

Change maturin to not set CARGO_ENCODED_RUSTFLAGS if rustflags has not been changed. This is probably the most preferred way, because there are no adverse effects whatsoever and the fixes will be available as soon as they are released a new versions.

Opened PyO3/maturin#1978 with this approach.

[taiki-e]

PyO3/maturin#1978 has been released in maturin 1.5.0.

[GPSnoopy]

You are an absolute hero! Impressive rollout with a new release of maturin within 24h of raising this initial issue.

We've tested maturin 1.5.0 and can confirm it fixes the rebuild issues. Timings of the CI/CD pipeline are much better.

[taiki-e]

FYI, the underlying Cargo bug has been fixed in the latest stable (1.79), and in the latest portable-atomic, the workaround in the portable-atomic side is only used for old Cargo.

portable-atomic/build.rs

Lines 65 to 72 in 6cc21f3

	// https://github.com/rust-lang/rust/pull/123745 (includes https://github.com/rust-lang/cargo/pull/13560) merged in Rust 1.79 (nightly-2024-04-11).
	if !version.probe(79, 2024, 4, 10) {
	// HACK: If --target is specified, rustflags is not applied to the build
	// script itself, so the build script will not be recompiled when rustflags
	// is changed. That in itself is not a problem, but the old Cargo does
	// not rerun the build script as well, which can be problematic.
	// https://github.com/rust-lang/cargo/issues/13003
	// This problem has been fixed in 1.79 so only older versions need a workaround.