Cargo: Add cargo-add (from cargo-edit) to cargo proper

[killercup]

Cargo as well as cargo-edit have gone a long way since #4 was opened. It might now make sense to move (parts of) cargo-edit into cargo itself. Basically, we now have a format-preserving TOML library that -- at least for adding a dependency line -- work quite well (you can test this with cargo install cargo-edit --vers 0.3.0-beta.1 -f).

Speaking with @matklad, I believe I now have a somewhat solid understanding of what needs to happen to add a new built-in cargo subcommand. One important realization was that cargo-install already contains some of the parts we need.

Here are some steps to get started:

1. Create a src/bin/cargo/commands/add.rs (as duplicate of install.rs at first)
2. In src/bin/cargo/commands/mod.rs, add add to builtin/builtin_exec, and to the list of mods
3. Create a src/bin/cargo/ops/cargo_add.rs (as duplicate of cargo_install.rs at first)

One thing we should aim for though, is to now just copy-paste the whole of cargo-edit but to re-use as much of cargo's infrastructure as possible. For example, we should try to use cargo's way of querying the registry as well as its CLI args handling and output formatting. We can, for example, also use args.workspace.current_manifest to get the manifest and deal with workspace.

[killercup]

@rust-lang/cargo, is this something you'd be interested in being worked on right now? This might work as a mentored even if you can't spend much time to work on it ourselves.

[alexcrichton]

I'd personally be thrilled to see this enter Cargo itself! I would ideally prefer to avoid having two TOML parsers built in but if we need to have that for the time being I think it's not the end of the world. (I don't know much about the technical design of cargo-edit right now)

[steveklabnik]

This is the number one thing I want in Cargo; a simple 👍 is not enough. Tons and tons and tons of people are very excited for this 🎊

[ibaryshnikov]

I'd like to implement it, work in-progress is here
https://github.com/ibaryshnikov/cargo/tree/issue-5586-cargo-add

[japaric]

I know that lots of people (myself included) are plenty busy with stuff for the upcoming edition release and that we don't have much spare ~human~ Rustacean resources but I still want to say that it would be AMAZING to have cargo add in Cargo 2018. As mentioned by boats, cargo add fits perfectly with the module / path changes of the edition. And I fully agree, but to me the biggest win would be not having to cargo install cargo-edit, which involves compiling / linking to C code and can be a (very) frustrating experience.

@rust-lang/cargo @killercup @aturon could we make this happen, somehow? :pray:

[steveklabnik]

Huuuuuuuuuuuuge :+1: here

On Sep 6, 2018, at 7:59 PM, Jorge Aparicio [e-mail] wrote:

I know that lots of people (myself included) are plenty busy with stuff for the upcoming edition release and that we don't have much spare human Rustacean resources but I still want to say that it would be AMAZING to have cargo add in Cargo 2018. As mentioned by boats, cargo add fits perfectly with the module / path changes of the edition. And I fully agree, but to me the biggest win would be not having to cargo install cargo-edit, which involves compiling / linking to C code and can be a (very) frustrating experience.

@rust-lang/cargo @killercup @aturon could we make this happen, somehow? 🙏

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

[joshtriplett]

:+1: here.

[ibaryshnikov]

I did some preparations in this pr like filling the boilerplate and updating errors from cargo-edit to fit more for cargo, but farther work requires some patches to toml parser which is used in cargo (in case we don't want to have two parsers), and currently I don't have enough time to do required updates in the parser on my own. If you find some of the code in this pr fits for the solution, feel free to use it.

[thejpster]

I just wanted to add that I think cargo-add is a great tool for people new to Rust, as is cargo-generate. Unfortunately if I recommend them in a guide or tutorial, I have to explain how to install them and that they need to first install GCC and cmake and libssl-dev, which is non-trivial.

[withoutboats]

Definitely would love to see this added to cargo, since my workflow setting up a new dev environment is:

1. Install Rust
2. Install ripgrep
3. cargo install cargo-edit

As @ibaryshnikov points out though, its nontrivial work since the TOML parser cargo uses right now doesn't preserve layout, whitespace, or comments. We need to migrate to using the same parser cargo-edit uses before we can upstream any of its features into cargo.

[withoutboats]

We discussed this in the cargo meeting. There was strong support for upstreaming the functionality of cargo add into cargo. We doubt it can be stable in time for the 2018 edition (since the final cargo master for the edition needs to be merged roughly 5 weeks from today) but we do think its good to start making progress on adding this functionality.

We are comfortable having two TOML parsers in cargo for now (with a view to eventually merging them) so long as they share a common lexer, to be confident they aren't interpreting the TOML significantly differently. @alexcrichton says he's left a comment on the repo for the parser cargo-edit is using to this effect.

[sandangel]

Hi everyone, may I know the status of this issue? It's 2019 now and Happy New Year. ^^

[Eh2406]

From the discord Today at 5:48 PM:
"ehuss: (btw, my format-preserving toml parser just passed all tests :tada: :smile: )"

[yhaskell]

Hello! Does anyone know what is the status of this?

[ordian]

In https://github.com/rust-lang/cargo/pull/5611#issuecomment-401895847 a concern was raised that we should (partially) deduplicate toml parsing libraries (cargo currently uses toml-rs, but that's not a lossless toml parser, e.g. it will eat your comments, cargo-edit currently uses toml_edit) before using it in cargo. Unfortunately, I don't have time to work on it (link to issues), but would be happy to mentor if someone wants to make this happen (if you're interested, please leave a message in https://gitter.im/toml_edit/Lobby).

After the above issues are resolved, we can hopefully revive #5611.

[goodSyntax808]

@SamedhG and I have started to work on updating toml_edit to support toml v0.5 We wanted to hear the opinions of the rust team and see which is a better option, or if there is not a significant difference between the two:

1. Add lossless parsing to toml_rs and adapt cargo_edit to use that instead of toml_edit, then merge cargo-edit into cargo proper
2. Update toml_edit to support toml v0.5 and update cargo to use toml_edit, then merge cargo-edit into cargo proper

@killercup @alexcrichton @steveklabnik @ordian

[ehuss]

I'd be very wary of having a second TOML parser to Cargo. I think it was previously mentioned that if they could share a lexer, that might be a compromise. I'm still concerned about adding a large dependency that has doubt around how well it is maintained.

I did work on an experimental preserving API for toml-rs, but never finished it. I do have the intention of picking it back up, but probably not for a long while.

Creating a good, clean API that handles all the intricacies of toml is quite difficult, and it will require some moderately invasive changes to toml-rs. I unfortunately don't have a lot of time to help with this right now.

[ordian]

I agree with @ehuss there. On the other hand, I don't know how different the first approach would be in terms of maintenance, because in the end it would still be two different toml libraries sharing the same lexer and maybe some other parts. Even though I don't have time to work on some features myself, I still can review PRs and accept contributions.

[ehuss]

I have pushed an updated branch of the changes I was working on toml-rs for providing a raw API (raw-api). The idea is to have a low-level API exposed in the toml deserializer, and then a format-preserving API can be built on top of it. If someone wants to help push this along, one possibility is to build that API on top of that raw API and see how it goes from there. I had started on a document API earlier this year (document) that uses the raw API, but I think it ended up being too ambitious, trying to automatically handle all the table kinds in a an intelligent way.

@alexcrichton What do you think about that strategy, and the general shape of the API changes to toml-rs? In essence, Deserializer::fragment is the same as the old line function, with pub added. Each Fragment kind retains all the whitespace/comments of the surrounding text.

[alexcrichton]

Looks great to me! I'd be down for adding that and trying to build cargo-edit on top of that.

[ordian]

Just to give an update from my side: I didn't have enough spare to work on toml_edit myself (but hopefully this will change soon), but there is an open PR to add dotted keys support. The thing is it's tricky to make an easy to use and lossless API to work with dotted keys, at least it requires some thought and time.

I would be happy to unify and merge our efforts @ehuss, so that we can make this happen sooner. Also, there is a need for lossless toml parser outside of cargo-edit as well, e.g. for cargo-release, cargo-tidy, cargo-audit-fix, etc. It would be a shame if it will end up being a one-man job. Let's collaborate to increase the bus factor!

[tyt2y3]

@ehuss Can you name a few intricacies of TOML you think is difficult to handle? I'd like to assess how difficult the task would be, if I or someone would be able to help.

[ehuss]

Some of the tricky bits I can think of:

• 4 different table types (bracketed tables, inline tables, dotted keys, and array-of-tables). Generally when editing, you want to preserve the existing style, and insert into the correct location. I wanted to make this transparent so that the user doesn't need to do a lot of work to detect which is the correct type to use, and where it can be placed.
• Array-of-tables are contextual, with tables defined after an array belonging to the most recent table.
• It would be nice to have an API that supported index operations like serde_json's Value type does, but since TOML doesn't have a null/none concept, that is quite tricky. It looks like toml_edit does this, too.

I would suggest starting with the raw API I created, and creating something very simple that can convert the raw elements into a structured document with a read-only API, and a way to re-render the document. Then try to add simple mutation methods that don't break the structure, and incrementally try to create more sophisticated APIs that are easier to use and more natural. I would probably look at toml_edit and at least use it as inspiration if not make it identical or modify it to use the toml crate.

[DrYSG]

How is this ticket different than: https://crates.io/crates/cargo-edit

[steveklabnik]

This ticket is about up streaming that, at least conceptually.

[kornelski]

It looks like this highly-desirable feature has got delayed for years waiting for a super-perfect format-preserving TOML reparser.

IMHO perfection in the parser is not that important. Seriously, don't worry about it. I've been happily using cargo-edit even when it had non-preserving parser and sorted all the keys alphabetically.

Non-preserving parser is almost a feature! It's like cargo fmt for TOML files :) Any cargo add at all is better than none.

[lig]

@kornelski Personally, I'd prefer the one which sorts everything as I do it myself every time after cargo-add:) The reason is exactly the same as the reason behind cargo fmt is.

[joshtriplett]

The current version of cargo add will keep your dependencies in sorted order as long as they started out in sorted order.

[Boscop]

@joshtriplett It doesn't seem to be the case (?)
I had this:

[dependencies]
persistence = { path = "../persistence" }

then I did cargo add serde serde_derive and then cargo add anyhow. Then it looked like this:

[dependencies]
persistence = { path = "../persistence" }
serde = "1.0.118"
serde_derive = "1.0.118"
anyhow = "1.0.37"

Why didn't it add anyhow before the other deps?