Add `--force` flag to cargo install #2405

Merged
merged 2 commits into from May 2, 2016

Conversation

Projects
None yet
5 participants
@gkoz
Contributor

gkoz commented Feb 21, 2016

Close #2082.

Adding --force (-f) instructs cargo to overwrite existing binaries (updating the metadata accordingly).
This allows updating crates via cargo install -f <crate>.

Installation happens in two stages now: binaries are copied into a temporary subdirectory of the destination first, then moved into destination. This should catch some errors earlier.

In case of installation error cargo will remove new binaries but won't attempt to undo successful overwrites.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Feb 21, 2016

r? @huonw

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

r? @huonw

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

@gkoz

This comment has been minimized.

Show comment
Hide comment
Contributor

gkoz commented Feb 21, 2016

@rust-highfive rust-highfive assigned alexcrichton and unassigned huonw Feb 21, 2016

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 22, 2016

Member

Thanks for the PR @gkoz! There's a few technical aspects I'd tweak here, but I agree that the first thing to do here is probably to decide whether we want to do this in the first place. Let's discuss more on the issue, though, as that's probably the best place to have this discussion.

(I'll write a comment over there shortly)

Member

alexcrichton commented Feb 22, 2016

Thanks for the PR @gkoz! There's a few technical aspects I'd tweak here, but I agree that the first thing to do here is probably to decide whether we want to do this in the first place. Let's discuss more on the issue, though, as that's probably the best place to have this discussion.

(I'll write a comment over there shortly)

src/cargo/ops/cargo_install.rs
+ if replace {
+ return Ok(Some(p.clone()))
+ }
+ msg.push_str(&format!(" as part of `{}` (use --replace to override)", p));
}
Err(human(msg))

This comment has been minimized.

@gkoz

gkoz Feb 23, 2016

Contributor

I've realized that it's safer to always list all conflicts when run without --replace thus the error should probably be generated somewhere up the stack.

@gkoz

gkoz Feb 23, 2016

Contributor

I've realized that it's safer to always list all conflicts when run without --replace thus the error should probably be generated somewhere up the stack.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Feb 25, 2016

Contributor

There doesn't seem to be any opposition to this change.

Contributor

gkoz commented Feb 25, 2016

There doesn't seem to be any opposition to this change.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 11, 2016

Member

Ok, @wycats and I had a chance to talk about this recently, and this seems like it's desirable as it's pretty orthogonal from the --update flag. Put another way, cargo install --replace is distinct from cargo install --update --replace, so it seems fine to have both!

One thought we had was perhaps this could be renamed -f or --force? That follows the convention of many other tools for saying "just do this". Also, from the discussion on #2082 it sounds like the opinions about --update may have changed in light of our current situation, so if you'd like to implement that as well here feel free!

Member

alexcrichton commented Mar 11, 2016

Ok, @wycats and I had a chance to talk about this recently, and this seems like it's desirable as it's pretty orthogonal from the --update flag. Put another way, cargo install --replace is distinct from cargo install --update --replace, so it seems fine to have both!

One thought we had was perhaps this could be renamed -f or --force? That follows the convention of many other tools for saying "just do this". Also, from the discussion on #2082 it sounds like the opinions about --update may have changed in light of our current situation, so if you'd like to implement that as well here feel free!

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Mar 11, 2016

Contributor

Yes --force would work. I had avoided it to sidestep a bigger question of whether cargo should have such a flag and consistency considerations (if install gets it other commands might want to as well).

I'll keep --update in mind but wouldn't do it straight away.

You had mentioned some implementation issues. I'm guessing you don't like the replace (or force) flag being passed as a bool from bin/install.rs?

Contributor

gkoz commented Mar 11, 2016

Yes --force would work. I had avoided it to sidestep a bigger question of whether cargo should have such a flag and consistency considerations (if install gets it other commands might want to as well).

I'll keep --update in mind but wouldn't do it straight away.

You had mentioned some implementation issues. I'm guessing you don't like the replace (or force) flag being passed as a bool from bin/install.rs?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 11, 2016

Member

Ah I believe what I had in mind was the "transactional" nature of the install. Right now the uninstall happens first, but it probably wants to get delayed until right before the actual new binaries are moved into place (either that or nothing is really uninstalled, just metadata is updated).

Member

alexcrichton commented Mar 11, 2016

Ah I believe what I had in mind was the "transactional" nature of the install. Right now the uninstall happens first, but it probably wants to get delayed until right before the actual new binaries are moved into place (either that or nothing is really uninstalled, just metadata is updated).

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Mar 12, 2016

Contributor

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

Contributor

bors commented Mar 12, 2016

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

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 6, 2016

Member

ping @gkoz, any update on this?

Member

alexcrichton commented Apr 6, 2016

ping @gkoz, any update on this?

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Apr 7, 2016

Contributor

Oh wow, it's been a month? I'll revisit this in a few days.

Contributor

gkoz commented Apr 7, 2016

Oh wow, it's been a month? I'll revisit this in a few days.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 8, 2016

Member

Sounds good to me! Feel free to ping me if you need help pushing over the finish line.

Member

alexcrichton commented Apr 8, 2016

Sounds good to me! Feel free to ping me if you need help pushing over the finish line.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Apr 12, 2016

Contributor

Ah I believe what I had in mind was the "transactional" nature of the install. Right now the uninstall happens first, but it probably wants to get delayed until right before the actual new binaries are moved into place (either that or nothing is really uninstalled, just metadata is updated).

To avoid any misunderstanding... The current patch calls uninstall after building but before copying the binaries. If that is too risky (after uninstalling we might find there's not enough space for the new binaries?) I could perhaps create a temporary subdirectory (TempDir::new_in(&dst)), copy new binaries into it and then overwrite the old ones.

Contributor

gkoz commented Apr 12, 2016

Ah I believe what I had in mind was the "transactional" nature of the install. Right now the uninstall happens first, but it probably wants to get delayed until right before the actual new binaries are moved into place (either that or nothing is really uninstalled, just metadata is updated).

To avoid any misunderstanding... The current patch calls uninstall after building but before copying the binaries. If that is too risky (after uninstalling we might find there's not enough space for the new binaries?) I could perhaps create a temporary subdirectory (TempDir::new_in(&dst)), copy new binaries into it and then overwrite the old ones.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 12, 2016

Member

Basically what I'm getting at is two properties:

  1. If a normal error happens for whatever reason, we shouldn't tamper with the install root at all
  2. If ctrl-c happens, the "window of where things are corrupted" should be as small as possible

So given that, the error that I'm worried about here is that we successfully remove something but then hit an I/O error copying in a new file, In that case we won't restore the just-removed binaries. Could this be updated to handle that?

Member

alexcrichton commented Apr 12, 2016

Basically what I'm getting at is two properties:

  1. If a normal error happens for whatever reason, we shouldn't tamper with the install root at all
  2. If ctrl-c happens, the "window of where things are corrupted" should be as small as possible

So given that, the error that I'm worried about here is that we successfully remove something but then hit an I/O error copying in a new file, In that case we won't restore the just-removed binaries. Could this be updated to handle that?

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Apr 15, 2016

Contributor

I've pushed a version, that copies all binaries into a temp subdirectory of the destination and only after that succeeds tries to replace the existing binaries. If overwriting is partially successful, it doesn't try to undo anything (should it? that could further increase complexity) but still updates the metadata.

Another difference from former behavior is (when --force is absent) listing all of the binaries to be overwritten, not just the first one. However the error message still doesn't make it very clear that without --bin the old packages will be removed completely.

Error handling is not ideal here:

  • I'm not sure how to make several lines of "binary ... already exists ..." and the "Add --force to overwrite" hint look nicer,
  • there doesn't seem to be a way to report two unrelated but equally important errors.

Oh and I haven't tested on Windows yet to make sure there's no issue with EXE_SUFFIX.

Contributor

gkoz commented Apr 15, 2016

I've pushed a version, that copies all binaries into a temp subdirectory of the destination and only after that succeeds tries to replace the existing binaries. If overwriting is partially successful, it doesn't try to undo anything (should it? that could further increase complexity) but still updates the metadata.

Another difference from former behavior is (when --force is absent) listing all of the binaries to be overwritten, not just the first one. However the error message still doesn't make it very clear that without --bin the old packages will be removed completely.

Error handling is not ideal here:

  • I'm not sure how to make several lines of "binary ... already exists ..." and the "Add --force to overwrite" hint look nicer,
  • there doesn't seem to be a way to report two unrelated but equally important errors.

Oh and I haven't tested on Windows yet to make sure there's no issue with EXE_SUFFIX.

@gkoz gkoz changed the title from Add `--replace` flag to cargo install to Add `--force` flag to cargo install Apr 15, 2016

src/cargo/ops/cargo_install.rs
+ }
+ }
+ }
+ // If `--bin` or `--example` weren't specified, uninstall obsolete

This comment has been minimized.

@alexcrichton

alexcrichton Apr 15, 2016

Member

This seems a little surprising to me, this is basically saying if package A installed foo and bar, then B installed foo, you'd remove A's bar? I'd probably expect that A's bar stuck around and you could uninstall it with cargo uninstall A

@alexcrichton

alexcrichton Apr 15, 2016

Member

This seems a little surprising to me, this is basically saying if package A installed foo and bar, then B installed foo, you'd remove A's bar? I'd probably expect that A's bar stuck around and you could uninstall it with cargo uninstall A

This comment has been minimized.

@gkoz

gkoz Apr 16, 2016

Contributor

All this time I'd been assuming it's a good idea for cargo install -f foo to completely remove the remnants of the old package by default because that's probably what the user wants. :) If that's not the case I should probably add a hint "run 'cargo uninstall foo:{previous version}' to remove remaining binaries."

@gkoz

gkoz Apr 16, 2016

Contributor

All this time I'd been assuming it's a good idea for cargo install -f foo to completely remove the remnants of the old package by default because that's probably what the user wants. :) If that's not the case I should probably add a hint "run 'cargo uninstall foo:{previous version}' to remove remaining binaries."

This comment has been minimized.

@alexcrichton

alexcrichton Apr 16, 2016

Member

Hm yeah I guess it depends on how you look at it. On one hand you could have installed a package with a ton of binaries, some of which you never used, and then you overwrite one of the never used ones. On the other hand this could also hamstring a package to be useless if it's the one binary that counts.

To me though -f just means "do this anyway" which doesn't necessarily imply "do the extra work of removing everything else", and I like the idea of adding a note that other binaries from the package are lying around and may wish to be removed.

@alexcrichton

alexcrichton Apr 16, 2016

Member

Hm yeah I guess it depends on how you look at it. On one hand you could have installed a package with a ton of binaries, some of which you never used, and then you overwrite one of the never used ones. On the other hand this could also hamstring a package to be useless if it's the one binary that counts.

To me though -f just means "do this anyway" which doesn't necessarily imply "do the extra work of removing everything else", and I like the idea of adding a note that other binaries from the package are lying around and may wish to be removed.

src/cargo/ops/cargo_install.rs
@@ -27,23 +28,12 @@ struct CrateListingV1 {
v1: BTreeMap<PackageId, BTreeSet<String>>,
}
-struct Transaction {

This comment has been minimized.

@alexcrichton

alexcrichton Apr 15, 2016

Member

I think we may still want this transaction type in some form. Before we write out the metadata of new binaries if an error happens we need to remove them. If we overwrite something an then an error happens, it's fine to throw our hands up in the air

@alexcrichton

alexcrichton Apr 15, 2016

Member

I think we may still want this transaction type in some form. Before we write out the metadata of new binaries if an error happens we need to remove them. If we overwrite something an then an error happens, it's fine to throw our hands up in the air

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 15, 2016

Member

Thanks @gkoz! Can you also write some tests for some of the corner cases here? Just to make sure the metadata is managed properly and everything works as expected.

Member

alexcrichton commented Apr 15, 2016

Thanks @gkoz! Can you also write some tests for some of the corner cases here? Just to make sure the metadata is managed properly and everything works as expected.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Apr 26, 2016

Contributor

Still working on adding tests. Wondering how to imitate failure modes.

Want to get this right first, and then add the hint about outdated binaries, possibly in another PR.

Contributor

gkoz commented Apr 26, 2016

Still working on adding tests. Wondering how to imitate failure modes.

Want to get this right first, and then add the hint about outdated binaries, possibly in another PR.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 27, 2016

Member

It's ok to not necessarily exhaustively test them, it's fine to just smoke test a few simple ones like something already exists, a build failure when overwriting, etc.

Member

alexcrichton commented Apr 27, 2016

It's ok to not necessarily exhaustively test them, it's fine to just smoke test a few simple ones like something already exists, a build failure when overwriting, etc.

gkoz added some commits Apr 15, 2016

Add `--force` flag to cargo install
Close #2082

Adding `--force` (`-f`) instructs cargo to overwrite existing binaries
(updating the metadata accordingly).
This allows updating crates via `cargo install -f <crate>`.

Installation happens in two stages now: binaries are copied into a
temporary subdirectory of the destination first, then moved into
destination. This should catch some errors earlier.

In case of installation error cargo will remove new binaries but won't
attempt to undo successful overwrites.
cargo-install: move binaries from the build dir if possible
Try moving the binaries (and fall back to copying) if the build
directory is a temporary one (source isn't a local path).
@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz May 1, 2016

Contributor

Added more tests and an optional optimization in a separate commit.

Contributor

gkoz commented May 1, 2016

Added more tests and an optional optimization in a separate commit.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 2, 2016

Member

@bors: r+ 9f0fa24

Thanks!

Member

alexcrichton commented May 2, 2016

@bors: r+ 9f0fa24

Thanks!

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 2, 2016

Contributor

⌛️ Testing commit 9f0fa24 with merge 401209f...

Contributor

bors commented May 2, 2016

⌛️ Testing commit 9f0fa24 with merge 401209f...

bors added a commit that referenced this pull request May 2, 2016

Auto merge of #2405 - gkoz:reinstall, r=alexcrichton
Add `--force` flag to cargo install

Close #2082.

Adding `--force` (`-f`) instructs cargo to overwrite existing binaries (updating the metadata accordingly).
This allows updating crates via `cargo install -f <crate>`.

Installation happens in two stages now: binaries are copied into a temporary subdirectory of the destination first, then moved into destination. This should catch some errors earlier.

In case of installation error cargo will remove new binaries but won't attempt to undo successful overwrites.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 2, 2016

Contributor

💔 Test failed - cargo-win-msvc-32

Contributor

bors commented May 2, 2016

💔 Test failed - cargo-win-msvc-32

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 2, 2016

Member

@bors: retry

On Mon, May 2, 2016 at 10:21 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - cargo-win-msvc-32
http://buildbot.rust-lang.org/builders/cargo-win-msvc-32/builds/391


You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#2405 (comment)

Member

alexcrichton commented May 2, 2016

@bors: retry

On Mon, May 2, 2016 at 10:21 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - cargo-win-msvc-32
http://buildbot.rust-lang.org/builders/cargo-win-msvc-32/builds/391


You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#2405 (comment)

@bors

This comment has been minimized.

Show comment
Hide comment
Contributor

bors commented May 2, 2016

@bors bors merged commit 9f0fa24 into rust-lang:master May 2, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment