Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: strip feature dep when dep is dev dep #13518

Merged
merged 2 commits into from
Mar 14, 2024
Merged

fix: strip feature dep when dep is dev dep #13518

merged 2 commits into from
Mar 14, 2024

Conversation

baby230211
Copy link
Contributor

@baby230211 baby230211 commented Mar 2, 2024 •
edited
Loading

What does this PR try to resolve?

This change aims to strip features dependencies without a version key to be published.
If a dev-dependency is missing the version, it will be stripped from the packaged manifest.

The features table may contains the deps in following places.

  • Target
    • normal
    • dev
    • build
    • normal-and-dev
  • normal
  • dev
  • build
  • normal-and-dev

How should we test and review this PR?

See the initial commit, it shows current behavior that will cause error when feature has deps that point to dev_dep and doesn't have a version specified.

Title orignal toml published toml
Before feature = ["dev-dep/feature"]
[dev-dependencies]
dev-dep = { .., features: ["feature"] }		 feature = ["dev-dep/feature"]
[dev-dependencies]
dev-dep = { .., features: ["feature"] }
After feature = ["dev-dep/feature"]
[dev-dependencies]
dev-dep = { .., features: ["feature"] }		 feature = []
[dev-dependencies] ```

Fix: #12225

@rustbot
Copy link
Collaborator

rustbot commented Mar 2, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-manifest Area: Cargo.toml issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 2, 2024
@baby230211 baby230211 changed the title fix: strip feature dep when dep is dev dep [WIP] fix: strip feature dep when dep is dev dep Mar 2, 2024
@weihanglo
Copy link
Member

Thanks for the contribution!

Not sure if @linyihai has started on this, though I believe they are willing to help :)
One reminder is that even this is not ready for review, it would be lovely to have a PR description for reviewers to understand what's going on.

Since this PR is marked as draft, so assuming you have some questions. Feel free to ask anything here or on Zulip.

@weihanglo
Copy link
Member

Oh you've just updated the PR description 👍🏾

weihanglo
weihanglo reviewed Mar 3, 2024
View reviewed changes
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
@linyihai

This comment was marked as resolved.

Sign in to view
@rustbot rustbot added A-interacts-with-crates.io Area: interaction with registries Command-publish labels Mar 4, 2024
@epage
Copy link
Contributor

epage commented Mar 4, 2024

When this is ready for review, I'd recommend reworking the commits

  • An initial commit should exist that has passing tests, showing the current behavior
  • A follow up commit should change the behavior and update the tests so they still pass
    • This helps show we are testing the right thing and makes the change in behavior very clear
  • Any refactorings along the way should be separate commits

@baby230211
Copy link
Contributor Author

When this is ready for review, I'd recommend reworking the commits

  • An initial commit should exist that has passing tests, showing the current behavior

  • A follow up commit should change the behavior and update the tests so they still pass

    • This helps show we are testing the right thing and makes the change in behavior very clear

  • Any refactorings along the way should be separate commits

Sure. i'll start working on commits.

@baby230211 baby230211 force-pushed the fix/strip-feature-dev-dep branch 3 times, most recently from c87376e to 460dfc5 Compare March 6, 2024 00:20
epage
epage reviewed Mar 6, 2024
View reviewed changes
tests/testsuite/publish.rs Outdated Show resolved Hide resolved
epage
epage reviewed Mar 8, 2024
View reviewed changes
tests/testsuite/publish.rs Outdated Show resolved Hide resolved
epage
epage reviewed Mar 8, 2024
View reviewed changes
tests/testsuite/publish.rs Outdated Show resolved Hide resolved
@baby230211 baby230211 force-pushed the fix/strip-feature-dev-dep branch 2 times, most recently from dc2f54c to 7981af5 Compare March 9, 2024 14:52
@baby230211 baby230211 changed the title [WIP] fix: strip feature dep when dep is dev dep fix: strip feature dep when dep is dev dep Mar 9, 2024
@baby230211 baby230211 marked this pull request as ready for review March 9, 2024 15:42
epage
epage reviewed Mar 12, 2024
View reviewed changes
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
epage
epage reviewed Mar 12, 2024
View reviewed changes
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
epage
epage reviewed Mar 12, 2024
View reviewed changes
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
epage
epage reviewed Mar 12, 2024
View reviewed changes
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
epage
epage reviewed Mar 12, 2024
View reviewed changes
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
epage
epage reviewed Mar 13, 2024
View reviewed changes
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
epage
epage reviewed Mar 13, 2024
View reviewed changes
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented Mar 14, 2024

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Mar 14, 2024

📌 Commit 75130eb has been approved by epage

It is now in the queue for this repository.

@bors 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 Mar 14, 2024
@bors
Copy link
Contributor

bors commented Mar 14, 2024

⌛ Testing commit 75130eb with merge 403fbe2...

@bors
Copy link
Contributor

bors commented Mar 14, 2024

☀️ Test successful - checks-actions
Approved by: epage
Pushing 403fbe2 to master...

@bors bors merged commit 403fbe2 into rust-lang:master Mar 14, 2024
21 checks passed
@weihanglo weihanglo mentioned this pull request Mar 15, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 16, 2024
@bors
Auto merge of rust-lang#122575 - weihanglo:update-cargo, r=weihanglo
05a2be3 
Update cargo

6 commits in 7065f0ef4aa267a7455e1c478b5ccacb7baea59c..2fe739fcf16c5bf8c2064ab9d357f4a0e6c8539b
2024-03-12 13:25:15 +0000 to 2024-03-15 21:39:18 +0000
- feat: Add 'open-namespaces' feature (rust-lang/cargo#13591)
- refactor: Expose source/spans to Manifest for emitting lints (rust-lang/cargo#13593)
- feat(tree): Control `--charset` via auto-detecting config value (rust-lang/cargo#13337)
- refactor(toml): Flatten manifest parsing (rust-lang/cargo#13589)
- fix: strip feature dep when dep is dev dep (rust-lang/cargo#13518)
- fix(ci): bump check error when PR is behind master (rust-lang/cargo#13581)

r? ghost
@rustbot rustbot added this to the 1.78.0 milestone Mar 16, 2024
@epage epage mentioned this pull request Jul 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo left review comments

@epage epage epage left review comments

Assignees

@epage epage

Labels
A-interacts-with-crates.io Area: interaction with registries A-manifest Area: Cargo.toml issues Command-publish S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.78.0
Development

Successfully merging this pull request may close these issues.

Cargo should strip features of dev dependencies
6 participants
@baby230211 @rustbot @weihanglo @linyihai @epage @bors