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

Avoid calling rustup in tests #4218

Closed
oxalica opened this issue Apr 30, 2020 · 10 comments · Fixed by #4219
Closed

Avoid calling rustup in tests #4218

oxalica opened this issue Apr 30, 2020 · 10 comments · Fixed by #4219

Comments

@oxalica
Copy link
Contributor

oxalica commented Apr 30, 2020

I'm on NixOS and use nixpkgs-mozilla to setup cargo, rustc and other components, without rustup.

cargo test in rust-analyzer results in 3 failure for me:

  • cli::check_code_formatting
  • cli::generated_assists_are_fresh
  • cli::generated_grammar_is_fresh

They are all trying to invoke rustup run stable -- ... and then rustup toolchain install stable. Obviously, they failed because there is no rustup.
But I already have cargo and rustfmt installed!

Why not invoking cargo ... or rustfmt ... directly? Since we are expecting stable, I think it should not lead to problem even if nightly components are invoked.

@lnicola
Copy link
Member

lnicola commented Apr 30, 2020

See also #3243 for a fun issue (the tests run in parallel, but rustup is racy). It's supposed to make things easier for contributors, but I suspect that a "please install the stable toolchain" panic would be nicer.

Why not invoking cargo ... or rustfmt ... directly? Since we are expecting stable, I think it should not lead to problem even if nightly components are invoked.

It can. I use nightly and there were formatting differences between nightly and stable rustfmt (because of bugs, nightly was clearly better). So I ran cargo fmt before commit, but CI failed with a nonsensical formatting diff.

EDIT: but yes, running cargo +stable instead of rustup run stable -- cargo might help with your issue.

@bjorn3
Copy link
Member

bjorn3 commented Apr 30, 2020

cargo +stable is also a rustup feature.

@oxalica
Copy link
Contributor Author

oxalica commented Apr 30, 2020

@lnicola

See also #3243 for a fun issue.

I also think installing toolchains/components during tests is a bad idea due to the side effects.
Panicking should be better.

We can simply check to refuse nightly rustfmt and directly invoke rustfmt/cargo in tests.

> nix run nixpkgs.latest.rustChannels.nightly.rustfmt-preview -c rustfmt --version
rustfmt 1.4.14-nightly (a5cb5d2 2020-04-14)
> nix run nixpkgs.latest.rustChannels.stable.rustfmt-preview -c rustfmt --version
rustfmt 1.4.12-stable (a828ffe 2020-03-11)

If this is acceptable, I'm willing to implement it.

@flodiebold
Copy link
Member

Just running rustfmt --version would break the tests for users who have stable installed, but nightly as default. That could be solved by a rust-toolchain file though, I guess 🤔

@oxalica
Copy link
Contributor Author

oxalica commented Apr 30, 2020

@flodiebold
How about setting RUSTUP_TOOLCHAIN=stable to run cargo fmt? This works in both rustup and bare installations (the latter one ignores it, so we still need version check).

@flodiebold
Copy link
Member

Sounds plausible...

@flodiebold
Copy link
Member

Regarding the installation, my guess is it's a holdover from when we were using a specific beta version for formatting instead of just stable. For stable, it seems fine to me to bail if it's not installed...

@polyzen
Copy link
Contributor

polyzen commented May 8, 2020

Still getting the test failures with rust-analyzer 2020-05-04 and rustfmt provided by the Arch Linux rust package:

> rustfmt --version
rustfmt 1.4.12-
thread 'cli::check_code_formatting' panicked at 'Failed to run rustfmt from toolchain 'stable'. ...

@polyzen
Copy link
Contributor

polyzen commented Jul 6, 2020

Bump @oxalica @matklad

@matklad
Copy link
Member

matklad commented Jul 6, 2020

@polyzen rust-analyzer test suite is guaranteed to work only in rust-analyzer repo, we ship the binary, we don't ship test suite. PRs to improve testing in downstreams packagers are welcome though.

archlinux-github pushed a commit to archlinux/svntogit-community that referenced this issue Sep 8, 2020
rust-lang/rust-analyzer#4218 (comment)

git-svn-id: file:///srv/repos/svn-community/svn@700626 9fca08f4-af9d-4005-b8df-a31f2cc04f65
archlinux-github pushed a commit to archlinux/svntogit-community that referenced this issue Sep 8, 2020
rust-lang/rust-analyzer#4218 (comment)


git-svn-id: file:///srv/repos/svn-community/svn@700626 9fca08f4-af9d-4005-b8df-a31f2cc04f65
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants