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

warn newer available version of the x tool #104552

Merged
merged 16 commits into from
Jan 3, 2023

Conversation

DebugSteven
Copy link
Contributor

@DebugSteven DebugSteven commented Nov 17, 2022

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 adds a --wrapper-version argument to x to determine the version at runtime, .

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 Nov 17, 2022

r? @jyn514

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

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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 Nov 17, 2022
@Noratrieb
Copy link
Member

I like the idea of having a way to check for a outdated x. This has to be a warning and not a fatal error because that would be too annoying.

I'm not sure whether tidy is the right place for that. Maybe it should live in bootstrap instead? jyn will have better ideas on that hopefully.. :)

@rust-log-analyzer

This comment has been minimized.

jyn514
jyn514 previously requested changes Nov 18, 2022
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether tidy is the right place for that. Maybe it should live in bootstrap instead? jyn will have better ideas on that hopefully.. :)

I think I agree bootstrap is a better place; that way you get warned every time instead of just when you run tidy.

That said if x is working for you there's probably not a hard need to upgrade? so maybe being less spammy is a good thing? don't have super strong opinions here.

src/tools/tidy/src/main.rs Outdated Show resolved Hide resolved
src/tools/tidy/src/lib.rs Outdated Show resolved Hide resolved
src/tools/tidy/src/x.rs Outdated Show resolved Hide resolved
src/tools/tidy/src/x.rs Outdated Show resolved Hide resolved
src/tools/tidy/src/x.rs Outdated Show resolved Hide resolved
@ChrisDenton
Copy link
Member

Hmm, I'll admit I installed x once and it works for me so I've somewhat intentionally avoided the hassle of (potentially breaking) upgrades, or at least I'm lazy enough not to bother. So personally speaking I'd rather not be nagged about this.

On the other hand if it did fail for some reason then having a "hey, you're using an old version" would be useful.

@jyn514 jyn514 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 Nov 19, 2022
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks better, thanks :) I think once you fix my other comments we can merge it.

src/tools/tidy/src/x_version.rs Show resolved Hide resolved
src/tools/tidy/src/x_version.rs Outdated Show resolved Hide resolved
src/tools/tidy/src/x_version.rs Outdated Show resolved Hide resolved
src/tools/x/src/main.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Dec 31, 2022

☔ The latest upstream changes (presumably #105058) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Dec 31, 2022

☔ The latest upstream changes (presumably #106324) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot
Copy link
Collaborator

rustbot commented Dec 31, 2022

Some changes occurred in src/tools/cargo

cc @ehuss

@DebugSteven
Copy link
Contributor Author

@ehuss You can disregard this. I made a mistake in rebasing. There are no changes to src/tools/cargo.

src/tools/tidy/src/x_version.rs Outdated Show resolved Hide resolved
src/tools/tidy/src/x_version.rs Outdated Show resolved Hide resolved
src/tools/tidy/src/x_version.rs Outdated Show resolved Hide resolved
DebugSteven and others added 3 commits January 1, 2023 14:34
Co-authored-by: Joshua Nelson <github@jyn.dev>
Co-authored-by: Joshua Nelson <github@jyn.dev>

// Parse latest version out of `x` Cargo.toml
fn get_x_wrapper_version() -> Option<Version> {
let cmd = Command::new("cargo")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know this will be run from the root of the rust-lang/rust checkout?
I tested this by being in some random directory (inside the project) & it did use the root, but can we count on that or should the directory be set?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can test by running ../../../x.py test tidy from src/tools/x, which isn't in the workspace. If that works, and rust/x.py test tidy works (from outside the source directory), this is fine :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err sorry - actually can you use cargo_metadata, like in

/// `root` is path to the directory with the root `Cargo.toml` (for the workspace). `cargo` is path
/// to the cargo executable.
pub fn check(root: &Path, cargo: &Path, bad: &mut bool) {
let mut cmd = cargo_metadata::MetadataCommand::new();
cmd.cargo_path(cargo)
.manifest_path(root.join("Cargo.toml"))
.features(cargo_metadata::CargoOpt::AllFeatures);
let metadata = t!(cmd.exec());
? that takes root as an argument which avoids this question altogether.

Copy link
Contributor Author

@DebugSteven DebugSteven Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running both of the above commands didn't end up working for me, even with the new change to use cargo_metadata.

I end up getting tidy errors for autogenerated files in the build directory... This seems like a separate issue though.

Building rustbuild
    Finished dev [unoptimized] target(s) in 0.08s
Building stage0 tool tidy (aarch64-apple-darwin)
   Compiling tidy v0.1.0 (/Users/jhaigh/developer/Rust/rust/src/tools/tidy)
    Finished release [optimized] target(s) in 2.62s
tidy check
* 633 error codes
* highest error code: E0791
tidy error: /Users/jhaigh/developer/Rust/rust/src/tools/x/build/bootstrap/debug/build/typenum-
949154f977098d36/out/consts.rs: leading newline
tidy error: /Users/jhaigh/developer/Rust/rust/src/tools/x/build/bootstrap/debug/build/typenum-
949154f977098d36/out/consts.rs: missing trailing newline
tidy error: /Users/jhaigh/developer/Rust/rust/src/tools/x/build/bootstrap/debug/build/typenum-
949154f977098d36/out/tests.rs: leading newline
tidy error: /Users/jhaigh/developer/Rust/rust/src/tools/x/build/bootstrap/debug/build/typenum-
949154f977098d36/out/tests.rs: missing trailing newline
tidy error: /Users/jhaigh/developer/Rust/rust/src/tools/x/build/bootstrap/debug/build/typenum-
949154f977098d36/out/tests.rs: too many lines (17255) (add `// ignore-tidy-filelength` to the 
file to suppress this error)
tidy error: /Users/jhaigh/developer/Rust/rust/src/tools/x/build/bootstrap/debug/build/typenum-
949154f977098d36/out/op.rs: leading newline
tidy error: /Users/jhaigh/developer/Rust/rust/src/tools/x/build/bootstrap/debug/build/typenum-
949154f977098d36/out/op.rs: missing trailing newline
tidy error: /Users/jhaigh/developer/Rust/rust/src/tools/x/build/aarch64-apple-darwin/stage0/sh
are/doc/cargo/README.md: too many trailing newlines (2)
tidy error: /Users/jhaigh/developer/Rust/rust/src/tools/x/build/aarch64-apple-darwin/stage0/sh
are/doc/rustfmt/README.md:108: trailing whitespace
* 392 features
some tidy checks failed
Build completed unsuccessfully in 0:00:05

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yeah that's from when I asked you to test ../../../x test tidy, sorry about that 😅 rm -rf src/tools/x/build will make the errors go away (although I've been wanting to get tidy to ignore untracked files for a while; but that's a separate PR.)

Co-authored-by: Joshua Nelson <github@jyn.dev>
@jyn514 jyn514 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 3, 2023
@jyn514
Copy link
Member

jyn514 commented Jan 3, 2023

@bors r+ rollup

Thanks!

@bors
Copy link
Contributor

bors commented Jan 3, 2023

📌 Commit 85f649f has been approved by jyn514

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 3, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 3, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#104552 (warn newer available version of the x tool)
 - rust-lang#105681 (some fixes/improvements to mir::visit module)
 - rust-lang#106005 (Test the borrowck behavior of if-let guards)
 - rust-lang#106356 (clean: Remove `ctor_kind` from `VariantStruct`.)
 - rust-lang#106365 (Grammar : Missing "is" in format specifier diagnostic)
 - rust-lang#106388 (rustdoc: remove legacy box-sizing CSS)
 - rust-lang#106392 (`has_overflow` only if value is *not* within limit)
 - rust-lang#106402 (Fix dupe word typos)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 258a0fb into rust-lang:master Jan 3, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 3, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 12, 2023
…er-x, r=jyn514

Revert "warn newer available version of the x tool"

Reverts rust-lang#104552

Running the x executable directly created an [issue](rust-lang#106469) here. There are other options for warning a user that a newer version of x exists in the issue's discussion as well.

r? `@jyn514`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jan 12, 2023
…er-x, r=jyn514

Revert "warn newer available version of the x tool"

Reverts rust-lang#104552

Running the x executable directly created an [issue](rust-lang#106469) here. There are other options for warning a user that a newer version of x exists in the issue's discussion as well.

r? ``@jyn514``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants