Skip to content

Update is happening even if user has a tool and that tool is missing #1486

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

Closed
nrc opened this issue Aug 24, 2018 · 9 comments
Closed

Update is happening even if user has a tool and that tool is missing #1486

nrc opened this issue Aug 24, 2018 · 9 comments
Labels

Comments

@nrc
Copy link
Member

nrc commented Aug 24, 2018

This should require --force.

cc rust-lang/rls#641

@nrc nrc added the bug label Aug 24, 2018
@kennytm
Copy link
Member

kennytm commented Aug 24, 2018

We need to block nightly on RLS etc. again if rustup cannot prevent uninstall of manually added components.

(cc @rust-lang-nursery/infra) (P.S. we need to sync the team members between the two orgs...)

@pietroalbini
Copy link
Member

The generated manifests when rls is missing are wrong.

The rustup behavior was changed in #1419 to handle removed components (when the docs for some tier 2 platforms were removed and users weren't able to update), but the old behavior is still present when the component is marked as available = false in the manifest.

Currently there is no mention of rls in the nightly manifest at all. The easiest way to fix is to keep rls in the manifest but mark it as unavailable, and this doesn't require any rustup update.

bors added a commit to rust-lang/rust that referenced this issue Aug 26, 2018
…excrichton

Include missing tools in the manifest and mark them as unavailable

This PR changes the `build-manifest` tool to always include the missing components in the manifest, marking them as `available = false`. This blocks rustup from updating to a different nightly if the component is installed.

The code builds and _should_ be correct, but I don't know a way to test the changes locally.

r? @alexcrichton
cc @kennytm rust-lang/rustup#1486
@nrc
Copy link
Member Author

nrc commented Aug 27, 2018

This should be fixed with @pietroalbini's PR

@nrc nrc closed this as completed Aug 27, 2018
@sfackler
Copy link
Member

When will that fix propagate out? The 2018-08-29 nightly still upgraded and warned about missing rls-preview and clippy-preview components.

@nrc
Copy link
Member Author

nrc commented Aug 29, 2018

It should have worked today :-(

@nrc nrc reopened this Aug 29, 2018
@nrc
Copy link
Member Author

nrc commented Aug 29, 2018

In today's manifest, rls and Clippy have available = false as expected, so I guess there must be a problem with Rustup

@pietroalbini
Copy link
Member

pietroalbini commented Aug 29, 2018

Nope, experimented with this for a bit locally, and it's still a manifest problem. While the components themselves are available = false, they're still missing from the rust package. If we add to the manifest this snippet (for example) rustup behaves correctly for clippy on linux and prevents the download:

[[pkg.rust.target.x86_64-unknown-linux-gnu.components]]
pkg = "clippy-preview"
target = "x86_64-unknown-linux-gnu"

screenshot from 2018-08-29 09-32-41

The error messages are still really bad, but at least we don't upgrade people with components installed. I'll look into sending a PR to rustc soon to tweak manifest generation again.

bors added a commit to rust-lang/rust that referenced this issue Aug 30, 2018
Fix manifests for broken tools: take 2

This is a follow up of #53715, to avoid stripping unavailable components from the extensions list. This time I also figured out how to test the changes, so the produced manifest is correct.

Along with the fix I added a README with instructions on how to test the tool, and a new `BUILD_MANIFEST_DISABLE_SIGNING` env var to avoid dealing with gpg while testing the tool. I chose an env var instead of a flag because it's more difficult to have it slip in by accident on CI, and there is also another protection that panics if that env var is set on CI, just to be sure we don't release unsigned artifacts.

r? @alexcrichton
cc rust-lang/rustup#1486
@pietroalbini
Copy link
Member

Ok, this should be fixed now.

@nrc
Copy link
Member Author

nrc commented Aug 30, 2018

Awesome, thank you!

@nrc nrc closed this as completed Aug 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants