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

Generated rustfmt/cargo-fmt shims may not work? #1305

Open
alexcrichton opened this Issue Dec 8, 2017 · 9 comments

Comments

5 participants
@alexcrichton
Member

alexcrichton commented Dec 8, 2017

I got rustup to self-update to 1.8.0 (from the dev archives) and correctly got:

warning: tool `rustfmt` is already installed, remove it from `/home/alex/.cargo/bin`, then run `rustup update` to have rustup manage this tool.
warning: tool `cargo-fmt` is already installed, remove it from `/home/alex/.cargo/bin`, then run `rustup update` to have rustup manage this tool.

I then ran cargo uninstall rustfmt, which removed the bins, but no amount of rustup update seems to be causing the files to come into existence.

I think that these tools only come into existence during a self update?

cc @nrc

@nrc

This comment has been minimized.

Contributor

nrc commented Dec 11, 2017

Yeah, dammit, we require a self update, but running rustup update won't do that. I wonder if it is enough to change the warning message to recommend rustup self update rather than rustup update or whether since there is nothing to update, it will skip the self update any way?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 11, 2017

Yeah I think I tried the self update and that didn't work as well (as it didn't do anything :( )

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Dec 14, 2017

I have a patch that I think fixes this, but I didn’t manage to write a test that fails on master. This passes:

diff --git a/tests/cli-self-upd.rs b/tests/cli-self-upd.rs
index c8bc9c4b..3a24f93a 100644
--- a/tests/cli-self-upd.rs
+++ b/tests/cli-self-upd.rs
@@ -1273,5 +1273,12 @@ fn update_does_not_overwrite_rustfmt() {
                          "`rustfmt` is already installed");
         assert!(rustfmt_path.exists());
         assert_eq!(utils::file_size(rustfmt_path).unwrap(), 0);
+
+        // Remove the file as suggested by the error "already installed" error message.
+        utils::remove_file("rustfmt", rustfmt_path).unwrap();
+        assert!(!rustfmt_path.exists());
+
+        expect_ok(config, &["rustup", "self", "update"]);
+        assert!(rustfmt_path.exists());
     });
 }
@nrc

This comment has been minimized.

Contributor

nrc commented Dec 14, 2017

@SimonSapin did you confirm that doing rustup self update fails in real life? I would expect that test to fail

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Dec 14, 2017

“Real” rustup self update does nothing when the current version is the latest. Are the tests not running the same binary?

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Dec 14, 2017

#1310 hopefully fixes this.

SimonSapin added a commit to SimonSapin/rustup.rs that referenced this issue Dec 16, 2017

SimonSapin added a commit to SimonSapin/rustup.rs that referenced this issue Dec 16, 2017

@djc

This comment has been minimized.

djc commented Jan 9, 2018

This bug still seems to be there (I ran into this twice over the past few weeks, on different machines) with rustup-1.9.0, is that expected?

@boxofrox

This comment has been minimized.

boxofrox commented Feb 16, 2018

I can confirm rustup self update fixed the missing rustfmt shims in my situation.

Terminal commands I ran to update rustc stable and fix broken rustfmt shims (click to expand)
$ rustup self update
info: checking for self-updates
info: downloading self-update
info: rustup updated successfully to 1.11.0
warning: tool `rustfmt` is already installed, remove it from `/home/boxofrox/.cargo/bin`, then run `rustup update` to have rustup manage this tool.
warning: tool `cargo-fmt` is already installed, remove it from `/home/boxofrox/.cargo/bin`, then run `rustup update` to have rustup manage this tool.

$ rustup update stable
info: syncing channel updates for 'stable-x86_64-unknown-linux-gnu'
info: latest update on 2018-02-15, rust version 1.24.0 (4d90ac38c 2018-02-12)
info: downloading component 'rustc'
 47.8 MiB /  47.8 MiB (100 %)  16.6 MiB/s ETA:   0 s
info: downloading component 'rust-std'
 67.2 MiB /  67.2 MiB (100 %)  18.8 MiB/s ETA:   0 s
info: downloading component 'cargo'
info: downloading component 'rust-docs'
  4.6 MiB /   4.6 MiB (100 %)   3.5 MiB/s ETA:   0 s
info: downloading component 'rust-src'
info: removing component 'rustc'
info: removing component 'rust-std'
info: removing component 'cargo'
info: removing component 'rust-docs'
info: removing component 'rust-src'
info: installing component 'rustc'
info: installing component 'rust-std'
info: installing component 'cargo'
info: installing component 'rust-docs'
info: installing component 'rust-src'

  stable-x86_64-unknown-linux-gnu updated - rustc 1.24.0 (4d90ac38c 2018-02-12)

$ cargo uninstall rustfmt-nightly
    Removing /home/boxofrox/.cargo/bin/cargo-fmt
    Removing /home/boxofrox/.cargo/bin/git-rustfmt
    Removing /home/boxofrox/.cargo/bin/rustfmt
    Removing /home/boxofrox/.cargo/bin/rustfmt-format-diff

$ rustup component add rustfmt-preview
info: downloading component 'rustfmt-preview'
info: installing component 'rustfmt-preview'

$ which rustfmt
rustfmt not found

$ rustup self update
info: checking for self-updates

$ which rustfmt
/home/boxofrox/.cargo/bin/rustfmt

@nrc

This comment has been minimized.

Contributor

nrc commented Feb 16, 2018

I assume that if you have a really old version of rustup and don't rustup self update you won't get the fix and thus will still have problems, but that should be fixed by self-updating.

mattico added a commit to mattico/rustup.rs that referenced this issue Apr 5, 2018

@Diggsey Diggsey added this to Proxying & telemetry in Issue Categorisation May 30, 2018

@ordian ordian referenced this issue Jun 21, 2018

Merged

Release 0.3.0 #215

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment