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

Stabilize Cargo's weak-dep-features and namespaced-features. #3143

Merged
merged 3 commits into from
Jul 30, 2021

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jun 22, 2021

This proposes to stabilize the weak-dep-features and namespaced-features.

Rendered

@ehuss ehuss added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Jun 22, 2021
@pksunkara
Copy link

Looks good. What version of cargo/rust onwards would this be stabilized?

@ehuss ehuss force-pushed the cargo-weak-namespaced-features branch from 45164dd to 7914438 Compare June 22, 2021 20:30
@ehuss
Copy link
Contributor Author

ehuss commented Jun 22, 2021

It depends how long it takes for the RFC to be accepted. The team is on board with moving forward with this, so it just depends on what kind of feedback the RFC generates.

EDIT: It also depends on the final implementation side. crates.io will need to be updated, for example.

@ehuss
Copy link
Contributor Author

ehuss commented Jun 22, 2021

cc @rust-lang/crates-io, this will require some small changes on the crates.io side (which I'm fine with implementing).

This was pursued in [PR #9111](https://github.com/rust-lang/cargo/pull/9111), but it was considered not necessary.
[crates.io] is the only registry that can support older versions of Cargo.
Other registries that don't support the new syntax may reject publishing with the new syntax (if they perform validation), or they may accept it (if the don't validate), in which case it should just work.
The `"v"` field addition is only necessary for Cargo's older than 1.51, and most use cases of other registries are generally expected to have stricter control over which versions of Cargo are in use.
Copy link
Member

Choose a reason for hiding this comment

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

As I understand it, this description of the "v" field is backwards. Cargo older than 1.51 will process all index entries, even if that may result in errors or unexpected behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, that was backwards. I tried to fix it, though the explanation is still a little awkward.

@jtgeibel
Copy link
Member

Thanks for the ping @ehuss. The only concern I have is regarding support for older versions of cargo. As I understand it, it seems like this would split the ecosystem into 3 tiers:

  • Versions of cargo that support "v"=2 entries in the index and can work with all published crates.
  • Cargo from 1.51 until this is implemented, which will filter out the "v"=2 entries in the index and cannot see newer crates that use either of these features.
  • Cargo versions older than 1.51 will use all entries in the index but ignore the features2 field, resulting in errors or unexpected behavior.

For the last 2 bullet points, the reference section does state "older versions will ignore the "features2" field, allowing them to behave correctly assuming there is a Cargo.lock file (or the packages they need do not use the new syntax)." So maybe it is fine to run cargo update with a newer cargo and then old versions of cargo will work correctly, but I'm not really clear on how that works. Cargo.lock doesn't appear to directly record any data about which features are enabled for each crate, but it will drop a crate from the dependencies array if the crate is optional and not required by any enabled features. However, if the new dependency-name?/feature-name syntax is used, then it seems like a newer version of cargo will correctly include dependency-name in the dependency tree encoded in Cargo.lock such that older versions of cargo will probably include dependency-name in their build, but I don't see how the older version of cargo would know to correctly enable feature-name when building dependency-name.


To provide some data for versions of cargo currently in the wild, I've gone through the last 48 hours of request logs on crates.io (with the vast majority of requests being to the download endpoint). A few notes about the results:

  • The major version in the version string was bumped from 0 to 1 in 1.26, so 1.26 and 0.26 are reported in separate rows of the table. I haven't updated the chart to include this, because the data points aren't visible at that scale anyway.
  • It appears that rustc 1.55.0-nightly currently ships with cargo 1.54.0-nightly (44456677b 2021-06-12), so the 1.54 results include currently beta and nightly users. The few results for cargo 1.55 are likely to be locally compiled builds.
  • Blank user-agent strings (which are only allowed on the crate download endpoint) are way more prevalent than I expected, at nearly 31 million requests in the last 48 hours (nearly 3x all reported cargo versions). It is unclear if this is a bug, or if maybe there is a large org or CI provider sending these requests (via a 3rd party tool, or possibly by setting CARGO_HTTP_USER_AGENT or http.user-agent to the empty string). I did find Not all cargo requests contains User-Agent header cargo#8979 which could be related.

In the chart below, I've dropped the blank user agents so that the pattern of known cargo versions is more visible:

2021-06-23--requests-per-cargo-version-last-48-hours

As I understand it, 1.47 is the minimum supported rustc version for current Firefox stable. I know that Ubuntu has backported this version to LTS distros for this reason. Strangely, while the deb package is versions as 0.47.0-1~exp1ubuntu1~20.04.1, on an Ubuntu 20.04 LTS system I have, cargo -V reports cargo 1.46.0. I don't know what versions other distros ship, but I'm guessing distro packaging helps explain the spikes at 1.47 and 1.46.

Raw data table

Results of searching logs from the last 48 hours for strings of the form user_agent="cargo 1.xx.

User agent Request count
blank 30,954,588
1.55 548
1.54 3,748,333
1.53 3,215,703
1.52 1,191,330
1.51 534,757
1.50 149,988
1.49 115,636
1.48 59,168
1.47 486,106
1.46 1,178,520
1.45 351,759
1.44 226,001
1.43 77,520
1.42 141,152
1.41 62,193
1.40 79,282
1.39 77,918
1.38 36,395
1.37 9,059
1.36 29,513
1.35 2,704
1.34 3,808
1.33 1,871
1.32 19,918
1.31 1,499
1.30 310
1.29 543
1.28 426
1.27 74
1.26 77
0.26 2,091
0.25 30
0.24 725
0.23 0
0.22 44
0.21 8
0.20 232
0.19 1
0.18 0
0.17 0

@ehuss
Copy link
Contributor Author

ehuss commented Jun 23, 2021

Thanks for digging in!

I don't see how the older version of cargo would know to correctly enable feature-name when building dependency-name.

They won't work correctly.

In order to use a Cargo.lock that contains a package using the new syntax, you will need to use a version of Cargo where this RFC is stabilized.

Versions from 1.51 to whenever this is stabilized will still be allowed to run cargo update (or create new Cargo.lock files), but will not be able to update to packages that contain the new syntax.

Versions before 1.51 running cargo update may or may not work, per the scenarios outlined in the RFC.

It is recommended that package authors only start using the new syntax if they are OK with setting their minimum supported version to when this is stabilized. They may also consider only doing that as a semver-major release if they do not want to disrupt users from 1.50 or older.

Does that maybe help clarify?


It is quite surprising that so many requests are missing a user agent. It might be nice to understand where all that traffic is coming from. I'm not aware of any of the major CI providers stripping headers or proxying the registry.

I'm also a bit surprised that 1.54 has so much traffic. I would assume beta traffic is close to zero, and I thought there were significantly fewer nightly users than stable ones. I wonder if that is maybe skewed due to the incremental compilation issue driving people to nightly?

The nightly version number should update within a few days.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jun 25, 2021

I must admit I don't like the asymmetry of the weak deps syntax.

[features]
std = ["serde?/std"]

This might be instead written:

[features]
"and(serde, std)" = ["serde/std"]
# or we could do full `cfg(and(feature = "serde", feature = "std"))` like with `[target]`

which makes clear its not that "std is the strong one, serde is the weak one" or any silliness like that, but simply that "the combination of these features depends on these additional things", which puts serde and std on equal footing.

I think my syntax also more expressive. In this example, the downstream crate supports 2 axes of features, whereas the upstream crate only support a single coarser combined feature:

[features]
"and(some-dep, alloc, net)" = ["some-dep/bells-and-whistles"]

In this example, we simply want an additional (non-optional) dep to support the combination of features:

[features]
"and(alloc, net)" = ["dep:extra-thing-we-want"]

I don't think these are expressible with today's syntax, even though they are hardly different semantically.

If we would need my and(...) syntax for the slight generalization anyways, I soon as well just have that, and not bother with the ? syntax --- less to learn and no one gets tripped up about ? meaning something unrelated to its use in Rust.

@ehuss
Copy link
Contributor Author

ehuss commented Jul 2, 2021

@Ericson2314 Thanks for the feedback! Overall I think we would like to avoid getting into a more generalized expression syntax, just to keep it relatively simple and tractable. There will definitely be situations where this RFC won't be able to cover every use case. Do you have some real-world examples that could benefit from having more complex combinations? I'm having a hard time understanding exactly how that would be used.

Also, just to set your expectations, I think employing a large scale change in the way these conditional features are expressed is unlikely to make progress here on this RFC. This is intended to be a small-scale, incremental step forward which resolves a particular issue (where dep-name/feat-name doesn't work as most people want it to). More than this is beyond my capacity at this time.

@jplatte
Copy link
Contributor

jplatte commented Jul 2, 2021

real-world examples that could benefit from having more complex combinations

https://github.com/launchbadge/sqlx/blob/e33e4510fcab184f6bb655be3ba811e7f8a5b63f/sqlx-rt/Cargo.toml#L13

SQLx and a few other libraries have Cargo features for async runtimes & TLS backends. Ideally there would be separate tls-native-tls and runtime-tokio cargo features, with the combination activating the tokio-native-tls dependency, and equivalent for other runtimes & TLS backends. Currently it's required for sqlx-rt and every reverse dependency that wants to forward this choice of async runtime & tls backen to declare one feature per async runtime + tls backend combination.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jul 7, 2021

I have opened #3146 which also happens to use this all(...) syntax. Having come up twice already (I swear I didn't force it!), I believe it just falls naturally and we would strive in vain to avoid it.

I was not aware of @jplatte's example, and was just thinking through the problem in the abstract. What a pleasant surprise!

More than this is beyond my capacity at this time.

I don't want to diminish the great work you've done with Cargo, @ehuss, and I appreciate the honesty, but I also think stabilizing a feature because of one person's (volunteer?) bandwidth constraints should raise a very visible red flag to the Rust community at large. Moreover, I think this is emblematic of a longstanding problem where Cargo's design has been under-budgeted. Lacking the capacity to do more, it seems we are often scatching just the most irksome itches, and doing so with solutions that due to those resource constraints can't help but be somewhat incremental and ad-hoc.

I'm a firm believe in the Scheme slogan "Programming languages should be designed not by piling feature on top of feature, but by removing the weaknesses and restrictions that make additional features appear necessary". Moreover, I think it's not just a good design principle in the abstract, but an economic one. To wit, if we did the ? syntax now, and then did the all(...) syntax later, we would be maintaining more complexity in a now-redundant syntax in perpetuity.

I'd also like to through out I'm a big fan of RFC 2957. One hand, yes it is cleaning up something from Cargo's beginnings that someone could argue is unfortunate to have occurred in the first place -- people were raising concerns about cross compilation then. On the other hand, it is exactly the sort of "deep cleanup" that that gets me excited, the sort of work that sets a new standard of cleanliness rather than just playing wack-a-mole.

Of course, there is no real limit for that Scheme principle of up-front design investment and it could be taken to far, but the current gap between the care Rust's potential features receive vs Cargo's indicates to me that merely narrowing the gap will be safe. There is an initial upfront cost in doing extra work like this I fully admit, but if we can muster it I believe it will pay itself off quite quickly.

I would love to solve this resourcing problem in a way that doesn't asking more of the existing maintainers!

@pksunkara
Copy link

This is intended to be a small-scale, incremental step forward

I still think we need to stabilize this before discussing the next steps for evolving this. AFAIK quite a few libraries are encountering this issue in particular for a long time.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jul 7, 2021

To be clear, migrating the implementation to my more general construct I'd estimate as, roughly, a good day's work pair programming to get an initial PR. It's not a huge undertaking.

Of course, migrating the ecosystem would take longer, but people should not adopt unstable features and then expect them not to change.

The weak deps feature is only 8 months old (by it's tracking issue). Of course, the problem is older, and the goal is good, so that will build anticipation. Waiting a little bit longer is annoying, but being extra careful about design is, again, the sort of things that avoids these problems in the first place.

@Ericson2314
Copy link
Contributor

Replying above got me curious. This is the heart of the refactor:

  1. Today:
    https://github.com/rust-lang/cargo/blob/3ebb5f15a940810f250b68821149387af583a79e/src/cargo/core/resolver/features.rs#L412
    deferred_weak_dependencies: HashMap<(PackageId, bool, InternedString), HashSet<InternedString>>,
  2. Allow activated package to be different than conditioning package. n.b. host vs target tracked in ActivityMap:
    deferred_weak_dependencies: HashMap<(PackageId, InternedString), ActivityMap>,
  3. Allow activated condition to be dep or feature, new bool means which (dep vs feature):
    deferred_weak_dependencies: HashMap<(bool, InternedString), ActivityMap>,
  4. Allow multiple conditioning deps/features:
    deferred_weak_dependencies: HashMap<HashSet<(bool, InternedString)>, bool, ActivityMap>,
  5. Avoid bad O(n) looking for condition set that contains current package:
    type DepOrFeatureKey = (bool, InternedString);
    
    struct Cover<K, V>(HashMap<
        K, 
        HashMap<
            Rc<HashSet<K>>, // sets which contain outer key
            Rc<V>, // Value per set
        >,
    >);
    
    deferred_weak_dependencies: Cover<DepOrFeatureKey, ActivityMap>,
    This sort of query is called a "stabbing query". I am algorithms noob. I hope there is a crate for Cover and stabbing queries for this to save dev time and perhaps also be more performant, though I don't expect large all(...) to make the latter an issue in practice.
  6. Track with conditions have been met, so can continue to worry about just one input at a time:
    type DepOrFeatureKey = ...; // same
    
    struct Cover<K, V>(...); // same
    
    deferred_weak_dependencies: Cover<
        DepOrFeatureKey,
        (
            // Remove conditions as they have been met,
            // merge activity map into main once all conds met
            HashSet<DepOrFeatureKey>,
            ActivityMap,
        ),
    >,

I've there is no turn-key Cover or stabbing-query, then my estimate is too low. If there is, however, I stand by it.

@pksunkara
Copy link

It is not enough to implement it. It needs to go through the RFC process, thoroughly discussed/vetted, implemented as unstable feature and then stabilized. All of which will take time.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jul 7, 2021

@pksunkara the last one didn't require an RFC to land as an unstable feature. Nevermind this specific case at hand. If alternative designs come up in general that aren't hard to implement, we should be able to "run the experiment" creating the unstable feature and allowing the community to try it out in short order. It wouldn't be hard to then change this RFC while it is still a PR to go with the alterative.

RFC process, thoroughly discussed/vetted, implemented as unstable feature and then stabilized

Fundamentally these are important steps and yes we shouldn't cut them short. But in bringing up alternatives I am helping discussed/vetted the original. Saying we should cut back on the discussing/vetting the original because discussing/vetting an alliterative would take too long is just odd.

@djc
Copy link
Contributor

djc commented Jul 8, 2021

I am sympathetic to the need for a more expressive way of expressing feature relations (I think I've mentioned Gentoo's USE flags on internals before), I also think @Ericson2314's proposal, while more expressive than the original design proposed here, is a pretty bad fit with the rest of the syntax/design of how Cargo features work. In particular, using expressions containing operators in place of "keys" of what in so far has been a very key-value like structure would make this hard to understand to many users.

So yes, while I somewhat agree that it would be good to have a more powerful system for expressing feature relations, I think @Ericson2314's proposal itself is also too incremental, in the sense that it doesn't work from first principles to come up with a good design, but sort of stuffs one particular feature into the existing design in a way that's a pretty bad fit. If you want to come up with a more flexible, expressive design, I think that would be great, but at this point that should probably start as an internals pre-RFC.

While the last feature (or really, combination of features) did not require an RFC (although even at the time I argued for namespaced-features that it might make sense to do an RFC), that is also different because they're incremental feature that fit in well with the current design. If you're proposing to move the feature system into a different direction (which, again, I would be enthousiastic about), I think that deserves more upfront discussion (in part, as a way to unburden the Cargo team).

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jul 8, 2021

@djc Is not [target.'cfg(..)'] precedent?

See also my #3146 I linked which needs the same and(...) syntax. Because that addresses a problem blocking RFC #3140 for which a solution was asked for by the libs team, I think that RFC has a better change than me trying to change a design here last minute. But if that does go through, now the and(...) syntax (or whatever is agreed upon, I don't care) has an exact precedent and my counter-proposal here is a syntactically incremental design too.

I do agree that the fact we're stuffing a programming language in TOML has been an syntactic disaster, but trying change course there will unleash the mother of all bikeshedding. I don't personally want to spend time on that. My syntax may be new, but so is the ? syntax: we're both stuffing syntax into strings. Conversely, the semantics of both i'd argue is incremental. If you peer behind the mess of syntax we have today, what you will see is a monotonic system where enabling features always enables equal or more features/deps. My counter proposal is squarely within that design, which is why it is easy to implement.

@djc
Copy link
Contributor

djc commented Jul 12, 2021

It's not the and(..) syntax per se I have a problem with, it is the use inside of what has so far been the key in a key-value based design. target.cfg(..) feels conceptually different to me even though you could certainly argue that it is also key-like. But in that case, the operator is not applied to keys within the key-value system, but rather to keys from a different, environmental selection, which feels less surprising (though it certainly is of no great beauty and I have a hard time remembering the exact syntax off the top of my head).

The ? has some precedence at least outside Rust, for example in the Gentoo 'USE` flag system I mentioned before, where it has a very similar meaning. Because it's used in the value part of the key-value system and because it's a single character sigil, it seems much less of a change in direction to me.

The design of both is conceptually incremental, but to me your proposed syntax is much more alien.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jul 13, 2021

What about

[[feature-combinations]]
if_all = [ "feat0", "feat1" ]
then = ... # RHS as before

? We loose TOML insuring the keys are unique, but we get a way out of abusing strings in return.

I am still wary of what I feel is bike-shedding syntax, but in the hopes that something like this is good enough to get us back to discussing semantics, I'll propose alternatives.

@djc
Copy link
Contributor

djc commented Jul 17, 2021

Like your proposal in #3146, that would depend on adding a whole different section to Cargo.toml. I just don't see that making sense for both of these features, which are very core to how features work and will be used a lot.

Overall I (again) totally agree the use case is real, but none of the proposals so far seem convincing to me, which makes me all the more convinced we should stabilize weak dependencies now which will at least make this problem that much more tractable across the ecosystem, at the cost of some verbosity/boilerplate. A different solution to the verbosity/boilerplate should seek to attack that problem, not try to move it around, which I feel @Ericson2314's proposals are leaning towards.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jul 17, 2021

@djc I am having a hard time responding to your points. I feel like on one, big changes you say are too complex, and on the other other, smaller not but insignificant changes trying to "blend in" with existing syntax you find too awkward. I feel like you are therefore ruling out the all but the tiniest incremental steps, like the single-character ?, which dooms Cargo to keep on accumulating tiny ad-hoc features, scratched itch by scratched itch, until it it becomes a C++.

In general, I have find trying to propose ideas for Cargo over the years very frustrating because I feel the overtone window is so narrow, more so than Rust itself.

The irony to me is that programming languages itself had similar problems, where on one hand we need to standardize our intuitions over smaller more principled languages, but on the other hand, we have an obligation to continue to support existing programs. The solution was "core languages", where all the existing features in the "surface syntax" are supported, but desugared into the core langauge. This may or may not be done in the implementations (error messages complicate wanting to loose the original representation as quickly as possible), but if even if it doesn't, it provides the community with some agreed upon ways to think about myriad features and how they fit together, as opposed to everyone developing their own intractable intuition. Rust has this with MIR, and Chalk.

I think we really need that for Cargo, to take a step back from worrying about what the Cargo.toml looks like, how old versions of Cargo deal with newer Cargo.toml and index, and all these other important-but-distributing issues, and figure out the core language --- since it is internal to the implementation and our heads --- it side steps all those. Only after that, when we've cleared up our semantics and semantics, would I want to start really debating surface syntax. Trying to do everything at once is a recipe for failure.

If you're proposing to move the feature system into a different direction (which, again, I would be enthusiastic about), I think that deserves more upfront discussion (in part, as a way to unburden the Cargo team).

I hope per that @djc we could actually agree. And yes that step is certainly off-topic for here, so I will open an internals post for it.

I've done that https://internals.rust-lang.org/t/a-core-language-for-cargo/15033

@ehuss
Copy link
Contributor Author

ehuss commented Jul 19, 2021

Thanks everyone for the feedback!

I'm going to move forward with FCP, as the team has been discussing this, and would like to proceed in this direction. I understand that there are use cases that the feature system is not able to cover, and I think those can be explored with other RFCs or community discussions.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 19, 2021

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Jul 19, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Jul 19, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Jul 19, 2021
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. to-announce and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Jul 29, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Jul 29, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@ehuss ehuss force-pushed the cargo-weak-namespaced-features branch from 7c41ed0 to b120b56 Compare July 30, 2021 17:24
@ehuss ehuss merged commit 8b7e128 into rust-lang:master Jul 30, 2021
@ehuss
Copy link
Contributor Author

ehuss commented Jul 30, 2021

Huzzah! The @rust-lang/cargo team has decided to accept this RFC.

To track further discussion, subscribe to the tracking issues here:
rust-lang/cargo#5565 rust-lang/cargo#8832

bors added a commit to rust-lang/cargo that referenced this pull request Apr 18, 2022
feat: Import cargo-add into cargo

### Motivation

The reasons I'm aware of are:
- Large interest, see #5586
- Make it easier to add a dependency when you don't care about the version (instead of having to find it or just using the major version if thats all you remember)
- Provide a guided experience, including
  - Catch or prevent errors earlier in the process
  - Bring the Manifest format documentation into the terminal via `cargo add --help`
  - Using `version` and `path` for `dependencies` but `path` only for `dev-dependencies` (see crate-ci/cargo-release#288 which led to killercup/cargo-edit#480)

### Drawbacks

1. This is another area of consideration for new RFCs, like rust-lang/rfcs#3143 (this PR supports it) or rust-lang/rfcs#2906 (implementing it will require updating `cargo-add`)

2. This is a high UX feature that will draw a lot of attention (ie Issue influx)

e.g.
- killercup/cargo-edit#521
- killercup/cargo-edit#126
- killercup/cargo-edit#217

We've tried to reduce the UX influx by focusing the scope to preserving semantic information (custom sort order, comments, etc) but being opinionated on syntax (style of strings, etc)

### Behavior

Help output
<details>

```console
$ cargo run -- add --help
cargo-add                                                                                                                                  [4/4594]
Add dependencies to a Cargo.toml manifest file

USAGE:
    cargo add [OPTIONS] <DEP>[`@<VERSION>]` ...
    cargo add [OPTIONS] --path <PATH> ...
    cargo add [OPTIONS] --git <URL> ...

ARGS:
    <DEP_ID>...    Reference to a package to add as a dependency

OPTIONS:
        --no-default-features     Disable the default features
        --default-features        Re-enable the default features
    -F, --features <FEATURES>     Space-separated list of features to add
        --optional                Mark the dependency as optional
    -v, --verbose                 Use verbose output (-vv very verbose/build.rs output)
        --no-optional             Mark the dependency as required
        --color <WHEN>            Coloring: auto, always, never
        --rename <NAME>           Rename the dependency
        --frozen                  Require Cargo.lock and cache are up to date
        --manifest-path <PATH>    Path to Cargo.toml
        --locked                  Require Cargo.lock is up to date
    -p, --package <SPEC>          Package to modify
        --offline                 Run without accessing the network
        --config <KEY=VALUE>      Override a configuration value (unstable)
    -q, --quiet                   Do not print cargo log messages
        --dry-run                 Don't actually write the manifest
    -Z <FLAG>                     Unstable (nightly-only) flags to Cargo, see 'cargo -Z help' for
                                  details
    -h, --help                    Print help information

SOURCE:
        --path <PATH>        Filesystem path to local crate to add
        --git <URI>          Git repository location
        --branch <BRANCH>    Git branch to download the crate from
        --tag <TAG>          Git tag to download the crate from
        --rev <REV>          Git reference to download the crate from
        --registry <NAME>    Package registry for this dependency

SECTION:
        --dev                Add as development dependency
        --build              Add as build dependency
        --target <TARGET>    Add as dependency to the given target platform

EXAMPLES:
  $ cargo add regex --build
  $ cargo add trycmd --dev
  $ cargo add --path ./crate/parser/
  $ cargo add serde serde_json -F serde/derive
```

</details>

Example commands
```rust
cargo add regex
cargo add regex serde
cargo add regex@1
cargo add regex@~1.0
cargo add --path ../dependency
```
For an exhaustive set of examples, see [tests](https://github.com/killercup/cargo-edit/blob/merge-add/crates/cargo-add/tests/testsuite/cargo_add.rs) and associated snapshots

Particular points
- Effectively there are two modes
  - Fill in any relevant field for one package
  - Add multiple packages, erroring for fields that are package-specific (`--rename`)
  - Note that `--git` and `--path` only accept multiple packages from that one source
- We infer if the `dependencies` table is sorted and preserve that sorting when adding a new dependency
- Adding a workspace dependency
  - dev-dependencies always use path
  - all other dependencies use version + path
- Behavior is idempotent, allowing you to run `cargo add serde serde_json -F serde/derive` safely if you already had a dependency on `serde` but without `serde_json`
- When a registry dependency's version req is unspecified, we'll first reuse the version req from another dependency section in the manifest.  If that doesn't exist, we'll use the latest version in the registry as the version req

### Additional decisions

Accepting the proposed `cargo-add` as-is assumes the acceptance of the following:
- Add the `-F` short-hand for `--features` to all relevant cargo commands
- Support ``@`` in pkgids in other commands where we accept `:`
- Add support for `<name>`@<version>`` in more commands, like `cargo yank` and `cargo install`

### Alternatives

- Use `:` instead of ``@`` for versions
- Flags like `--features`, `--optional`, `--no-default-features` would be position-sensitive, ie they would only apply to the crate immediate preceding them
  - This removes the dual-mode nature of the command and remove the need for the `+feature` syntax (`cargo add serde -F derive serde_json`)
  - There was concern over the rarity of position-sensitive flags in CLIs for adopting it here
- Support a `--sort` flag to sort the dependencies (existed previously)
  - To keep the scope small, we didn't want general manifest editing capabilities
- `--upgrade <POLICY>` flag to choose constraint (existed previously)
  - The flag was confusing as-is and we feel we should instead encourage people towards `^`
- `--allow-prerelease` so a `cargo add clap` can choose among pre-releases as well
  - We felt the pre-release story is too weak in cargo-generally atm for making it first class in `cargo-add`
- Offer `cargo add serde +derive serde_json` as a shorthand
- Infer path from a positional argument

### Prior Art

- *(Python)* [poetry add](https://python-poetry.org/docs/cli/#add)
  - `git+` is needed for inferring git dependencies, no separate  `--git` flags
  - git branch is specified via a URL fragment, instead of a `--branch`
- *(Javascript)* [yarn add](https://yarnpkg.com/cli/add)
  - `name@data` where data can be version, git (with fragment for branch), etc
  - `-E` / `--exact`, `-T` / `--tilde`, `-C` / `--caret` to control version requirement operator instead of `--upgrade <policy>` (also controlled through `defaultSemverRangePrefix` in config)
  - `--cached` for using the lock file (killercup/cargo-edit#41)
  - In addition to `--dev`, it has `--prefer-dev` which will only add the dependency if it doesn't already exist in `dependencies` as well as `dev-dependencies`
  - `--mode update-lockfile` will ensure the lock file gets updated as well
- *(Javascript)* [pnpm-add](https://pnpm.io/cli/add)
- *(Javascript)* npm doesn't have a native solution
  - Specify version with ``@<version>``
  - Also overloads `<name>[`@<version>]`` with path and repo
    - Supports a git host-specific protocol for shorthand, like `github:user/repo`
    - Uses fragment for git ref, seems to have some kind of special semver syntax for tags?
  - Only supports `--save-exact` / `-E` for operators outside of the default
- *(Go)* [go get](https://go.dev/ref/mod#go-get)
  - Specify version with ``@<version>``
  - Remove dependency with ``@none``
- *(Haskell)* stack doesn't seem to have a native solution
- *(Julia)* [pkg Add](https://docs.julialang.org/en/v1/stdlib/Pkg/)
- *(Ruby)* [bundle add](https://bundler.io/v2.2/man/bundle-add.1.html)
  - Uses `--version` / `-v` instead of `--vers` (we use `--vers` because of `--version` / `-V`)
  - `--source` instead of `path` (`path` correlates to manifest field)
  - Uses `--git` / `--branch` like `cargo-add`
- *(Dart)* [pub add](https://dart.dev/tools/pub/cmd/pub-add)
  - Uses `--git-url` instead of `--git`
  - Uses `--git-ref` instead of `--branch`, `--tag`, `--rev`

### Future Possibilities

- Update lock file accordingly
- Exploring the idea of a [`--local` flag](killercup/cargo-edit#590)
- Take the MSRV into account when automatically creating version req (killercup/cargo-edit#587)
- Integrate rustsec to report advisories on new dependencies (killercup/cargo-edit#512)
- Integrate with licensing to report license, block add, etc (e.g. killercup/cargo-edit#386)
- Pull version from lock file (killercup/cargo-edit#41)
- Exploring if any vendoring integration would be beneficial (currently errors)
- Upstream `cargo-rm` (#10520), `cargo-upgrade` (#10498), and `cargo-set-version` (in that order of priority)
- Update crates.io with `cargo add` snippets in addition to or replacing the manifest snippets

For more, see https://github.com/killercup/cargo-edit/issues?q=is%3Aissue+is%3Aopen+label%3Acargo-add

### How should we test and review this PR?

This is intentionally broken up into several commits to help reviewing
1. Import of production code from cargo-edit's `merge-add` branch, with only changes made to let it compile (e.g. fixing up of `use` statements).
2. Import of test code / snapshots.  The only changes outside of the import were to add the `snapbox` dev-dependency and to `mod cargo_add` into the testsuite
3. This extends the work in #10425 so I could add back in the color highlighting I had to remove as part of switching `cargo-add` from direct termcolor calls to calling into `Shell`

Structure-wise, this is similar to other commands
- `bin` only defines a CLI and adapts it to an `AddOptions`
- `ops` contains a focused API with everything buried under it

The "op" contains a directory, instead of just a file, because of the amount of content.  Currently, all editing code is contained in there.  Most of this will be broken out and reused when other `cargo-edit` commands are added but holding off on that for now to separate out the editing API discussions from just getting the command in.

Within the github UI, I'd recommend looking at individual commits (and the `merge-add` branch if interested), skipping commit 2.  Commit 2 would be easier to browse locally.

`cargo-add` is mostly covered by end-to-end tests written using `snapbox`, including error cases.

There is additional cleanup that would ideally happen that was excluded intentionally from this PR to keep it better scoped, including
- Consolidating environment variables for end-to-end tests of `cargo`
- Pulling out the editing API, as previously mentioned
  - Where the editing API should live (`cargo::edit`?)
  - Any more specific naming of types to reduce clashes (e.g. `Dependency` or `Manifest` being fairly generic).
- Possibly sharing `SourceId` creation between `cargo install` and `cargo edit`
- Explore using `snapbox` in more of cargo's tests

Implementation justifications:
- `dunce` and `pathdiff` dependencies: needed for taking paths relative to the user and make them relative to the manifest being edited
- `indexmap` dependency (already a transitive dependency): Useful for preserving uniqueness while preserving order, like with feature values
- `snapbox` dev-dependency: Originally it was used to make it easy to update tests as the UX changed in prep for merging but it had the added benefit of making some UX bugs easier to notice so they got fixed.  Overall, I'd like to see it become the cargo-agnostic version of `cargo-test-support` so there is a larger impact when improvements are made
- `parse_feature` function: `CliFeatures` forces items through a `BTreeSet`, losing the users specified order which we wanted to preserve.

### Additional Information

See also [the internals thread](https://internals.rust-lang.org/t/feedback-on-cargo-add-before-its-merged/16024).

Fixes #5586
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-cargo Relevant to the Cargo team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants