[WIP]: Add cargo-add (from cargo-edit) to cargo proper

[ibaryshnikov]

Implements #5586

The work is at early stage and contains mostly code copied from cargo install command. But it compiles and it is possible to call the subcommand: ./target/debug/cargo add some_crate

I've opened this pr to discuss the key decisions during the implementation process.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[killercup]

Thanks for opening this! Let me know as soon as you want feedback or have questions!

[ibaryshnikov]

@killercup inside cargo the data structure for Cargo.toml file is TomlManifest
Is it a good idea to add to_string method to this structure and use it instead of Manifest from cargo-edit?

[ibaryshnikov]

After some research I found that cargo's own toml parser doesn't preserve file structure (like comments and whitespaces). It needs to move toml_edit here in addition to cargo-edit. In case to have only one parser.

But currently I'm stuck with error kinds while moving manifest file from cargo-edit to cargo. Can you give me an advice how to update these errors to be more appropriate for cargo?
And what is the best place for manifest.rs? I chose util/toml because it represents the data type for a toml file.

[bors]

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

[alexcrichton]

Thanks for opening this @ibaryshnikov and sorry for the delay! I'm pretty excited to see this start moving and I'm also excited that cargo-edit has come so far!

It looks like some of the integration points are still missing here in terms of being a mergeable PR, but in general it may also be worthwhile to discuss integration strategies. First off I'm personaly in favor of merging cargo-edit into Cargo now if it has a style-preserving TOML parser, that's all we were waiting for! Second, though, I think we'll want to integrate the project into Cargo's codebase in an idiomatic fashion with Cargo's own code (aka errors and such).

I think for integration though we'll also want to try to avoid having two TOML parsers (if we can). I'd ideally like to keep everything on the same parser if we can to help reduce bugs and such. For example it looks like toml_edit is using a nom-based parser, but perhaps tokens.rs could be exported and/or extracted to a crate and shared between the two?

[ibaryshnikov]

Thank you for the explanations, @alexcrichton !

I've just replaced ErrorKind errors with format_err macro. The next step is to find a way to replace chain_err.

[alexcrichton]

Ok, great!

[alexcrichton]

ping @ibaryshnikov, have you had a chance to work on this recently?

[ibaryshnikov]

not yet (
I'm going to hack into it once more this weekend.
it would be great if someone could help me with this issue

[ibaryshnikov]

Found out how to deal with chain_err. Now I'm going to find how to use tokens.rs here

[ordian]

Hello, author of toml_edit. I'm happy to help to make this happen.

For example it looks like toml_edit is using a nom-based parser, but perhaps tokens.rs could be exported and/or extracted to a crate and shared between the two?

Actually toml_edit uses combine-based parser. I can hack over this weekend to extract and reuse tokens.rs if it helps.

[alexcrichton]

@ordian ok that'd be great yeah! That would deduplicate some of the most complicated parts of TOML parsing which would work well. Currently the toml crate doesn't expose its tokens as a public module, but I think it would be fine to either extract what's there to a separate crate or just make it public

[ibaryshnikov]

Implemented minimal working example using toml_edit in its current state.
Only works with packages from crates.io at the moment.
To test execute something like cargo run add first second
What is a good way to specify versions? cargo install subcommand uses --version flag, and in cargo_edit we can see both flag and name@version pattern.

[ordian]

What is a good way to specify versions? cargo install subcommand uses --version flag, and in cargo_edit we can see both flag and name@version pattern.

I think we should be consistent with cargo update, so consider using --precise flag (w/o name@version).

[ordian]

Also I would suggest using more recent version of cargo-edit, where we support specifying both path and version at the same time (see killercup/cargo-edit#214).

[ibaryshnikov]

Thank you for your comments, @ordian ! I picked the latest code from cargo-edit for dependency.rs
Tried to test it,

cargo run add toml --git=https://github.com/alexcrichton/toml-rs --version=0.4.6

produces

toml = { version = "0.4.6", path = "https://github.com/alexcrichton/toml-rs#8181a7e6" }

[alexcrichton]

Ok this has been a bit stale for quite some time now, so I'm going to close this at this time. The Cargo team very much wants to add this, however, and we'll be looking into allocating resources to implement this change in early 2019 hopefully!

[ibaryshnikov]

@alexcrichton have you had a chance to look through the code before closing it? I was waiting for your comments. I mean, allocating some resources would be helpful, but may be some changes from this PR could be used, too.

[alexcrichton]

@ibaryshnikov unfortunately no I didn't have time to do a thorough review (and it looks like no others did either). Reviewing this will likely require a pretty significant investment from someone on the Cargo team because it's a very highly-desired and relatively large piece of new functionality. There's quite a lot of code to review here not only in this repository but in the crates.io dependencies.

[ibaryshnikov]

@alexcrichton do you know anyone who can do the review?
The #5586 was created as a part of the impl days in Paris this summer. It supposed to be an easy task to help people start contributing. Also I was going to continue working on it during the impl days in Rome in several weeks. I hoped there would be someone who can help with my questions.

[alexcrichton]

@rust-lang/cargo would others be able to help out in reviewing this? @ibaryshnikov is requesting a review.

Ah sorry @ibaryshnikov but this probably should definitely not be classified as an easy task, that was a mistake :(

[shirshak55]

any status?