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.

Sign up for GitHub

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

Jump to bottom

check for x version updates #107048

Merged
merged 1 commit into from
Jan 21, 2023
Merged

check for x version updates #107048

merged 1 commit into from
Jan 21, 2023

Conversation

DebugSteven
Copy link
Contributor

This PR adds a check to tidy to assert that the installed version of x is equal to the version in src/tools/x/Cargo.toml. It checks the installed version of x by parsing the output of cargo install --list (as an option proposed in this issue).

It does not warn if x has not yet been installed, on the assumption that the user isn't interested in using it.

@rustbot
Copy link
Collaborator

rustbot commented Jan 18, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 18, 2023
@albertlarsan68
Copy link
Member

The code itself works, but it is never called.
Also, you are missing the semver dependency.

Here is the patch that got me to get it to work: https://gist.github.com/albertlarsan68/bd298307c84efbfcc2ee2edfd50b2ba5
r? @albertlarsan68
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 19, 2023
@DebugSteven
Copy link
Contributor Author

Hey, thank you for catching that. This commit was originally continuing off of this branch & I forgot to pull over the rest of the changes. There was one other change I made on that branch that I moved over as well, but otherwise I did almost exactly what you wrote. I previously tested this change by running x test tidy & also made sure that it worked with an arbitrary x executable, in the output of cargo install --list, that didn’t have the src/tools/x path.

For the other change I moved over, the check macro has a leading comma if you don’t have any path args. I ran into this because at one point I didn’t need any of the paths & tried to call check!(x_version). I believe it expanded to check!(, x_version) & error’d out because of the syntax. I’d need to reproduce it to double check. Let me know if you’d like me to squash these commits or have a separate PR for the macro change.

@DebugSteven
Copy link
Contributor Author

One remaining concern I have is: should any of this code be using tidy_error? I’m thinking about the feedback in this issue that tidy shouldn’t emit an error if it sees old versions of x. I think it would make sense to throw an error if cargo fails to run, but would it be more appropriate to warn a user with println for the other errors here?

@albertlarsan68
Copy link
Member

Thanks for your PR!
If you can extract the macro change into another PR and assign it to me, it would be great.
I think squashing the 2 first commits is also a good idea.
Post @rustbot ready when it is done.

@DebugSteven
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 20, 2023
@albertlarsan68
Copy link
Member

Thanks!
@bors r+ rollup=iffy (we may want to revert it)

@bors
Copy link
Contributor

bors commented Jan 20, 2023

📌 Commit 540ca2f has been approved by albertlarsan68

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 20, 2023
@compiler-errors
Copy link
Member

@albertlarsan68, for reference, rollup=iffy is used when the PR has a risk of failing in CI. It doesn't affect whether or not we can revert the PR in the future.

I think this can be marked as maybe.
@bors rollup=maybe

@compiler-errors compiler-errors mentioned this pull request Jan 21, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 21, 2023
@bors
Auto merge of rust-lang#107143 - compiler-errors:rollup-zabvmo5, r=co…
e098eb1 
…mpiler-errors

Rollup of 9 pull requests

Successful merges:

 - rust-lang#104154 (Change `bindings_with_variant_name` to deny-by-default)
 - rust-lang#104347 (diagnostics: suggest changing `s@self::{macro}`@::macro`` for exported)
 - rust-lang#104672 (Unify stable and unstable sort implementations in same core module)
 - rust-lang#107048 (check for x version updates)
 - rust-lang#107061 (Implement some more new solver candidates and fix some bugs)
 - rust-lang#107095 (rustdoc: remove redundant CSS selector `.sidebar .current`)
 - rust-lang#107112 (Fix typo in opaque_types.rs)
 - rust-lang#107124 (fix check macro expansion)
 - rust-lang#107131 (rustdoc: use CSS inline layout for radio line instead of flexbox)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d26c88c into rust-lang:master Jan 21, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 21, 2023
@jyn514 jyn514 mentioned this pull request Feb 5, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@albertlarsan68 albertlarsan68

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.68.0
Development

Successfully merging this pull request may close these issues.
6 participants
@DebugSteven @rustbot @albertlarsan68 @bors @compiler-errors @Mark-Simulacrum