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
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4805,18 +4805,18 @@ checksum = "1ef965a420fe14fdac7dd018862966a4c14094f900e1650bbc71ddd7d580c8af"

[[package]]
name = "semver"
version = "1.0.12"
version = "1.0.14"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "a2333e6df6d6598f2b1974829f853c2b4c5f4a6e503c10af918081aa6f8564e1"
checksum = "e25dfac463d778e353db5be2449d1cce89bd6fd23c9f1ea21310ce6e5a1b29c4"
dependencies = [
"serde",
]

[[package]]
name = "serde"
version = "1.0.147"
version = "1.0.152"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "d193d69bae983fc11a79df82342761dfbf28a99fc8d203dca4c3c1b590948965"
checksum = "bb7d1f0d3021d347a83e556fc4683dea2ea09d87bccdf88ff5c12545d89d5efb"
dependencies = [
"serde_derive",
]
Expand All @@ -4833,9 +4833,9 @@ dependencies = [

[[package]]
name = "serde_derive"
version = "1.0.147"
version = "1.0.152"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "4f1d362ca8fc9c3e3a7484440752472d68a6caa98f1ab81d99b5dfe517cec852"
checksum = "af487d118eecd09402d70a5d72551860e788df87b464af30e5ea6a38c75c541e"
dependencies = [
"proc-macro2",
"quote",
Expand All @@ -4853,9 +4853,9 @@ dependencies = [

[[package]]
name = "serde_json"
version = "1.0.85"
version = "1.0.91"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "e55a28e3aaef9d5ce0506d0a14dbba8054ddc7e499ef522dd8b26859ec9d4a44"
checksum = "877c235533714907a8c2464236f5c4b2a17262ef1bd71f38f35ea592c8da6883"
dependencies = [
"indexmap",
"itoa",
Expand Down Expand Up @@ -5133,9 +5133,9 @@ dependencies = [

[[package]]
name = "syn"
version = "1.0.102"
version = "1.0.107"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "3fcd952facd492f9be3ef0d0b7032a6e442ee9b361d4acc2b1d0c4aaa5f613a1"
checksum = "1f4064b5b16e03ae50984a5a8ed5d4f8803e6bc1fd170a3cda91a1be4b18e3f5"
dependencies = [
"proc-macro2",
"quote",
Expand Down Expand Up @@ -5309,6 +5309,8 @@ dependencies = [
"lazy_static",
"miropt-test-tools",
"regex",
"semver",
"serde_json",
"termcolor",
"walkdir",
]
Expand Down
3 changes: 1 addition & 2 deletions src/bootstrap/bootstrap.py
Original file line number Diff line number Diff line change
Expand Up @@ -934,8 +934,7 @@ def main():
if len(sys.argv) > 1 and sys.argv[1] == 'help':
sys.argv = [sys.argv[0], '-h'] + sys.argv[2:]

help_triggered = (
'-h' in sys.argv) or ('--help' in sys.argv) or (len(sys.argv) == 1)
help_triggered = len(sys.argv) == 1 or any(x in ["-h", "--help", "--version"] for x in sys.argv)
try:
bootstrap(help_triggered)
if not help_triggered:
Expand Down
2 changes: 2 additions & 0 deletions src/tools/tidy/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ miropt-test-tools = { path = "../miropt-test-tools" }
lazy_static = "1"
walkdir = "2"
ignore = "0.4.18"
semver = "1.0.14"
serde_json = "1.0.91"
termcolor = "1.1.3"

[[bin]]
Expand Down
1 change: 1 addition & 0 deletions src/tools/tidy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,4 @@ pub mod ui_tests;
pub mod unit_tests;
pub mod unstable_book;
pub mod walk;
pub mod x_version;
4 changes: 3 additions & 1 deletion src/tools/tidy/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ fn main() {

let handle = s.spawn(|| {
let mut flag = false;
$p::check($($args),* , &mut flag);
$p::check($($args, )* &mut flag);
if (flag) {
bad.store(true, Ordering::Relaxed);
}
Expand Down Expand Up @@ -107,6 +107,8 @@ fn main() {
check!(alphabetical, &compiler_path);
check!(alphabetical, &library_path);

check!(x_version);

let collected = {
drain_handles(&mut handles);

Expand Down
80 changes: 80 additions & 0 deletions src/tools/tidy/src/x_version.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
use semver::Version;
use serde_json::Value;
use std::io::ErrorKind;
use std::process::{Command, Stdio};

pub fn check(bad: &mut bool) {
let result = Command::new("x").arg("--wrapper-version").stdout(Stdio::piped()).spawn();
// This runs the command inside a temporary directory.
// This allows us to compare output of result to see if `--wrapper-version` is not a recognized argument to x.
let temp_result = Command::new("x")
.arg("--wrapper-version")
.current_dir(std::env::temp_dir())
.stdout(Stdio::piped())
.spawn();

let (child, temp_child) = match (result, temp_result) {
(Ok(child), Ok(temp_child)) => (child, temp_child),
(Err(e), _) | (_, Err(e)) => match e.kind() {
ErrorKind::NotFound => return,
_ => return tidy_error!(bad, "failed to run `x`: {}", e),
},
};

let output = child.wait_with_output().unwrap();
let temp_output = temp_child.wait_with_output().unwrap();

if output != temp_output {
return tidy_error!(
bad,
"Current version of x does not support the `--wrapper-version` argument\nConsider updating to the newer version of x by running `cargo install --path src/tools/x`"
);
}

if output.status.success() {
let version = String::from_utf8_lossy(&output.stdout);
let version = Version::parse(version.trim_end()).unwrap();
jyn514 marked this conversation as resolved.
Show resolved Hide resolved

if let Some(expected) = get_x_wrapper_version() {
if version < expected {
return tidy_error!(
bad,
"Current version of x is {version}, but the latest version is {expected}\nConsider updating to the newer version of x by running `cargo install --path src/tools/x`"
);
}
} else {
return tidy_error!(
bad,
"Unable to parse the latest version of `x` at `src/tools/x/Cargo.toml`"
);
}
} else {
return tidy_error!(bad, "failed to check version of `x`: {}", output.status);
}
}

// 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.)

.arg("metadata")
.args(["--no-deps", "--format-version", "1", "--manifest-path", "src/tools/x/Cargo.toml"])
.stdout(Stdio::piped())
.spawn();

let child = match cmd {
Ok(child) => child,
Err(e) => {
println!("failed to get version of `x`: {}", e);
return None;
}
};

let cargo_output = child.wait_with_output().unwrap();
let cargo_output_str =
String::from_utf8(cargo_output.stdout).expect("Unable to parse `src/tools/x/Cargo.toml`");

let v: Value = serde_json::from_str(&cargo_output_str).unwrap();
let vesrion_str = &v["packages"][0]["version"].as_str()?;

Some(Version::parse(vesrion_str).unwrap())
}
8 changes: 8 additions & 0 deletions src/tools/x/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,14 @@ fn exec_or_status(command: &mut Command) -> io::Result<ExitStatus> {
}

fn main() {
match env::args().skip(1).next().as_deref() {
Some("--wrapper-version") => {
let version = env!("CARGO_PKG_VERSION");
println!("{}", version);
return;
}
_ => {}
}
let current = match env::current_dir() {
Ok(dir) => dir,
Err(err) => {
Expand Down