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

refactor: New variant FeaturesFor::ArtifactDep #11184

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented Oct 7, 2022

What does this PR try to resolve?

As the existence of FeaturesFor is acting as a key that discriminate
activated features for the same package with different circumstances.
Artifact dependencies deserve its own variant and it makes things clear to me.

  • NormalOrDevOrArtifactTarget(None) -> NormalOrDev
  • NormalOrDevOrArtifactTarget(Some(CompileTarget)) -> ArtifactDep(CompileTarget)

How should we test and review this PR?

Shouldn't have any regression. No functionality change in this pull request.

Additional information

This refactor doesn't break things, though it might be inaccurate to reflect how Cargo unifies features across normal and artifact dependencies under the same target.

From RFC 3028:

Cargo will not unify features across dependencies for different targets. One dependency tree may have both ordinary dependencies and artifact dependencies on the same crate, with different features for the ordinary dependency and for artifact depenencies for different targets.

It's not immediately clear to me which target it refers to in this paragraph. Assumed it is talking about target-triple, then something might be wrong in the current implementation as well as after this refactor. As I see it, NormalOrDevOrArtifactTarget(None) and NormalOrDevOrArtifactTarget(Some(…)) never get unified, even they are built for the same target. It seems that RFC doesn't tell whether Cargo should unify them. Should it?

Anyhow, this patch still gets the current code a bit better to read.

@rust-highfive
Copy link

r? @epage

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 7, 2022
@weihanglo weihanglo force-pushed the features2-refactor branch 2 times, most recently from 5d28121 to 5ea964a Compare October 7, 2022 09:55
@epage
Copy link
Contributor

epage commented Oct 7, 2022

This refactor doesn't break things, though it might be inaccurate to reflect how Cargo unifies features across normal and artifact dependencies under the same target.

I think someone with more background should then give this a look

r? @ehuss

@rust-highfive rust-highfive assigned ehuss and unassigned epage Oct 7, 2022
As the existence of `FeaturesFor` is acting as a key that discriminate
activated features for the same package with different circumstances.
Artifact dependencies deserve its own variant and it makes things clear to me.

- `NormalOrDevOrArtifactTarget(None)` ->  `NormalOrDev`
- `NormalOrDevOrArtifactTarget(Some(Target))` -> `ArtifactDep(Target)`
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@ehuss
Copy link
Contributor

ehuss commented Oct 12, 2022

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 12, 2022

📌 Commit cc204e8 has been approved by ehuss

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 Oct 12, 2022
@bors
Copy link
Collaborator

bors commented Oct 12, 2022

⌛ Testing commit cc204e8 with merge 953a630...

@bors
Copy link
Collaborator

bors commented Oct 12, 2022

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 953a630 to master...

@bors bors merged commit 953a630 into rust-lang:master Oct 12, 2022
@weihanglo weihanglo deleted the features2-refactor branch October 12, 2022 23:54
Dylan-DPC pushed a commit to Dylan-DPC/rust that referenced this pull request Oct 15, 2022
12 commits in b8f30cb23c4e5f20854a4f683325782b7cff9837..b332991a57c9d055f1864de1eed93e2178d49440
2022-10-10 19:16:06 +0000 to 2022-10-13 22:05:28 +0000
- Differentiate the warning when an alias (built-in or user-defined) shadows an external subcommand (rust-lang/cargo#11170)
- chore: Update tests for latest clap (rust-lang/cargo#11235)
- feat(publish): Support 'publish.timeout' config behind '-Zpublish-timeout' (rust-lang/cargo#11230)
- Add missing edition (rust-lang/cargo#11231)
- doc(profiles): add module level doc (rust-lang/cargo#11219)
- refactor(publish): Clarify which SourceId is being used (rust-lang/cargo#11216)
- Add new SourceKind::SparseRegistry to differentiate sparse registries (rust-lang/cargo#11209)
- Fix deadlock when build scripts are waiting for input on stdin (rust-lang/cargo#11205)
- refactor: New variant `FeaturesFor::ArtifactDep` (rust-lang/cargo#11184)
- Fix rustdoc warning about unclosed HTML tag (rust-lang/cargo#11221)
- refactor(tests): Prepare for wait-for-publish test changes (rust-lang/cargo#11210)
- Add configuration option for controlling crates.io protocol (rust-lang/cargo#11215)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 15, 2022
Update cargo

12 commits in b8f30cb23c4e5f20854a4f683325782b7cff9837..b332991a57c9d055f1864de1eed93e2178d49440 2022-10-10 19:16:06 +0000 to 2022-10-13 22:05:28 +0000
- Differentiate the warning when an alias (built-in or user-defined) shadows an external subcommand (rust-lang/cargo#11170)
- chore: Update tests for latest clap (rust-lang/cargo#11235)
- feat(publish): Support 'publish.timeout' config behind '-Zpublish-timeout' (rust-lang/cargo#11230)
- Add missing edition (rust-lang/cargo#11231)
- doc(profiles): add module level doc (rust-lang/cargo#11219)
- refactor(publish): Clarify which SourceId is being used (rust-lang/cargo#11216)
- Add new SourceKind::SparseRegistry to differentiate sparse registries (rust-lang/cargo#11209)
- Fix deadlock when build scripts are waiting for input on stdin (rust-lang/cargo#11205)
- refactor: New variant `FeaturesFor::ArtifactDep` (rust-lang/cargo#11184)
- Fix rustdoc warning about unclosed HTML tag (rust-lang/cargo#11221)
- refactor(tests): Prepare for wait-for-publish test changes (rust-lang/cargo#11210)
- Add configuration option for controlling crates.io protocol (rust-lang/cargo#11215)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 15, 2022
Update cargo

12 commits in b8f30cb23c4e5f20854a4f683325782b7cff9837..b332991a57c9d055f1864de1eed93e2178d49440 2022-10-10 19:16:06 +0000 to 2022-10-13 22:05:28 +0000
- Differentiate the warning when an alias (built-in or user-defined) shadows an external subcommand (rust-lang/cargo#11170)
- chore: Update tests for latest clap (rust-lang/cargo#11235)
- feat(publish): Support 'publish.timeout' config behind '-Zpublish-timeout' (rust-lang/cargo#11230)
- Add missing edition (rust-lang/cargo#11231)
- doc(profiles): add module level doc (rust-lang/cargo#11219)
- refactor(publish): Clarify which SourceId is being used (rust-lang/cargo#11216)
- Add new SourceKind::SparseRegistry to differentiate sparse registries (rust-lang/cargo#11209)
- Fix deadlock when build scripts are waiting for input on stdin (rust-lang/cargo#11205)
- refactor: New variant `FeaturesFor::ArtifactDep` (rust-lang/cargo#11184)
- Fix rustdoc warning about unclosed HTML tag (rust-lang/cargo#11221)
- refactor(tests): Prepare for wait-for-publish test changes (rust-lang/cargo#11210)
- Add configuration option for controlling crates.io protocol (rust-lang/cargo#11215)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 15, 2022
Update cargo

12 commits in b8f30cb23c4e5f20854a4f683325782b7cff9837..b332991a57c9d055f1864de1eed93e2178d49440 2022-10-10 19:16:06 +0000 to 2022-10-13 22:05:28 +0000
- Differentiate the warning when an alias (built-in or user-defined) shadows an external subcommand (rust-lang/cargo#11170)
- chore: Update tests for latest clap (rust-lang/cargo#11235)
- feat(publish): Support 'publish.timeout' config behind '-Zpublish-timeout' (rust-lang/cargo#11230)
- Add missing edition (rust-lang/cargo#11231)
- doc(profiles): add module level doc (rust-lang/cargo#11219)
- refactor(publish): Clarify which SourceId is being used (rust-lang/cargo#11216)
- Add new SourceKind::SparseRegistry to differentiate sparse registries (rust-lang/cargo#11209)
- Fix deadlock when build scripts are waiting for input on stdin (rust-lang/cargo#11205)
- refactor: New variant `FeaturesFor::ArtifactDep` (rust-lang/cargo#11184)
- Fix rustdoc warning about unclosed HTML tag (rust-lang/cargo#11221)
- refactor(tests): Prepare for wait-for-publish test changes (rust-lang/cargo#11210)
- Add configuration option for controlling crates.io protocol (rust-lang/cargo#11215)
lyming2007 pushed a commit to lyming2007/rust that referenced this pull request Oct 21, 2022
12 commits in b8f30cb23c4e5f20854a4f683325782b7cff9837..b332991a57c9d055f1864de1eed93e2178d49440
2022-10-10 19:16:06 +0000 to 2022-10-13 22:05:28 +0000
- Differentiate the warning when an alias (built-in or user-defined) shadows an external subcommand (rust-lang/cargo#11170)
- chore: Update tests for latest clap (rust-lang/cargo#11235)
- feat(publish): Support 'publish.timeout' config behind '-Zpublish-timeout' (rust-lang/cargo#11230)
- Add missing edition (rust-lang/cargo#11231)
- doc(profiles): add module level doc (rust-lang/cargo#11219)
- refactor(publish): Clarify which SourceId is being used (rust-lang/cargo#11216)
- Add new SourceKind::SparseRegistry to differentiate sparse registries (rust-lang/cargo#11209)
- Fix deadlock when build scripts are waiting for input on stdin (rust-lang/cargo#11205)
- refactor: New variant `FeaturesFor::ArtifactDep` (rust-lang/cargo#11184)
- Fix rustdoc warning about unclosed HTML tag (rust-lang/cargo#11221)
- refactor(tests): Prepare for wait-for-publish test changes (rust-lang/cargo#11210)
- Add configuration option for controlling crates.io protocol (rust-lang/cargo#11215)
@ehuss ehuss added this to the 1.66.0 milestone Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants