Normalization of Cargo.toml can break crates with deps. that read Cargo.toml

[ErichDonGubler]

Problem

Commentary surrounding items in a crate's Cargo.toml is stripped when cargo package or cargo vendor are used to vendor in said crate. For most crates, this does not cause a problem. However, this can cause build regressions or changes for crates with dependencies that use Cargo.toml's contents. As a concrete example of this happening, document-features uses Cargo.toml to emit documentation for features. However, a few users of crates that consume document-features indirectly have found that cargo vendor copies of crates run into build regressions (see also slint-ui/document-features#20), because feature commentary is missing from the "normalized" Cargo.toml manifests.

Steps

1. First, create a "dependency" crate that depends on document_features 0.2.7, uses its document_features!(…) macro, and which has at least one "documented" dependency according to document-features' convention. For the purposes of these steps, it is important that this crate be readily vendor-able via cargo vendor, hence a Git repo will be recommended from this point forward.

   • For convenience, one can just use the provided minimum reproducible example "dependency" crate at https://gitlab.com/erichdongubler-mre/2023-12-cargo-toml-normalization-breakage.

     • Cargo.toml:

     [package]
     name = "lib"
     version = "0.1.0"
     edition = "2021"
     
     [features]
     ## ermehgersh so beautiful
     my_awesome_feature = []
     
     [dependencies]
     document-features = "0.2.7"

     • src/lib.rs:

     //! # Features
     //!
     #![doc = document_features::document_features!(feature_label = r#"<span class="stab portability"><code>{feature}</code></span>"#)]

2. Next, create the "consumer" crate, which depends on the "dependency" crate

   • For convenience, one can use this Cargo.toml file as a replacement for a crate otherwise generated with cargo new --lib …; continuing with the lib crate as defined above:

      [package]
      name = "consumer"
      version = "0.1.0"
      edition = "2021"
      
      [dependencies]
      lib = { git = "https://gitlab.com/erichdongubler-mre/2023-12-cargo-toml-normalization-breakage", rev = "77d3dacdc1dc0a47bbf63f2f56a48c98c7ae6cc4" }
      # # Another crate that exhibits the same behavior.
      # wgpu = { git = "https://github.com/gfx-rs/wgpu", rev = "090f2f757c2d21a36c3bec1c0f43dc56aa9b9dd3"}

3. Run compiler checks against the "consumer" crate with cargo check. Observe that it succeeds.

4. Vendor in the "dependency" crate as a dependency of the "consumer" crate. Continuing the above examples, one can use a .cargo/config.toml file in the "consumer" file tree like the following:

   [source.crates-io]
   replace-with = "vendored-sources"
   
   [source."git+https://gitlab.com/erichdongubler-mre/2023-12-cargo-toml-normalization-breakage?rev=77d3dacdc1dc0a47bbf63f2f56a48c98c7ae6cc4"]
   git = "https://gitlab.com/erichdongubler-mre/2023-12-cargo-toml-normalization-breakage"
   rev = "77d3dacdc1dc0a47bbf63f2f56a48c98c7ae6cc4"
   replace-with = "vendored-sources"
   
   [source.vendored-sources]
   directory = "vendor"

5. As in step 3, run compiler checks against the "consumer" crate with cargo check. Observe that it fails with an error like the following:

   $ cargo c
       Checking lib v0.1.0 (https://gitlab.com/erichdongubler-mre/2023-12-cargo-toml-normalization-breakage?rev=77d3dacdc1dc0a47bbf63f2f56a48c98c7ae6cc4#77d3dacd)
   error: Could not find documented features in Cargo.toml
    --> /Users/mozilla/Downloads/repro-document-features-breakage/consumer/vendor/lib/src/lib.rs:3:10
     |
   3 | ... = document_features::document_features!(feature_label = r#"<span class="stab portability"><code>{feature}</code></span>"...
     |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: this error originates in the macro `document_features::document_features` (in Nightly builds, run with -Z macro-backtrace for more info)
   
   error: could not compile `lib` (lib) due to previous error
   

Possible Solution(s)

My suggestion is to preserve commentary for different sections as possible. In the concrete case of offered reproduction steps, preserving the contents of commentary in the [features] section would fix the issue. I don't know what to recommend in the cases where the preservation of original Cargo.toml structure cannot be achieved, though.

Alternatively, Cargo can change its documentation to disclaim that commentary in Cargo.toml may be purged with cargo package or cargo vendor, and that crate authors should not rely upon it. This would break the ecosystem that currently exists, but would provide a clear guarantee going forward.

Notes

Related issue for document-features as a "dependency" crate: slint-ui/document-features#20

Version

cargo 1.74.1 (ecb9851af 2023-10-18)
release: 1.74.1
commit-hash: ecb9851afd3095e988daaa35a48bc7f3cb748e04
commit-date: 2023-10-18
host: aarch64-apple-darwin
libgit2: 1.7.1 (sys:0.18.0 vendored)
libcurl: 8.1.2 (sys:0.4.68+curl-8.4.0 system ssl:(SecureTransport) LibreSSL/3.3.6)
ssl: OpenSSL 1.1.1u  30 May 2023
os: Mac OS 14.1.1 [64-bit]

[epage]

With my super-stickler hat on, comments in TOML are not syntactically meaningful. document-features takes inspiration from Rust but those comments are somewhat meaningful. Long term, our focus should probably be on #4956 and the associated RFC.

Without that hat on, I still feel this is likely in document-features camp. The only regression should be the lack of features; not an error. Besides not-failing, document-features could instead check if Cargo.toml.orig exists and read that.

[ErichDonGubler]

@epage:

… Long term, our focus should probably be on #4956 and the associated RFC.

Having a "blessed" way to write feature descriptions definitely sounds like an all-around better solution!

Without that hat on, I still feel this is likely in document-features camp. The only regression should be the lack of features; not an error. Besides not-failing, document-features could instead check if Cargo.toml.orig exists and read that.

Agreed; I have already encouraged document-features' maintainership to eliminate this error case. CC @ogoffart.

[ogoffart]

document-features already fallbacks to Cargo.toml.orig. I think the problem happens when the dependencies end up vendored. Not sure what tools their using that removes the Cargo.toml.orig.

But I agree it was too strong to throw an error from the document-features macro.

[epage]

As this is relying on non-syntactic content and the cost to paper over this to help with these applications would be great, I'm going to propose to the rest of the cargo team that we close this.

[weihanglo]

Agree, and since Cargo.toml.orig is there, plugins should have alternative way to get the original manifest file.

[ErichDonGubler]

AFAIK, neither Mozilla nor my reproducible example in the OP change files; it seems that cargo vendor is not emitting Cargo.toml.orig. Is that expected?

[weihanglo]

Hmm… I didn't look closer.

Seems like cargo vendor intentionally skips Cargo.toml.orig since cargo-vendor was imported into cargo from a community plugin.

cargo/src/cargo/ops/vendor.rs

Lines 341 to 348 in 363a2d1

	// Skip patch-style orig/rej files. Published crates on crates.io
	// have `Cargo.toml.orig` which we don't want to use here and
	// otherwise these are rarely used as part of the build process.
	Some(filename) => {
	if filename.ends_with(".orig") || filename.ends_with(".rej") {
	continue;
	}
	}

We may want to reconsider copying Cargo.toml.orig over if it helps people to read the original source and maintain the integrity. However, that could bring churns to people finding that their git status is dirty after running cargo vendor.