tidy tries to run unrelated x binary

[ehuss]

Starting with #104552, ./x.py test tidy fails on my system:

thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: Error("unexpected character 'H' while parsing major version number")', src/tools/tidy/src/x_version.rs:36:58

I have an executable installed on my system called x that is unrelated to rust. I would prefer if tidy didn't execute it at all.

[jyn514]

Hmm, that's unfortunate, I didn't realize this was a name that was already being used :(

cc @DebugSteven, we might have to revert that PR :/

@ehuss would it be ok if we only ran your x command and silently hid the error, so nothing ends up happening? Or does even that cause issues?

[Mark-Simulacrum]

We can probably check that the binary is in ~/.cargo/bin or so before running it, at least. We can probably also add a custom section to it or other content that we can find without running it (e.g., by reading the contents and looking for a well known version).

I seem to recall cargo tracking installed versions too, maybe we can lean on that?

[jyn514]

I seem to recall cargo tracking installed versions too, maybe we can lean on that?

Oh, that's an excellent idea! yes, cargo install --list will output something like

x v0.1.0 (/home/gh-jyn514/rust3/src/tools/x):
    x

and we can only run x if we see src/tools/x in the output.

A debug section seems slightly overkill but I'm not opposed if you think cargo install --list will be unreliable for whatever reason; I suppose PATH could be in the wrong order or something like that.

[Mark-Simulacrum]

I think my point is that if we're diligent about bumping the cargo.toml version we don't ever need to run it, just look at Cargo output.

[ehuss]

I think for me it might be OK to ignore any errors running the command or any parsing errors on the output.

cargo install --list isn't intended to be machine-readable, but is also an option that could work.

Opening the executable and searching for a unique sequence of bytes that precedes the version number is also an option, but possibly a bit of a hassle. (Or it could look for some unique string that identifies it as the x.py wrapper, and then execute it.)

Any option is fine, and there's no urgency here unless it affects other people.

[Ezrashaw]

I really don't like the idea of just running a arbitrary binary, even if it appears in cargo install --list. On my system, the command X (note that it is a capital but this is too close for comfort) runs X11.

Perhaps the check can be moved into bootstrap (or anywhere out of tidy) so that it can know its version without running binaries? IMO, tidy is more about linting code; it shouldn't emit an error if a user has an old version of an optional and unessential tool.

[jyn514]

This was fixed in #107048 :)