Warn/error when cargo installing the cwd in 2015/2018

[dwijnand]

Fixes #5327

submitted for early review. feedback very welcome, I'm happy to iterate (and learn).

[rust-highfive]

r? @matklad

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

[dwijnand]

my test is failing (on AppVeyor) on nightly. I thought something was wrong with my local setup. is nightly Rust meant to be edition 2018?

[ehuss]

is nightly Rust meant to be edition 2018?

No, you have to opt-in to an edition, even on nightly. See unstable docs, Cargo.toml needs a rust key and cargo-features since editions are unstable.

[dwijnand]

I see! thanks @ehuss

that seems to have fixed the test for me locally.

[dwijnand]

I had tried another encoding to preserve the knowledge that the package source was from the cwd, so I'd be interested to know if there's a better approach that what I went with.

[matklad]

Oh no, I was sure that one can just handle this at the argument parsing level, but of course one needs to load a manifest to figure out which edition we are in....

I think a slightly better approach to thread the cwd information, instead of adding a specialized field to the SourceId, which is a core data-structure and is used everywhere, is to add an argument to the install and install_one functions.

[dwijnand]

of course! I like it.

[matklad]

LGTM!

Anyone, and @rust-lang/cargo specifically, if you have any objections, please speak up! We've discussed this change at yesterday's meeting, but not everyone was present, and this change could potentially upset some people, so I'd rather double-check with you all :)

The TL;DR here is that we deprecate cargo install in favor of cargo install --path ., because the first form might be wrongly used instead of cargo build, especially if you come from Ruby or JS ecosystems, where bundler install and npm install do completely different things. At the same time, installing the local crate seems to be an extremely niche use-case. So it's better to require slightly more verbose syntax for niche use-case, to be able to provide a targeted warning for common miss use.

@bors r+

[bors]

📌 Commit d678605 has been approved by matklad

[bors]

⌛ Testing commit d678605 with merge 552afa0342ea53caa0a5741d7e4060be6c1e0d5e...

[dwijnand]

So it's better to require slightly more verbose syntax for niche use-case, to be able to provide a targeted warning for common miss use.

👍 the https://en.wikipedia.org/wiki/Pareto_principle (iiuc)

[withoutboats]

I'm a bit confused by this. A warning isn't a breaking change, why is this gated on 2018?

I would expect this to warn in 2015 and stop working in 2018 if we were doing anything connected to the edition change.

[matklad]

@bors r-

[matklad]

It could work either way, we can start warning in this edition, and remove in 2018, or we can start warning in 2018, and remove in the next edition.

If we start to warn today, that risks causing a few warnings, if someone uses cargo install in some scripts. If we start to warn in 2018, than those warnings will appear only opt-in into the new edition.

The letter seems slightly less invasive, but now that I think of it, I slightly prefer just to warn regardless of the edition: the implementation would be simpler, and presumably only few scripts are doing cargo install?

[bors]

☀️ Test successful - status-appveyor, status-travis
State: approved= try=False

[dwijnand]

and presumably only few scripts are doing cargo install?

indeed. how many scripts are there that call cargo install (n.b not cargo install <some crates.io package>) and would be negatively affected by a new warning message?

[alexcrichton]

I agree that if we warn only in 2018 then we can't actually remove it until the next edition, and I'd personally be ok starting to warn in the 2015 edition and we can evaluate whether it's reasonable to remove from the 2018 edition

[dwijnand]

we can evaluate whether it's reasonable to remove from the 2018 edition

pushed a commit to aid evaluation 😄

[dwijnand]

(btw happy to cleanup the git history if you'd like)

[dwijnand]

note I had to adapt 3 tests, which I think is fine seeing as it's fair to assume that cargo's test suite is bound to be more likely to be sensitive to the sudden appearance of new warnings.

[matklad]

@bors r+

Thanks @dwijnand !

[bors]

📌 Commit d8f7828 has been approved by matklad

[bors]

⌛ Testing commit d8f7828 with merge b0d65af...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing b0d65af to master...