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.

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

Strip features which is owned by dev dependencies only #12230

Closed

Conversation

eval-exec
Copy link
Contributor

@eval-exec eval-exec commented Jun 4, 2023

What does this PR try to resolve?

This PR want to close #12225

How should we test and review this PR?

I added a unit test.

Additional information

@rustbot
Copy link
Collaborator

rustbot commented Jun 4, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-interacts-with-crates.io Area: interaction with registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2023
@eval-exec eval-exec force-pushed the exec/fix-strip-dev-deps-features branch from 25be678 to 21c512d Compare June 4, 2023 09:58
@eval-exec eval-exec force-pushed the exec/fix-strip-dev-deps-features branch from 21c512d to d787d28 Compare June 4, 2023 10:03
@eval-exec eval-exec changed the title [WIP] Remove dev-dependencies only feature when publish Strip features which is owned by dev dependencies only Jun 4, 2023
@eval-exec eval-exec marked this pull request as ready for review June 4, 2023 10:04
@eval-exec eval-exec marked this pull request as draft June 4, 2023 10:20
@eval-exec
Copy link
Contributor Author

eval-exec commented Jun 4, 2023

I miss to strip features for cargo package. fixing it

@eval-exec eval-exec force-pushed the exec/fix-strip-dev-deps-features branch from d787d28 to fc3e016 Compare June 4, 2023 13:05
@rustbot rustbot added A-manifest Area: Cargo.toml issues Command-package labels Jun 4, 2023
@eval-exec eval-exec force-pushed the exec/fix-strip-dev-deps-features branch 3 times, most recently from c6a5927 to dad53c5 Compare June 4, 2023 13:14
@eval-exec eval-exec marked this pull request as ready for review June 4, 2023 13:15
@ehuss
Copy link
Contributor

ehuss commented Jun 4, 2023

@rustbot author
When you're ready for review, please use @rustbot ready.

Just taking a quick peak at the current code, it looks like it may be unconditionally stripping dev-dependency features. I believe they should only be stripped for dev-dependencies that don't specify a version (see is_version_specified). Also beware that dev-dependencies can also be in the target table. If you have any questions, please let us know.

@rustbot rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2023
@eval-exec eval-exec force-pushed the exec/fix-strip-dev-deps-features branch from dad53c5 to fa9d482 Compare June 10, 2023 00:33
@eval-exec eval-exec force-pushed the exec/fix-strip-dev-deps-features branch from fa9d482 to 5c08db1 Compare June 10, 2023 00:35
@eval-exec
Copy link
Contributor Author

eval-exec commented Jun 10, 2023

Also beware that dev-dependencies can also be in the target table.

What's the target table?


I am currently in the process of creating additional unit tests.

@eval-exec
Copy link
Contributor Author

eval-exec commented Jun 10, 2023

I believe they should only be stripped for dev-dependencies that don't specify a version (see is_version_specified)

Consider there is a Cargo.toml:

[package]
name = "foo"
version = "0.1.0"
authors = []
license = "MIT"
description = "foo"
documentation = "foo"
homepage = "foo"
repository = "foo"

[dependencies]

[dev-dependencies]
byte-unit = {version = "4.0.19", features = ["alloc"]}

[features]
foo_feature = ["byte-unit/alloc"]

Do you agree that cargo package and cargo publish should strip foo_features = ["byte-unit/alloc"] to foo_features = []?

Cc. @Morganamilo

@ehuss
Copy link
Contributor

ehuss commented Jun 10, 2023

What's the target table?

The [target] table is used for specifying platform-specific dependencies. It has its own set of dependencies, dev-dependencies, and build-dependencies. See TomlManifest.target.

Do you agree that cargo package and cargo publish should strip foo_features = ["byte-unit/alloc"] to foo_features = []?

No. byte-unit is still a valid dependency. For example, crater is a tool the rust-lang org uses to download all crates from crates.io, and it needs dev-dependencies in or to run tests. In some situations, the features may need to be kept. It is otherwise harmless to keep them there.

It only really needs to strip the features when the dependency is completely removed via the is_version_specified filter. Also, it needs to consider if the dependency is specified in both dev-dependencies and one of the other dependency tables.

@bors
Copy link
Contributor

bors commented Jun 24, 2023

☔ The latest upstream changes (presumably #12290) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss
Copy link
Contributor

ehuss commented Oct 14, 2023

@eval-exec Just checking in if you are still interested in working on this, or if you have any questions.

@ehuss
Copy link
Contributor

ehuss commented Dec 4, 2023

I'm going to close due to inactivity. If you are interested in continuing on this, feel free to reopen (just beware that due to a quirk in GitHub, if you push to your branch before reopening that you'll need to open a new PR instead).
Thanks for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-interacts-with-crates.io Area: interaction with registries A-manifest Area: Cargo.toml issues Command-package S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cargo should strip features of dev dependencies
4 participants