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

Add code for simultaneous installation of multiple crates #3075

Closed
wants to merge 1 commit into from

Conversation

esclear
Copy link
Contributor

@esclear esclear commented Sep 6, 2016

This is a change in the backend, for now and will only be used in the internals, as discussed with @alexcrichton.

Proper command parsing will follow, there is already a prototype: https://gist.github.com/esclear/4715c7ec3ff129b10b2342cde05fd46b

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@esclear
Copy link
Contributor Author

esclear commented Sep 6, 2016

Oh, Tests failed.
I'll look into that.

@alexcrichton
Copy link
Member

Thanks! Could you be sure to add some tests for the new functionality as well?

@esclear
Copy link
Contributor Author

esclear commented Sep 8, 2016

That should have fixed the tests and also simplified the changes.
I could not check the cross-compiling-tests though.

@alexcrichton
Copy link
Member

Ok, got a chance to look at this. Cleared up on IRC that new new functionality was added to the CLI, just the backend.

Unfortunately though I think this suffers the same problem as #2601 where once the support is hooked up it'll print out and update the registry one time per crate being installed. That experience though, I believe, was the primary stalling point for the previous PR.

Maybe it'd be good to go ahead and add support to the CLI all in one go to see how it'd work?

@esclear
Copy link
Contributor Author

esclear commented Sep 20, 2016

This is indeed a problem.
I looked into the code for a bit.

It seems to me that we want to prevent the Registry::update function from being triggered multiple times.

Also, I'm not yet sure how to parse the arguments.
Docopt is probably not sufficient for what we would want to do. I asked some people and also opened an issue ( docopt/docopt#344 ), but I'm almost certain this cannot be done with it.

I might look into clap or something similar.

@alexcrichton
Copy link
Member

Yeah I think we'll basically want to persist the same Registry across multiple installations so it can retain the state of what's updated. I'd also prefer to avoid two command line parsers for now, let's stick to docopt and try to fix bugs and/or work around it.

@esclear
Copy link
Contributor Author

esclear commented Sep 26, 2016

I agree to having a persistent Registry.
But I think that it is not possible to implement a proper command parsing solution solely with docopt.

@bors
Copy link
Collaborator

bors commented Oct 6, 2016

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

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to resubmit with a rebase!

bors added a commit that referenced this pull request Jul 28, 2017
cargo install multiple crates

rust-lang/rustup#986 for `cargo install`

Revives #2601 @pwoolcoc, replaces #3075 @esclear, closes #2585 @kindlychung @cyplo

Avoids the sticking point of the previous two PRs (multiple registry updates) by threading through a first-run boolean flag to decide whether `select_pkg` needs to call `source.update()`.

There is still the issue that flags such as `--git` and `--vers` are "global" to the multiple packages you may be installing. The workaround is just to run `cargo install` separately. In the future we could add syntax like `cargo install foo=1.0 bar=2.5 quux=git://github.com/durka/quux#dev-branch` or something.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants