Add `cfg_version` support #15533

mo8it · 2025-05-16T15:36:21Z

What does this PR try to resolve?

#15531

mo8it · 2025-05-16T15:37:58Z

@epage I will of course take care of proper error handling, but I just want to get feedback on the initial direction.

~~cfg(nightly) can be already parsed as a Name.~~

weihanglo

I just tweaked the PR description a bit. We usually keep a tracking issue open until stabilization.

src/cargo/core/compiler/custom_build.rs

crates/cargo-platform/src/cfg.rs

epage · 2025-05-16T21:25:07Z

cfg(nightly) can be already parsed as a Name.

I'm not sure what this is referring to as I don't see this in the RFC or the rustc version

crates/cargo-platform/src/cfg.rs

src/cargo/core/compiler/custom_build.rs

epage · 2025-05-19T21:00:23Z

Be sure to check out rust-lang/rust#141137 for a summary of what has changed from the RFC

ehuss · 2025-05-20T16:14:24Z

A question came up about how older versions of cargo will handle this.

At least for a local package, this causes an error like:

failed to parse `version('1.80')` as a cfg expression: unexpected content `('1.80')` found after cfg expression

That seems fine to me.

However, I have a little more concern about how this impacts the index.

At a minimum, can you add a test that shows using a dependency from the index that itself uses a cfg() dependency? So, essentially, something like:

    Package::new("only_newer", "1.0.0").publish();
    Package::new("bar", "2.0.0")
        .file("src/lib.rs", "extern crate only_newer;")
        .target_dep("only_newer", "1.0", "cfg(version('1.89.0'))")
        .publish();

and having a regular dependency on bar. In my quick testing, this seems to be broken somehow, but I'm not sure how. Can you look into that?

Another thing to look into is if the crates.io parsing of the manifest will have any issues with this, or how it displays dependencies.

Unfortunately we don't have very good infrastructure for checking that older versions of cargo work with newer versions of the index. I think the best we have right now is #[cargo_test(requires_rustup_stable)], where we can make sure stable is compatible with what is generated by nightly. But that doesn't really help with a fixed version like "does rust 1.87 stay compatible with nightly". We have a whole module for doing that (https://github.com/rust-lang/cargo/blob/HEAD/tests/testsuite/old_cargos.rs), but those tests are disabled due to the cost.

If we are able to figure some way to test older versions, then one approach would be to make sure the index for a crate has a mix of old and new cfgs. So something like:

    Package::new("only_newer", "1.0.0").publish();
    Package::new("bar", "1.0.0").publish();
    Package::new("bar", "2.0.0")
        .file("src/lib.rs", "extern crate only_newer;")
        .target_dep("only_newer", "1.0", "cfg(version('1.89.0'))")
        .publish();

where you have a dependency on bar = "1.0" should still work on older versions of cargo (that is, they should not choke on the 2.0 index entry). AFAIK, that should be fine, since older cargos are supposed to ignore index entries it can't parse. However, I don't remember where cargo parses the cfg strings, and I don't want old versions to choke on that.

rustbot · 2025-05-20T18:00:02Z

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

epage · 2025-05-20T18:01:05Z

crates/cargo-platform/src/cfg.rs

        match *self {
-            CfgExpr::Not(ref e) => !e.matches(cfg),
-            CfgExpr::All(ref e) => e.iter().all(|e| e.matches(cfg)),
-            CfgExpr::Any(ref e) => e.iter().any(|e| e.matches(cfg)),
+            CfgExpr::Not(ref e) => !e.matches(cfg, rustc_version),
+            CfgExpr::All(ref e) => e.iter().all(|e| e.matches(cfg, rustc_version)),
+            CfgExpr::Any(ref e) => e.iter().any(|e| e.matches(cfg, rustc_version)),
+            CfgExpr::Value(Cfg::Version(CfgRustVersion { minor, patch })) => {
+                match minor.cmp(&rustc_version.minor) {
+                    Ordering::Less => true,
+                    Ordering::Equal => patch <= rustc_version.patch,
+                    Ordering::Greater => false,
+                }
+            }


I believe the tracking issue mentions specific behavior for nightlies

1.80 includes 1.80 nightlies

1.80.0 does not include 1.80 nightlies

I'm not quite following this. Can you say more about where that is being discussed? That doesn't seem to be the current behavior.

Maybe I'm misinterpeting what was said in

Ultimately, a compromise was agreed upon, in which nightly releases are treated as "complete" i.e. cfg(version(1.20)) evals to true on nightly builds with version 1.20.0, but there is a nightly flag -Z assume-incomplete-release to opt into the behaviour that doesn't do this assumption. This compromise was implemented in PR rust-lang/rust#81468.

The actual rustc tests should be checked to confirm. We probably should most or all of their tests.

Did I get the desired logic here?

cargo/crates/cargo-platform/tests/test_cfg.rs

Lines 224 to 234 in 120bea7

let v89_nightly = semver::Version {

major: 1,

minor: 89,

patch: 0,

pre: Prerelease::new("nightly").unwrap(),

build: BuildMetadata::EMPTY,

};

assert!(e!(version(89)).matches(&[], &v89_nightly));

assert!(!e!(version(89, 0)).matches(&[], &v89_nightly));

assert!(e!(version(88)).matches(&[], &v89_nightly));

assert!(e!(version(88, 0)).matches(&[], &v89_nightly));

cargo/crates/cargo-platform/src/cfg.rs

Lines 36 to 52 in 120bea7

pub fn matches(&self, rustc_version: &semver::Version) -> bool {

match self.minor.cmp(&rustc_version.minor) {

Ordering::Less => true,

Ordering::Equal => match self.patch {

Some(patch) => {

if rustc_version.pre.as_str() == "nightly" {

false

} else {

patch <= rustc_version.patch

}

}

None => true,

},

Ordering::Greater => false,

}

}

}

I don't think so. rustc ignores pre-release identifiers like nightly or beta. The logic is defined here via the PartialOrd derive. There is no special handling based on whether there is a patch specified, or if there is a pre-release.

The fun of going off of textual output as the lack of a patch version seems significant in that comment. I'm trying to get clarification on their testing situation as I can't tell the nightly behavior by looking at the UI tests.

@epage Any update on that?

I will have time on Tuesday or Wednesday to finish the PR after clarifying the desired behavior for nightly.

what ehuss said is what we should go with: all unstable versions are considered the same as their stable version. I tried pushing for explicit tests for this to explicitly specify the behavior but I'm getting push back. It doesn't help that the nightly behavior is considered unstable (comment) though its important for us to sync up on it.

crates/cargo-platform/src/cfg.rs

crates/cargo-platform/src/lib.rs

crates/cargo-platform/tests/test_cfg.rs

epage · 2025-05-20T18:18:37Z

tests/testsuite/cfg.rs

+#[cargo_test]
+fn cfg_version() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "a"
+                edition = "2015"
+
+                [target.'cfg(version("1.87.0"))'.dependencies]
+                b = { path = 'b' }
+            "#,
+        )
+        .file("src/lib.rs", "extern crate b;")
+        .file("b/Cargo.toml", &basic_manifest("b", "0.0.1"))
+        .file("b/src/lib.rs", "")
+        .build();
+    p.cargo("check -v").run();
+}


I wish we had a way to override the rust version so we could test with 1.2345 (the version we always use for "far future" in tests)

Could we use RUSTC_FORCE_RUSTC_VERSION?

crates/cargo-platform/src/cfg.rs

Co-authored-by: Ed Page <[email protected]>

mo8it · 2025-05-21T16:14:57Z

@epage Thanks for rewriting the history. I found it a bit disappointing that I wasn't listed as an author anymore. Therefore, I updated the author information of the commits and hope that you are happy with the current state.

I also fixed the CI by removing dbg!(input).

Finally, I resolved the merge conflicts.

Learning jj today helped a lot :D

If we agree on #15533 (comment), then the PR should be ready for merging.

epage · 2025-05-21T16:24:33Z

I found it a bit disappointing that I wasn't listed as an author anymore.

Sorry that I forgot to preserve that!

ehuss

Can you include some tests as mentioned from #15533 (comment)? Perhaps something like:

#[cargo_test]
fn cfg_version_from_index() {
    // Checks that cfg(version) works in the index.
    Package::new("only_newer", "1.0.0").publish();
    Package::new("bar", "1.0.0")
        .file("src/lib.rs", "extern crate only_newer;")
        .target_dep("only_newer", "1.0", "cfg(version(\"1.87\"))")
        .publish();

    let p = project()
        .file(
            "Cargo.toml",
            r#"
                [package]
                name = "foo"
                edition = "2024"

                [dependencies]
                bar = "1.0"
            "#,
        )
        .file("src/lib.rs", "")
        .build();

    p.cargo("check")
        .with_stderr_data(str![[r#"
[UPDATING] `dummy-registry` index
[LOCKING] 2 packages to latest Rust [..] compatible versions
[DOWNLOADING] crates ...
[DOWNLOADED] only_newer v1.0.0 (registry `dummy-registry`)
[DOWNLOADED] bar v1.0.0 (registry `dummy-registry`)
[CHECKING] only_newer v1.0.0
[CHECKING] bar v1.0.0
[CHECKING] foo v0.0.0 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

"#]])
        .run();

    p.cargo("tree")
        .with_stdout_data(str![[r#"
foo v0.0.0 ([ROOT]/foo)
└── bar v1.0.0
    └── only_newer v1.0.0

"#]])
        .run();
}

#[cargo_test]
fn cfg_version_from_index_in_future() {
    // Checks that cfg(version) for a version from the future is ignored.
    Package::new("only_newer", "1.0.0").publish();
    Package::new("bar", "1.0.0")
        .file("src/lib.rs", "")
        .target_dep("only_newer", "1.0", "cfg(version(\"1.2345\"))")
        .publish();

    let p = project()
        .file(
            "Cargo.toml",
            r#"
                [package]
                name = "foo"
                edition = "2024"

                [dependencies]
                bar = "1.0"
            "#,
        )
        .file("src/lib.rs", "")
        .build();

    p.cargo("tree")
        .with_stdout_data(str![[r#"
foo v0.0.0 ([ROOT]/foo)
└── bar v1.0.0

"#]])
        .run();
}

ehuss · 2025-05-26T15:41:58Z

tests/testsuite/cfg.rs

+error[E0463]: can't find crate for `b`
+ --> src/lib.rs:1:1
+  |
+1 | extern crate b;
+  | ^^^^^^^^^^^^^^^ can't find crate
+
+For more information about this error, try `rustc --explain E0463`.
+[ERROR] could not compile `foo` (lib) due to 1 previous error


We can't match on the exact output from rustc because if they change the output it would prevent this test from passing. I think something like the following should be close enough:

Suggested change

error[E0463]: can't find crate for `b`

--> src/lib.rs:1:1

|

1 | extern crate b;

| ^^^^^^^^^^^^^^^ can't find crate

For more information about this error, try `rustc --explain E0463`.

[ERROR] could not compile `foo` (lib) due to 1 previous error

error[E0463]: can't find crate for `b`

...

[ERROR] could not compile `foo` (lib) due to 1 previous error

epage · 2025-06-09T14:13:03Z

FYI a concern was raised on the new stabilization issue (rust-lang/rust#141766 (comment)) about what syntax is used for this.

rustbot added A-build-scripts Area: build.rs scripts A-cfg-expr Area: Platform cfg expressions labels May 16, 2025

mo8it changed the title ~~Start with cfg_version parsing~~ cfg_version May 16, 2025

weihanglo reviewed May 16, 2025

View reviewed changes

mo8it commented May 16, 2025

View reviewed changes

src/cargo/core/compiler/custom_build.rs Outdated Show resolved Hide resolved

epage reviewed May 16, 2025

View reviewed changes

crates/cargo-platform/src/cfg.rs Outdated Show resolved Hide resolved

epage reviewed May 16, 2025

View reviewed changes

crates/cargo-platform/src/cfg.rs Outdated Show resolved Hide resolved

epage reviewed May 16, 2025

View reviewed changes

crates/cargo-platform/src/cfg.rs Show resolved Hide resolved

epage reviewed May 16, 2025

View reviewed changes

src/cargo/core/compiler/custom_build.rs Outdated Show resolved Hide resolved

rustbot added the A-build-execution Area: anything dealing with executing the compiler label May 20, 2025

mo8it requested a review from epage May 20, 2025 17:59

mo8it marked this pull request as ready for review May 20, 2025 17:59

rustbot assigned ehuss May 20, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 20, 2025