fix(publish): Don't strip non-dev features #14325

epage · 2024-07-30T20:01:44Z

What does this PR try to resolve?

First, we added support for stripping of local-only dev-dependencies.
This was dual-implemented for Cargo.toml and Summary.

This left off stripping of dep/feature that reference dev-dependencies
(enabling features within dev-dependencies).
When we fixed this, we again dual-implemented it.

The Cargo.toml version was correct but the Summary version was
instead stripping too many features,
particularly features that reference renamed dependencies.
We didn't have tests for this case and it wasn't caught earlier because
crates.io re-generates the Summary from Cargo.toml, ignoring what we
post.
That makes this only show up with custom registries that trust what
Cargo posts.

Rather than fixing the Summary generation, I remove the
dual-implementation and instead generate the Summary from the
published Cargo.toml.

As a side effect, this also fixed it so the index lists filenames according to their location in .crate, rather than their location in the source.

Fixes #14321

How should we test and review this PR?

Additional information

Unfortunately, we don't have access directly to the packaged
Cargo.toml.
It could be passed around and I originally did so, hoping to remove use
of the local Package.
However, the local Package is needed for things like reading the
README.
So I scaled back and isolate the change to only what needs it.
This also makes it easier for prepare_transmit callers.
Fully open to someone exploring removing this extra prepare_for_publish in the future.

See rust-lang#14321

We'll be doing a second `prepare_for_publish` call that doesn't need the filtering, we don't have access to the `included`, and we don't want the warnings duplicated.

First, we added support for stripping of local-only dev-dependencies. This was dual-implemented for `Cargo.toml` and `Summary`. This left off stripping of `dep/feature` that reference dev-dependencies (enabling features within dev-dependencies). When we fixed this, we again dual-implemented it. The `Cargo.toml` version was correct but the `Summary` version was instead stripping too many features, particularly features that reference renamed dependencies. We didn't have tests for this case and it wasn't caught earlier because crates.io re-generates the `Summary` from `Cargo.toml`, ignoring what we post. That makes this only show up with custom registries that trust what Cargo posts. Rather than fixing the `Summary` generation, I remove the dual-implementation and instead generate the `Summary` from the published `Cargo.toml`. Unfortunately, we don't have access directly to the packaged `Cargo.toml`. It could be passed around and I originally did so, hoping to remove use of the local `Package`. However, the local `Package` is needed for things like reading the `README`. So I scaled back and isolate the change to only what needs it. This also makes it easier for `prepare_transmit` callers. Fully open to someone exploring removing this extra `prepare_for_publish` in the future. Fixes rust-lang#14321

rustbot · 2024-07-30T20:01:49Z

r? @weihanglo

rustbot has assigned @weihanglo.
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

weihanglo

Thanks for the fix. Unsure if there is a better way to fix this, but this seems good enough. Just need some more comments :)

weihanglo · 2024-07-30T20:50:26Z

tests/testsuite/publish.rs

@@ -1652,13 +1655,15 @@ fn publish_dev_dep_stripping() {
                "target-normal-and-dev/cat",
                "optional-dep-feature/cat",
                "dep:optional-namespaced",
+                "dep:optional-renamed-namespaced10",


Should we cover more rename cases?

I've added

renamed-dev-only

optional-renamed-dep-feature

Oops, hadn't refreshed. I'll post a follow up PR

weihanglo · 2024-07-30T20:53:09Z

src/cargo/util/toml/mod.rs

@@ -2608,7 +2608,8 @@ fn prepare_toml_for_publish(
    package.workspace = None;
    if let Some(StringOrBool::String(path)) = &package.build {
        let path = paths::normalize_path(Path::new(path));
-        let build = if included.contains(&path) {
+        let included = included.map(|i| i.contains(&path)).unwrap_or(true);


Why defaulting to true?

If the caller hasn't specified what files are included in the package, we should act as if all are included. I feel like that question is a preface for asking for a comment but I'm having a hard time thinking of what to comment because the intent is so "obvious". Maybe its the curse of knowledge as I also wrote all of this code that is being modified.

I feel like it is the same problem that the intent of packaged_files is unclear. Basically it does some checks and emits warnings, and we want to suppress warnings, hence unwrap_or(true).

I don't think it really needs a code comment. Thanks for the clarification!

weihanglo · 2024-07-30T21:06:10Z

tests/testsuite/inheritable_workspace_fields.rs

          "links": null,
          "name": "bar",
          "readme": "README.md",
-          "readme_file": "../README.md",
+          "readme_file": "README.md",


This seems like a side effect that fixes another bug.

Yes, I considered tweaking things to not have this happen but it seemed more correct now.

weihanglo · 2024-07-30T22:04:17Z

src/cargo/util/toml/mod.rs

@@ -2554,7 +2554,7 @@ fn unused_dep_keys(
 pub fn prepare_for_publish(


It is pretty obscure what the included argument is for. Could we document this function as well as arguments to make it clear? Something like

Suggested change

pub fn prepare_for_publish(

/// Makes the given [`Package`] into a self-contained, publish-ready `Package`.

///

/// The `included` argument is for optionally checking if a given path in a field is reachable.

///

/// See below for more details on what will be processed:

///

/// * [`prepare_toml_for_publish`]

/// * [`to_real_manifest`]

pub fn prepare_for_publish(

to_real_manifest is just to reuse the Package / Summary generation logic. At that point, I don't feel like calling out prepare_toml_for_publish really gains much as its name is so tied to this and it immediately follows it (in code, in rustdoc side bar which is alphabetical, and in rustdocs items which are in code-order).

I'm trying a rename of included to see if that clarifies it. I feel weird saying too much more without also specifying all of the other behavior.

weihanglo · 2024-07-30T22:05:48Z

src/cargo/ops/registry/publish.rs

    local_pkg: &Package,
    registry_id: SourceId,
 ) -> CargoResult<NewCrate> {
-    let deps = local_pkg
+    let included = None; // don't filter build-targets


it is unclear why the argument affects build target filtering.

weihanglo · 2024-07-31T03:57:19Z

@bors r+

bors · 2024-07-31T03:57:22Z

📌 Commit 7c17862 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2024-07-31T03:58:30Z

⌛ Testing commit 7c17862 with merge d3e84f0...

bors · 2024-07-31T04:25:32Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing d3e84f0 to master...

test(publish): More dev-dep stripping cases This is a follow up to #14325

[beta-1.81] fix(publish): Don't strip non-dev features Beta backports * #14325 In order to make CI pass, the following PRs are also cherry-picked: * #14282 363ddd4

Update cargo 15 commits in 257b72b8adfb1f2aa9916cefca67285c21666276..fa646583675d7c140482bd906145c71b7fb4fc2b 2024-07-30 15:08:27 +0000 to 2024-08-02 16:08:06 +0000 - refactor(toml): Rename 'resolved' to 'normalized' (rust-lang/cargo#14342) - faq: rephrase offline usage. (rust-lang/cargo#14336) - docs(unstable): Improve nightly link (rust-lang/cargo#14344) - Fix a typo in 1.81 changes (rust-lang/cargo#14343) - Change tests to support `rustc` wording changes (rust-lang/cargo#14341) - chore(deps): update rust crate windows-sys to 0.59 (rust-lang/cargo#14335) - chore(deps): update rust crate gix to 0.64.0 (rust-lang/cargo#14332) - chore(deps): update compatible (rust-lang/cargo#14331) - chore(deps): update rust crate rusqlite to 0.32.0 (rust-lang/cargo#14334) - fix: also build manpage for cargo.md (rust-lang/cargo#14339) - fix(config): Adjust MSRV resolve config field name / values (rust-lang/cargo#14296) - fix(toml): Resolve regression from toml_edit 0.22.18 (rust-lang/cargo#14329) - test(publish): More dev-dep stripping cases (rust-lang/cargo#14327) - Use gmake on AIX (rust-lang/cargo#14323) - fix(publish): Don't strip non-dev features (rust-lang/cargo#14325) r? ghost

epage added 6 commits July 30, 2024 13:38

test(publish): Consolidate dev-dep stripping tests

0ece9b9

test(publish): Add more dev-dep stripping regression cases

691c5a2

test(publish): Show bad dev-dep stripping

9e730ec

See rust-lang#14321

refactor(package): Make target filtering optional

53fe2b6

We'll be doing a second `prepare_for_publish` call that doesn't need the filtering, we don't have access to the `included`, and we don't want the warnings duplicated.

refactor(publish): Clarify we prepare_transmit using the local package

dc9014d

rustbot assigned weihanglo Jul 30, 2024

rustbot added A-interacts-with-crates.io Area: interaction with registries A-manifest Area: Cargo.toml issues Command-package Command-publish S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 30, 2024

epage mentioned this pull request Jul 30, 2024

Cargo 1.78+ incorrectly drops FeatureValues that match the implicit feature #14321

Closed

epage added the beta-nominated Nominated to backport to the beta branch. label Jul 30, 2024

weihanglo approved these changes Jul 30, 2024

View reviewed changes

epage added 2 commits July 30, 2024 20:49

refactor(toml): Clarify the 'included' parameter name

fb41fc3

docs(toml): Document prepare_for_publish

7c17862

weihanglo approved these changes Jul 31, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2024

bors merged commit d3e84f0 into rust-lang:master Jul 31, 2024
22 checks passed

epage mentioned this pull request Jul 31, 2024

test(publish): More dev-dep stripping cases #14327

Merged

bors added a commit that referenced this pull request Jul 31, 2024

Auto merge of #14327 - epage:14321-followup, r=weihanglo

6c9bccc

test(publish): More dev-dep stripping cases This is a follow up to #14325

epage mentioned this pull request Jul 31, 2024

[beta-1.81] fix(publish): Don't strip non-dev features #14328

Merged

weihanglo mentioned this pull request Aug 3, 2024

Update cargo rust-lang/rust#128576

Merged

rustbot added this to the 1.82.0 milestone Aug 3, 2024

pietroalbini mentioned this pull request Aug 4, 2024

Prepare Rust 1.80.1 point release rust-lang/rust#128635

Merged

This was referenced Aug 4, 2024

No workspace lints quinn-rs/quinn#1955

Merged

Cargo fails to add dependency if it is renamed, gated, and in private registry #14365

Closed

arlosi mentioned this pull request Aug 14, 2024

Cargo cannot find renamed dependency from crates-io in package from different registry #14399

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(publish): Don't strip non-dev features #14325

fix(publish): Don't strip non-dev features #14325

epage commented Jul 30, 2024 •

edited

Loading

rustbot commented Jul 30, 2024

weihanglo left a comment

weihanglo Jul 30, 2024

epage Jul 31, 2024

epage Jul 31, 2024

weihanglo Jul 30, 2024

epage Jul 31, 2024

weihanglo Jul 31, 2024

weihanglo Jul 30, 2024

epage Jul 31, 2024

weihanglo Jul 30, 2024

epage Jul 31, 2024

weihanglo Jul 30, 2024

weihanglo commented Jul 31, 2024

bors commented Jul 31, 2024

bors commented Jul 31, 2024

bors commented Jul 31, 2024

		@@ -2554,7 +2554,7 @@ fn unused_dep_keys(
		pub fn prepare_for_publish(

-pub fn prepare_for_publish(
+/// Makes the given [`Package`] into a self-contained, publish-ready `Package`.
+///
+/// The `included` argument is for optionally checking if a given path in a field is reachable.
+///
+/// See below for more details on what will be processed:
+///
+/// * [`prepare_toml_for_publish`]
+/// * [`to_real_manifest`]
+pub fn prepare_for_publish(

fix(publish): Don't strip non-dev features #14325

fix(publish): Don't strip non-dev features #14325

Conversation

epage commented Jul 30, 2024 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Jul 30, 2024

weihanglo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weihanglo commented Jul 31, 2024

bors commented Jul 31, 2024

bors commented Jul 31, 2024

bors commented Jul 31, 2024

epage commented Jul 30, 2024 •

edited

Loading