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

Allow installation of multiple packages in one line #2601

Closed
wants to merge 1 commit into from

Conversation

pwoolcoc
Copy link
Contributor

Closes #2585

@rust-highfive
Copy link

r? @huonw

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

@japaric
Copy link
Member

japaric commented Apr 21, 2016

If I call cargo install a b c and installation of b fails, does Cargo "keeps going" and tries to install c or does it stop at b's failure? From the logic it seems this PR implements the latter behavior. I'm not sure which one should be default, but if we implement the latter behavior we could add a -k (--keep-going) flag that provides the former behavior. If we change the defaults, I'm not sure what I would call the flag to switch the behaviors: -e like bash?.

@pwoolcoc
Copy link
Contributor Author

You are correct, if b fails, the command fails.

@@ -123,14 +123,20 @@ pub fn execute(options: Options, config: &Config) -> CliResult<Option<()>> {
try!(SourceId::for_central(config))
};

let krate = options.arg_crate.as_ref().map(|s| &s[..]);
let krate = options.arg_crate.as_ref().map(|s| s.iter().map(|t| &t[..]).collect::<Vec<_>>());
Copy link
Member

Choose a reason for hiding this comment

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

The other versions of this statement are actually just converting Option<&String> to Option<&str> but I don't think that's necessary if we have Vec<String>, so this part can probably just get removed.

@alexcrichton
Copy link
Member

Thanks for the PR @pwoolcoc! I'm fine with the fail-fast semantics here, although we may be able to get away with "more transactional" semantics in terms of we don't install anything if anything fails. That may not be the most important thing in the world, however.

@bors
Copy link
Contributor

bors commented May 3, 2016

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

@pwoolcoc
Copy link
Contributor Author

pwoolcoc commented May 3, 2016

Thanks @alexcrichton , I am taking a look at this today and hope to fix all the issues you brought up

@pwoolcoc
Copy link
Contributor Author

pwoolcoc commented May 3, 2016

@alexcrichton what do you think the best way to handle the various "crate-selecting" options would be, in the presence of multiple crates? I.e., what does this do:

cargo install --git https://github.com/foo/bar crate_one crate_two crate_three

Should we attempt to use the same source for all the listed crates? Or disable the various "crate-selecting" options when there are multiple crates passed (that would be --vers, --git, --branch, --tag, --rev, and --path)?

@alexcrichton
Copy link
Member

Hm, that's a good question! I wonder if there's a way that we could specify this to docopt. Something like where you can pass all these options multiple times, but they're all associated with only the next (or previous) command line argument. That way you could install multiple things from multiple sources at multiple versions.

That being said it may also indicate that this may not work out so hot...

@alexcrichton
Copy link
Member

Ok, got a chance to chat about this with @wycats today, and the conclusion was that this seems fine to add, even with the limitation that you can't install packages from multiple sources. This is ultimately just a convenience so if you happen to just not be able to install multiple packages from multiple sources, you can always just run the command twice!

@pwoolcoc
Copy link
Contributor Author

@alexcrichton great! Thanks for the clarification. So, just to be clear, should we still try to pull all the packages from a specified source, or should we ignore the specified soure entirely (or throw an error when specifying a source for multiple packages)?

@alexcrichton
Copy link
Member

Yeah this'd basically be a command where you can install multiple crates from one source, and you can just configure what that source is. Which is to say we should pull all the crates from the specified source, not throw it away

@bors
Copy link
Contributor

bors commented May 26, 2016

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

@pwoolcoc
Copy link
Contributor Author

pwoolcoc commented May 26, 2016

@alexcrichton I think I need a bit of help here. In order to avoid re-updating the registry every time it starts to install another crate, I moved the loop into cargo::ops::cargo_install::install. The problem is that, within the loop, we are passing ownership of the source (which is a Box<Source + 'a>) to the ops::compile_pkg function, so that, the next time through the loop, I get a 'use of moved value' error. I have been trying to figure out the best way to maintain ownership of that boxed trait object, and need some advice.

I have tried passing the source as a &Box<Source +'a>, but that involved changing a lot of code, from all over the codebase, and I didn't really think it was worth it.

I also tried changing it from Box<> to Rc<>, but that doesn't work because it isn't Sized.

Here is the exact place where we pass ownership: https://github.com/pwoolcoc/cargo/blob/issue-2585/src/cargo/ops/cargo_install.rs#L103

Any help you (or anyone else, really) can give would be appreciated.

@alexcrichton
Copy link
Member

@pwoolcoc it may make the most sense to just use the same PackageRegistry instance (which owns the Source object) across multiple compilations, so it could perhaps become an argument passed through?

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to resubmit with comments addressed!

@cyplo
Copy link

cyplo commented Jul 9, 2016

Hey ! @pwoolcoc: I am interested in helping on this one, let me know if you need any help.

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.

7 participants