Cargo 1.78+ incorrectly drops FeatureValues that match the implicit feature

[NiklasRosenstein]

Problem

Cargo 1.78 and 1.79 drops FeatureValues that match the default feature, resulting in an invalid feature map that later causes Cargo to ignore the version when parsing the index in the slow path of Summaries::parse() due to build_feature_map() returning an error.

Example error message:

   0.378811004s TRACE cargo::sources::registry::index: json parsed registry hs-clarity-resource-locator-rust/4.2.4
   0.381279937s  INFO cargo::sources::registry::index: failed to parse "hs/-c/hs-clarity-resource-locator-rust" registry package: optional dependency `tonic_010` is not included in any feature
Make sure that `dep:tonic_010` is included in one of features in the [features] table.

The corresponding Cargo.toml looks like this:

[package]
name = "hs-clarity-resource-locator-rust"
version = "0.1.0"
edition = "2021"

[features]
tonic_010 = ["dep:tonic_010"]

[dependencies]
tonic_010 = { package = "tonic", version = "0.10", optional = true }

❌ The error occurs because when this crate is published by Cargo 1.78 or 1.79, the feature map is broken:

{
  // ...
  "features": {
    "tonic_010": []
  }
}

✅ When published with Cargo 1.76 or 1.77 instead, you get:

{
  // ...
  "features": {
    "tonic_010": [
      "dep:tonic_010"
    ]
  }
}

✅ When removing the [features] table from the Cargo.toml, you get:

{
  // ...
  "features": {}
  }
}

My hypothesis is that Cargo does some "cleaning" of the features map, maybe attempting to remove features that would be implied anyway, but it results in a broken map.

Steps

1. Create a new crate with a Cargo.toml akin to the one shown above
2. Publish it to a registry
3. Inspect the published version's feature map

More elaborate local testing:

1. Step (1) from above
2. Run the sparsereg.py from below to record a newcrate.json when publishing
3. Publish the crate to sparse+http://localhost:8000/cargo/foorepo/index/
4. Apply 0001-Add-cargo-build-feature-map.rs.patch
5. cargo run --bin cargo-build-feature-map -- newcrate.json

Result

❯ cargo run --bin cargo-build-feature-map -- ~/Desktop/newcrate.json 
   Compiling cargo v0.83.0 (/home/coder/git/rust-lang/cargo)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 11.43s
     Running `target/debug/cargo-build-feature-map /home/coder/git/niklas/Desktop/newcrate.json`
{"name":"hs-clarity-resource-locator-rust","vers":"0.1.0","deps":[{"name":"tonic_010","optional":true,"explicit_name_in_toml":null}],"features":{"tonic_010":[]}}
Error: optional dependency `tonic_010` is not included in any feature
Make sure that `dep:tonic_010` is included in one of features in the [features] table.

Additional files

sparsereg.py

"""
Implements the basic API endpoints for a Cargo sparse registry to perform a `cargo publish`.
Records the received crate's metadata in a `newcrate.json` file in the CWD.

Dependencies:

- fastapi

Run:

    $ fastapi run sparsereg.py
"""

import struct
from typing import Any
from fastapi import FastAPI, Request


app = FastAPI()


@app.get("/cargo/{repository}/index/config.json")
def config_json(repository: str) -> dict[str, str]:
    return {
        "dl": "http://localhost:8000/cargo/v1/crates",
        "api": f"http://localhost:8000/cargo/{repository}",
    }


@app.put("/cargo/{repository}/api/v1/crates/new")
async def new_crate(repository: str, request: Request) -> dict[str, Any]:
    body = await request.body()
    length = struct.unpack("I", body[:4])[0]
    with open("newcrate.json", "wb") as fp:
        fp.write(body[4 : length + 4])
    return {}

0001-Add-cargo-build-feature-map.rs.patch

From 1169b45dcb59e503f980b926bda9a89b3fad51e2 Mon Sep 17 00:00:00 2001
From: Niklas Rosenstein <[email protected]>
Date: Tue, 30 Jul 2024 07:04:27 +0000
Subject: [PATCH 1/1] Add cargo-build-feature-map.rs

---
 Cargo.toml                         |  5 ++
 src/bin/cargo-build-feature-map.rs | 77 ++++++++++++++++++++++++++++++
 src/cargo/core/summary.rs          |  2 +-
 3 files changed, 83 insertions(+), 1 deletion(-)
 create mode 100644 src/bin/cargo-build-feature-map.rs

diff --git a/Cargo.toml b/Cargo.toml
index 77e7f9092..c9cf20eb4 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -251,6 +251,11 @@ name = "cargo"
 test = false
 doc = false
 
+[[bin]]
+name = "cargo-build-feature-map"
+test = false
+doc = false
+
 [features]
 vendored-openssl = ["openssl/vendored"]
 vendored-libgit2 = ["libgit2-sys/vendored"]
diff --git a/src/bin/cargo-build-feature-map.rs b/src/bin/cargo-build-feature-map.rs
new file mode 100644
index 000000000..48e9506a4
--- /dev/null
+++ b/src/bin/cargo-build-feature-map.rs
@@ -0,0 +1,77 @@
+use std::collections::BTreeMap;
+use std::error::Error;
+
+use cargo::core::summary::build_feature_map;
+use cargo::util::interning::InternedString;
+use itertools::Itertools;
+use serde::{Deserialize, Serialize};
+
+#[derive(Clone, Deserialize, Serialize)]
+struct PackageDep {
+    name: String,
+    optional: Option<bool>,
+
+    // For metadata from the Publish API, this contains the actual name in the TOML file (the alias).
+    // In the Sparse Registry Index API, the `name` field is already the actual name in the TOML.
+    explicit_name_in_toml: Option<String>,
+}
+
+impl PackageDep {
+    fn normalize(self) -> PackageDep {
+        if let Some(ref toml_name) = self.explicit_name_in_toml {
+            PackageDep {
+                name: toml_name.clone(),
+                optional: self.optional,
+                explicit_name_in_toml: None,
+            }
+        } else {
+            self.clone()
+        }
+    }
+}
+
+impl Into<cargo::core::Dependency> for PackageDep {
+    fn into(self) -> cargo::core::Dependency {
+        // Fake source ID, irrelevant.
+        let source_id = cargo::core::SourceId::from_url("git+https://foobar").unwrap();
+        let mut result =
+            cargo::core::Dependency::new_override(InternedString::new(&self.name), source_id);
+        result.set_optional(self.optional.unwrap_or(false));
+        result
+    }
+}
+
+#[derive(Deserialize, Serialize)]
+struct Package {
+    name: String,
+    vers: String,
+    deps: Vec<PackageDep>,
+    features: BTreeMap<InternedString, Vec<InternedString>>,
+}
+
+pub fn main() -> Result<(), Box<dyn Error>> {
+    let cmd = clap::Command::new("cargo-build-feature-map").args([clap::Arg::new("file")]);
+    let args = cmd.get_matches();
+
+    let file = args.get_one::<String>("file").unwrap();
+    let mut pkg: Package = serde_json::from_str(&std::fs::read_to_string(&file)?)?;
+
+    // Adjust for asymmetry between Registry API and Sparse registry API.
+    pkg.deps = pkg
+        .deps
+        .into_iter()
+        .map(PackageDep::normalize)
+        .collect_vec();
+
+    // Re-serialize the content. Makes it easier to compare one file with the other later.
+    println!("{}", serde_json::to_string(&pkg)?);
+
+    // Validates the feature map.
+    let feature_map = build_feature_map(
+        &pkg.features,
+        &pkg.deps.into_iter().map(Into::into).collect_vec(),
+    )?;
+    println!("{:?}", feature_map);
+
+    Ok(())
+}
diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs
index 424328fa6..05ff9f33c 100644
--- a/src/cargo/core/summary.rs
+++ b/src/cargo/core/summary.rs
@@ -186,7 +186,7 @@ const _: fn() = || {
 
 /// Checks features for errors, bailing out a CargoResult:Err if invalid,
 /// and creates FeatureValues for each feature.
-fn build_feature_map(
+pub fn build_feature_map(
     features: &BTreeMap<InternedString, Vec<InternedString>>,
     dependencies: &[Dependency],
 ) -> CargoResult<FeatureMap> {
-- 
2.45.2

Possible Solution(s)

No response

Notes

No response

Version

cargo 1.79.0 (ffa9cf99a 2024-06-03)
release: 1.79.0
commit-hash: ffa9cf99a594e59032757403d4c780b46dc2c43a
commit-date: 2024-06-03
host: x86_64-unknown-linux-gnu
libgit2: 1.7.2 (sys:0.18.3 vendored)
libcurl: 8.6.0-DEV (sys:0.4.72+curl-8.6.0 vendored ssl:OpenSSL/1.1.1w)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Ubuntu 22.4.0 (jammy) [64-bit]

[NiklasRosenstein]

I found this related issue after all: #10125

However it is described as a "entirely reasonable human error". I would classify this as a Cargo bug / breaking change, given that the Cargo.toml is semantically correct (even if not normalized given that one could rely on implied default features).

[epage]

Was hoping this was reproducible with cargo metadata (as most summary changes we've made recently should show up there) but it isn't

$ cargo +1.76 metadata | jq '.packages[0].features'
warning: please specify `--format-version` flag explicitly to avoid compatibility problems
{
  "tonic_010": [
    "dep:tonic_010"
  ]
}

$ cargo +1.77 metadata | jq '.packages[0].features'
warning: please specify `--format-version` flag explicitly to avoid compatibility problems
{
  "tonic_010": [
    "dep:tonic_010"
  ]
}

[epage]

On master,

cargo package Cargo.toml is good:

# THIS FILE IS AUTOMATICALLY GENERATED BY CARGO
#
# When uploading crates to the registry Cargo will automatically
# "normalize" Cargo.toml files for maximal compatibility
# with all versions of Cargo and also rewrite `path` dependencies
# to registry (e.g., crates.io) dependencies.
#
# If you are reading this file be aware that the original Cargo.toml
# will likely look very different (and much more reasonable).
# See Cargo.toml.orig for the original contents.

[package]
edition = "2021"
name = "hs-clarity-resource-locator-rust"
version = "0.1.0"
build = false
autobins = false
autoexamples = false
autotests = false
autobenches = false
readme = false

[[bin]]
name = "hs-clarity-resource-locator-rust"
path = "src/main.rs"

[dependencies.tonic_010]
version = "0.10"
optional = true
package = "tonic"

[features]
tonic_010 = ["dep:tonic_010"]

The Summary generated for the above is good:

        features: {
            "tonic_010": [
                Dep {
                    dep_name: "tonic_010",
                },
            ],
        },

But what we send to the registry is bad:

[src/cargo/ops/registry/publish.rs:473:5] &new_crate = NewCrate {
    name: "hs-clarity-resource-locator-rust",
    vers: "0.1.0",
    deps: [
        NewCrateDependency {
            optional: true,
            default_features: true,
            name: "tonic",
            features: [],
            version_req: "^0.10",
            target: None,
            kind: "normal",
            registry: None,
            explicit_name_in_toml: Some(
                "tonic_010",
            ),
            artifact: None,
            bindep_target: None,
            lib: false,
        },
    ],
    features: {
        "tonic_010": [],
    },
    authors: [],
    description: None,
    documentation: None,
    homepage: None,
    readme: None,
    readme_file: None,
    keywords: [],
    categories: [],
    license: None,
    license_file: None,
    repository: None,
    badges: {},
    links: None,
    rust_version: None,
}

@NiklasRosenstein are you using a custom registry? crates.io re-generates the summary, ignoring NewCrate. If this only happens for third-party registries, that might explain why this wasn't noticed before.

[jonhoo]

@epage Yep, can confirm this is happening for a third-party registry!

[epage]

#13518 is the only significant change I've seen in this area recently

[jonhoo]

@NiklasRosenstein May be a good idea to try a git bisect here that looks at the log output of what gets sent in NewCrate when running cargo publish against a simple HTTP receiver.

[jonhoo]

Alternatively may be easy to write a test case for https://github.com/rust-lang/cargo/blob/master/tests/testsuite/publish.rs and run git bisect with a script that will apply a diff that injects such a test and runs it.

[epage]

atm I am updating the tests to cover this and will then cherry pick to just before #13518.

[epage]

Confirmed that it is that PR. It is also specific to renaming.

[epage]

Posted a fix at #14325.

[NiklasRosenstein]

Hey @epage, thank you for fixing this. 🚀 It's going to land in Cargo 1.81, yes?

Speaking to how Cargo handles caches for crate indexes in general, I wonder if there is something that should be done with regards to error reporting and the caching behaviour.

As it stands today,

• When you are on an older Cargo version that doesn't support a particular version, the actual error is hidden in a very specific log message and it is not clear to the user why version resolution fails when clearly the registry indicates that the version you are looking to resolve exists.
• When you upgrade Cargo to a version that supports additional versions from the index that the previous version did not, you still can't resolve the respective versions until you either clear your local registry cache or the remote index is updated.

If I may suggest (maybe naively); Cargo could

• Maintain the original state of the index file in the cache and only validate versions on loading them back from disk
• Propagate information about versions that can't be considered because they are incompatible with the current Cargo version to the resolver or the error reporting

What do you think?

[weihanglo]

It's going to land in Cargo 1.81, yes?

We haven't done a beta-backport yet. Will do that in this week.

[epage]

This PR was against 1.82 but i have a cherry-pick to 1.81 ready to go when i get onto better internet.

I believe your first case is being tracked in #10623.

The second case shouldn't be a problem and I'd need reproduction steps.