-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Impl warn for locked install without Cargo.lock #9108
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
Thanks for the PR, but this is a new feature of Cargo which unfortunately isn't following our guidelines on adding new features. Existence of an issue isn't typically enough to warrant a PR to change Cargo, but rather typically discussion is needed first. We've explicitly decided in the past to not warn about this, for example, but there hasn't even been an opportunity to discuss this before this PR was created. |
I completely understand. Apologies. I'll comment on the issue whether this decision that you took stills being the same. If that's the case, I'll close the PR. If otherways you consider that it should be closed now, feel free to do so. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship it.
Gah sorry I completely misinterpreted this PR. This is a much more minor feature than I thought so this should be fine to land. I agree this is reasonable to land, and it just looks like there's some test fixes that need to happen. |
@alexcrichton Now that we agree on the landing part, I asked in the PR a question:
This is the help I need in order to understand whether or not the testing has to go on the direction I'm moving towards. |
The For testing that For this PR, I think you can just take out the call to |
Apologies again, my reading skills clearly need to improve... |
No worries at all @alexcrichton ! The PR is ready for review btw! 😃 |
If we're installing in --locked mode and there's no `Cargo.lock` published ie. the bin was published before rust-lang#7026 the cargo install errors were not stating that it was due to the lack of the `Cargo.lock` file. Instead, the error seemed completely unrelated. Therefore, this tries to address this by adding a warn in the stderr output. Closes rust-lang#9106
📌 Commit b526fad has been approved by |
☀️ Test successful - checks-actions |
Update cargo 5 commits in c3abcfe8a75901c7c701557a728941e8fb19399e..e099df243bb2495b9b197f79c19f124032b1e778 2021-01-25 16:16:43 +0000 to 2021-02-01 16:24:34 +0000 - Impl warn for locked install without Cargo.lock (rust-lang/cargo#9108) - Document -Z extra-link-arg. (rust-lang/cargo#9121) - Flip 'foo' and 'bar' to be consistent (rust-lang/cargo#9120) - Don't try to parse MSRV if feature is not enabled (rust-lang/cargo#9115) - simplify char range check (rust-lang/cargo#9110)
If we're installing in --locked mode and there's no
Cargo.lock
publishedie. the bin was published before #7026
the cargo install errors were not stating that it was due to the lack of
the
Cargo.lock
file. Instead, the error seemed completely unrelated.Therefore, this tries to address this by adding a warn in the stderr
output.
Closes #9106
I will need some help on the testing side (assuming the code I added for the warning is correct).
It looks to me that the publish function implemented for testing purposes does not publish
Cargo.lock
which is the actual convention. Should this be updated too? See #7026