Inconsistent handling of debug = 0 vs debug = false in profiles

[imotov]

Problem

After upgrading quickwit-oss/quickwit to rust toolchain 1.69 we discovered that all crates with edition = 2018 are getting recompiled when we run cargo run immediately after a successful run of cargo build. Upon further investigation, we found that cargo build compiles 2018 edition crates with -C debuginfo=0 argument while cargo run omits this flag triggering a change of fingerprint and recompilation. Once we replaced the section

[profile.dev]
debug = 0

with

[profile.dev]
debug = false

in Cargo.toml the recompilation issues disappeared.

Steps

Unfortunately, I wasn't able to reproduce that on a smaller project, but it is reproducible on a bigger project on Ubuntu 22.04. To reproduce:

1. Make sure that you have protoc and cmake installed on your system. It is needed by some dependencies and can be installed by running sudo apt install -y protobuf-compiler cmake on ubuntu
2. git clone https://github.com/quickwit-oss/quickwit.git
3. cd quickwit/quickwit
4. open Cargo.toml in a text editor and replace debug = false with debug = 0
5. cargo clean
6. cargo build
7. cargo run
8. Observe recompilation on the step 7
9. Replace debug = 0 with debug = false, repeat steps 5-7, observe not recompilation on the step 7.

Note: you don't have to wait for the step 6 to finish. Very early in the build it will try to compile the unicode_ident crate and in cargo build it will look something like this

Running `CARGO=/home/igor/.rustup/toolchains/1.69-aarch64-unknown-linux-gnu/bin/cargo CARGO_CRATE_NAME=unicode_ident CARGO_MANIFEST_DIR=/home/igor/.cargo/registry/src/github.com-1ecc6299db9ec823/unicode-ident-1.0.8 CARGO_PKG_AUTHORS='David Tolnay <dtolnay@gmail.com>' CARGO_PKG_DESCRIPTION='Determine whether characters have the XID_Start or XID_Continue properties according to Unicode Standard Annex #31' CARGO_PKG_HOMEPAGE='' CARGO_PKG_LICENSE='(MIT OR Apache-2.0) AND Unicode-DFS-2016' CARGO_PKG_LICENSE_FILE='' CARGO_PKG_NAME=unicode-ident CARGO_PKG_REPOSITORY='https://github.com/dtolnay/unicode-ident' CARGO_PKG_RUST_VERSION=1.31 CARGO_PKG_VERSION=1.0.8 CARGO_PKG_VERSION_MAJOR=1 CARGO_PKG_VERSION_MINOR=0 CARGO_PKG_VERSION_PATCH=8 CARGO_PKG_VERSION_PRE='' LD_LIBRARY_PATH='/home/igor/Projects/quickwit-oss/quickwit/quickwit/target/debug/deps:/home/igor/.rustup/toolchains/1.69-aarch64-unknown-linux-gnu/lib:/home/igor/.rustup/toolchains/1.69-aarch64-unknown-linux-gnu/lib' rustc --crate-name unicode_ident --edition=2018 /home/igor/.cargo/registry/src/github.com-1ecc6299db9ec823/unicode-ident-1.0.8/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --diagnostic-width=181 --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no -C debuginfo=0 -C metadata=ae9e877358182a13 -C extra-filename=-ae9e877358182a13 --out-dir /home/igor/Projects/quickwit-oss/quickwit/quickwit/target/debug/deps -L dependency=/home/igor/Projects/quickwit-oss/quickwit/quickwit/target/debug/deps --cap-lints warn`

But in cargo run it will be compiled like this:

Running `CARGO=/home/igor/.rustup/toolchains/1.69-aarch64-unknown-linux-gnu/bin/cargo CARGO_CRATE_NAME=unicode_ident CARGO_MANIFEST_DIR=/home/igor/.cargo/registry/src/github.com-1ecc6299db9ec823/unicode-ident-1.0.8 CARGO_PKG_AUTHORS='David Tolnay <dtolnay@gmail.com>' CARGO_PKG_DESCRIPTION='Determine whether characters have the XID_Start or XID_Continue properties according to Unicode Standard Annex #31' CARGO_PKG_HOMEPAGE='' CARGO_PKG_LICENSE='(MIT OR Apache-2.0) AND Unicode-DFS-2016' CARGO_PKG_LICENSE_FILE='' CARGO_PKG_NAME=unicode-ident CARGO_PKG_REPOSITORY='https://github.com/dtolnay/unicode-ident' CARGO_PKG_RUST_VERSION=1.31 CARGO_PKG_VERSION=1.0.8 CARGO_PKG_VERSION_MAJOR=1 CARGO_PKG_VERSION_MINOR=0 CARGO_PKG_VERSION_PATCH=8 CARGO_PKG_VERSION_PRE='' LD_LIBRARY_PATH='/home/igor/Projects/quickwit-oss/quickwit/quickwit/target/debug/deps:/home/igor/.rustup/toolchains/1.69-aarch64-unknown-linux-gnu/lib:/home/igor/.rustup/toolchains/1.69-aarch64-unknown-linux-gnu/lib' rustc --crate-name unicode_ident --edition=2018 /home/igor/.cargo/registry/src/github.com-1ecc6299db9ec823/unicode-ident-1.0.8/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --diagnostic-width=181 --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no -C metadata=6bb1a5f23e3b8481 -C extra-filename=-6bb1a5f23e3b8481 --out-dir /home/igor/Projects/quickwit-oss/quickwit/quickwit/target/debug/deps -L dependency=/home/igor/Projects/quickwit-oss/quickwit/quickwit/target/debug/deps --cap-lints warn`

The rest is basically just a fallout from this one other dependency recompilation. With debug = false the -C debuginfo=0 flag is not present in either builds.

Possible Solution(s)

I suspect if might have been caused by the change in the debug value parsing in #11252 but I didn't have time to investigate further.

Notes

The issue is reproducible on 1.69.0 as well as on beta and nightly. It is not reproducible on 1.68.2 and below.

Version

cargo 1.69.0 (6e9a83356 2023-04-12)
release: 1.69.0
commit-hash: 6e9a83356b70586d4b77613a6b33f9ea067b9cdf
commit-date: 2023-04-12
host: aarch64-unknown-linux-gnu
libgit2: 1.5.0 (sys:0.16.0 vendored)
libcurl: 7.86.0-DEV (sys:0.4.59+curl-7.86.0 vendored ssl:OpenSSL/1.1.1q)
os: Ubuntu 22.04 (jammy) [64-bit]

[weihanglo]

There are several issues behind this.

First,

quickwit-cli is the only member with default-run specified. That means when you run cargo run for the workspace, it will then translate into cargo run --package quickwit-cli. One optimization in #11252 is that if a build-dependency is also used as other kind of dependency in your dependency graph, it won't turn debuginfo off. So, for the first cargo build the dependency graph contains unicode-ident as a normal dependency in quickwit-codegen, which makes the build set -C debuginfo=0 explicitly.

cargo/src/cargo/ops/cargo_compile/mod.rs

Lines 690 to 692 in a6c0d43

	// The unit is present in both build time and runtime subgraphs: we canonicalize its
	// level to the other unit's, thus ensuring reuse between the two to optimize build times.
	canonical_debuginfo

Second,

setting debug = false does solve the issue, however, coincidentally. Unfortunately, it no longer work on the current nightly. Before #11958, debug = false was deserialized to DebugInfo::None for a certain Cargo profile, which implies Cargo won't set -C debuginfo for rustc. After #11958 (targeted at 1.71.0), when profile.<name>.debug = "something" is present, debug = false and debug = 0 are always deserialized to DebugInfo::Explicit(_), letting Cargo always set -C debuginfo.

Before:

cargo/src/cargo/core/profiles.rs

Line 534 in d71aa2e

Some(U32OrBool::Bool(false)) => profile.debuginfo = DebugInfo::None,

After:

cargo/src/cargo/core/profiles.rs

Lines 531 to 533 in a6c0d43

	if let Some(debuginfo) = toml.debug {
	profile.debuginfo = DebugInfo::Explicit(debuginfo);
	}

The third one

is also interesting. When Cargo figures out that it can turn off debuginfo for a build dependency (when it is not shared with runtime dependencies), it turns DebugInfo into DebugInfo::None. This is done by DebugInfo::weaken function here:

cargo/src/cargo/core/profiles.rs

Lines 783 to 785 in a6c0d43

	pub(crate) fn weaken(self) -> Self {
	DebugInfo::None
	}

That means, as aforementioned, Cargo won't pass any -C debuginfo to rustc due to profile.<name>.debug is deserialized as DebugInfo::None, even when you deliberately set profile.<name>.debug = 0. This is potentially harmful and may lead to some build cache misses, as profile is hashed into Cargo fingerprint to determine if it needs to recompile. I might propose only weaken debuginfo when it is set explicitly. Here is a patch:

diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs
index afaf3fb4f..e3da43093 100644
--- a/src/cargo/core/profiles.rs
+++ b/src/cargo/core/profiles.rs
@@ -781,7 +781,10 @@ impl DebugInfo {
 
     /// Reset to the lowest level: no debuginfo.
     pub(crate) fn weaken(self) -> Self {
-        DebugInfo::None
+        match self {
+            DebugInfo::None => DebugInfo::None,
+            _ => DebugInfo::Explicit(TomlDebugInfo::None),
+        }
     }
 }

---

It was a mix of implicit stuff that made this "bug". For your case, if you run cargo build -p quickwit-cli followed by cargo run -p quickwit-cli, everything will be fine even on nightly. I will try submitting a PR with my patch above so that every rustc invocation will contain -C debuginfo=0 in your case. Not sure if it brings more harm though. Need to think about it more.

[weihanglo]

The fix #12165 is also targeted a beta (1.70.0) backport with #12167.

@imotov, you can clone cargo repo and try it out, or wait a few days later for nightly/beta toolchains getting the update. Thank you.

[jyn514]

I missed this bug, sorry for the regression.

@weihanglo I wonder why Debuginfo::None exists at all? Can cargo explicitly pass -C debuginfo=0 every time instead? that should avoid caching issues and seems simpler than the fix in your PR.

[jyn514]

Alternatively we could deserialize = false into Debuginfo::None and remove TomlDebugInfo::None, i.e. never pass =0 to rustc explicitly.

[weihanglo]

Both seems valid to me, especially the second one. I am not sure why Debuginfo::None exists. Perhaps just reflect debuginfo = false in Cargo profile setting. Given the bug has been fixed I am fine with someone filing a fix or leaving it as-is.

[weihanglo]

BTW my PR is also simple in terms of code added. Just the matrix looks intimidating 😆