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 ensure option to cargo install #6595

Closed

Conversation

k-nasa
Copy link
Contributor

@k-nasa k-nasa commented Jan 24, 2019

Description

Add 'ensure'opsion to cargo_install
Options to prevent errors when crate is already installed.

Background

#6485

I think that this issue is made up of two things.

  • If it is already installed, it will terminate normally
  • Check for the same thing before downloading

This PR solves the first one.
I'll add the second correspondence later.

I ended up using std :: process :: exit. I would like to ask a review if this is OK

@rust-highfive
Copy link

r? @nrc

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

@nrc
Copy link
Member

nrc commented Jan 24, 2019

Thanks for the PR! I think that this should be the default behaviour and the current behaviour is wrong. I would like to change the behaviour and not require the --ensure flag (which I think would be an easy change to this PR). However, we should check with the whole team.

A wrinkle is that the current behaviour is specified by an RFC, so technically we should have an RFC to amend that. However, that seems like unnecessary process to me, but if anyone thinks we should please register a concern.

I note that changing the current behaviour is not backwards incompatible (technically) because it only affects situations which would give an error today.

So, @rfcbot fcp merge and to be precise, I'm proposing that cargo install with an already installed crate should do nothing if it is up to date and reinstall if it is not.

cc #2082

@nrc nrc added the T-cargo Team: Cargo label Jan 24, 2019
@nrc
Copy link
Member

nrc commented Jan 24, 2019

Trying again...

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 24, 2019

Team member @nrc has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@ehuss
Copy link
Contributor

ehuss commented Jan 24, 2019

I think there might be some edge cases to consider, but I think those can be tackled later, or during implementation/PR review. Just off the top of my head:

  • cargo install --path . once will install your crate. Then you make some modifications. Then run cargo install --path . again. What should it do? Since the version hasn't changed, there's no way to detect that it should be reinstalled. I'm not sure how to resolve that. (Treat --path differently? Keep a fingerprint in .crates.toml?).
  • What should cargo install foo then cargo install foo --features fname do? From a high level I would expect it to rebuild and reinstall. However, .crates.toml doesn't track features. Maybe another point for keeping some sort of fingerprint?

@withoutboats
Copy link
Contributor

@ehuss in both those examples, isnt the current behavior to error? It does seem like some scenarios we should not require --forceto reinstall, but currently we always do right? This is just changing it to not exit with an error code. Perhaps another issue should be opened to consider changing those behaviors as well.

@ehuss
Copy link
Contributor

ehuss commented Jan 24, 2019

Nick is proposing doing more than just changing the error code. "cargo install with an already installed crate should do nothing if it is up to date and reinstall if it is not". Maybe a little confusing, it is a different proposal from this PR. cargo install currently has no concept of "up to date". This PR takes the basic step of checking the package version, but there's more to "being up to date" than just the version. I would prefer if the above examples do not "do nothing" (which would happen if only the version is checked), since that might be surprising.

@alexcrichton
Copy link
Member

@rfcbot concern up-to-date-is-hard

Just gonna register @ehuss's concern, I agree that's something which should be worked out before we switch the behavior.

@k-nasa k-nasa force-pushed the feature_ensure_option_to_cargo_install branch from b4988f8 to 5544814 Compare January 27, 2019 13:28
@dwijnand
Copy link
Member

@rfcbot concern interaction-with-CARGO_INCREMENTAL=1

#4255 ("cargo install cargo-outdated fails with CARGO_INCREMENTAL=1") popped up in today's CodeTriage email. Seemed like a potential point of concern with #6564 also temporarily on. What do people think?

@k-nasa
Copy link
Contributor Author

k-nasa commented Feb 14, 2019

It seems that there was a little time, this is still under discussion, is not it?

@ehuss
Copy link
Contributor

ehuss commented Feb 14, 2019

@k-nasa I just posted a proposal on how to move forward on this at #6667. Feel free to leave any comments there, and if everyone agrees on a plan, if you are interested in working on it.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Feb 22, 2019
@ehuss ehuss added the S-blocked label Mar 7, 2019
@bors
Copy link
Collaborator

bors commented Apr 5, 2019

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

@alexcrichton
Copy link
Member

I believe that #6798 was intended to subsume this, so closing.

@rfcbot rfcbot removed the proposed-final-comment-period An FCP proposal has started, but not yet signed off. label Apr 26, 2019
@rfcbot rfcbot removed the disposition-merge FCP with intent to merge label Apr 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-cargo Team: Cargo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants