Skip to content

Conversation

Muscraft
Copy link
Member

@Muscraft Muscraft commented Apr 20, 2022

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

PRs in this RFC:

This is currently a draft

This PR focuses on adding initial support to allow running cargo add foo when a foo.workspace = true already exists.

Changes:

  • Added WorkspaceSource to allow for foo.workspace = true
  • Added MaybeWorkspace to allow returning a WorkspaceSource when trying to query for a dependency or trying to get a SourceId for that query.

Remaining implementation work for the RFC

r? @epage

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 20, 2022
@Muscraft Muscraft changed the title Rfc2906 part10 Allow running cargo add foo when a foo.workspace = true already exists Apr 20, 2022
@Muscraft
Copy link
Member Author

I accidentally had my commits from an earlier branch that hadn't been merged under this one. It should be fixed now.

Comment on lines 930 to 933
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unrelated to the source

Copy link
Member Author

Choose a reason for hiding this comment

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

Features and optional are unrelated to the source?

Copy link
Contributor

Choose a reason for hiding this comment

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

See

pub struct Dependency {
    /// The name of the dependency (as it is set in its `Cargo.toml` and known to crates.io)
    pub name: String,
    /// Whether the dependency is opted-in with a feature flag
    pub optional: Option<bool>,

    /// List of features to add (or None to keep features unchanged).
    pub features: Option<IndexSet<String>>,
    /// Whether default features are enabled
    pub default_features: Option<bool>,

    /// Where the dependency comes from
    pub source: Option<Source>,
    /// Non-default registry
    pub registry: Option<String>,

    /// If the dependency is renamed, this is the new name for the dependency
    /// as a string.  None if it is not renamed.
    pub rename: Option<String>,

    /// Features that are exposed by the dependency
    pub available_features: BTreeMap<String, Vec<String>>,
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah so we should make it so features from foo = { workspace = true, features = ["test"]} get put into the Dependency

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct

Copy link
Contributor

Choose a reason for hiding this comment

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

This will just print a bool. We should probably just print workspace

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this ever be false? Should we just have the presence of this assume true?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is up to you. I don't think it should be false ever since we check if it's false before we create it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just have this be a unit struct for now. We can always add fields later

Copy link
Contributor

Choose a reason for hiding this comment

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

We should delete all of the keys incompatible with workspace = true.

Copy link
Contributor

Choose a reason for hiding this comment

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

The other branches should remove "workspace"

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be more clear to say "{key}.workspace = false is unsupported"

Copy link
Contributor

Choose a reason for hiding this comment

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

I might go as far as an unreachable!("registry depedencies required, found a workspace dependency")

Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

unreachable!("path or git dependency expected, found workspace dependency");

Comment on lines 537 to 542
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need to be looking up the workspace's dependency and getting its query?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly, this was one area that I wasn't sure how we should be going about this. I know that this makes it so foo = { workspace = true, features = ["test"]} works.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have two feature lists

  • What the user specified
  • What the crate defines

This code is putting the user-specified features into the list of what the crate defines so anything the user enters will always be valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

#10581 will add a masquerade_as_nightly_cargo() function to Command.

Copy link
Member Author

Choose a reason for hiding this comment

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

I saw that and put this in until that PR is merged or we plan to merge this one and then have a cleanup PR that fixes it

@Muscraft Muscraft force-pushed the rfc2906-part10 branch 2 times, most recently from 46b90e7 to 730f69a Compare April 22, 2022 01:14
@Muscraft Muscraft marked this pull request as ready for review April 22, 2022 01:15
@Muscraft
Copy link
Member Author

Closing as #10606 includes this commit

@Muscraft Muscraft closed this Apr 27, 2022
bors added a commit that referenced this pull request Apr 28, 2022
Cargo add support for workspace inheritance

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

This PR adds all the required support for workspace inheritance within `cargo-add`. It was split up across a few different PRs as it required `snapbox` support from #10581 and a new `toml_edit` version from #10603. `@epage` and I decided to go ahead with this PR and add in some of the changes those PRs made. `@epage's` name on the commits is from helping to rewrite commits and some very minor additions.

Changes:
  - #10585
  - Muscraft#1
  - Muscraft#3
  - Muscraft#2
  - Muscraft#4

r? `@epage`
@Muscraft Muscraft deleted the rfc2906-part10 branch May 27, 2022 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants