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

Only check the active toolchain if rustup is installed #38

Merged
merged 5 commits into from
Jun 8, 2023

Conversation

ntrinquier
Copy link
Contributor

No description provided.

@j-baker
Copy link
Contributor

j-baker commented Jun 8, 2023

I don't think this is quite right. I expect the right solution for this problem is to do something like run rustc --version and check that the version string is a nightly release, if you want to ensure that everything runs with nightly.

It also still checks that all compilation occurs with nightly if one does have rustup, I think? Whereas I think the +nightly will dynamically enable it for the command, so we only really want to check if nightly is installed, not that it's active.

If one were checkign with rustup, I'd expect that the right way would be to list the installed toolchains and check that nightly is in the list?

@ntrinquier
Copy link
Contributor Author

ntrinquier commented Jun 8, 2023

I don't think this is quite right. I expect the right solution for this problem is to do something like run rustc --version and check that the version string is a nightly release?

It also still checks that all compilation occurs with nightly, I think?

This only affects cargo fmt, not the compilation (and now that we made it opt-out, only the user specified nightly=True)

@j-baker
Copy link
Contributor

j-baker commented Jun 8, 2023

Won't rustup show active-toolchain return nightly if rustc is going to compile with nightly? Or am I misunderstanding what that command returns?

@ntrinquier
Copy link
Contributor Author

I think the +nightly will dynamically enable it for the command, so we only really want to check if nightly is installed, not that it's active.

yeah that's fair

@ntrinquier ntrinquier merged commit 11d8f11 into develop Jun 8, 2023
8 checks passed
@ntrinquier ntrinquier deleted the nvt/check-rustup branch August 3, 2023 22:42
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 this pull request may close these issues.

None yet

3 participants