Commit 5e9cab3

committed

modify ensure_version_or_cargo_install to check existing binary

Current implementation uses bin_name to check if it exists, but it should use tool_root_dir/tool_bin_dir/bin_name instead. Otherwise the check fails every time, hence the function falls back to install the binary.

1 parent c7f6aa2 commit 5e9cab3Copy full SHA for 5e9cab3

File tree

1 file changed

-5

lines changed

src/tools/tidy/src
- lib.rs

1 file changed

-5

lines changed

`‎src/tools/tidy/src/lib.rs‎`

Lines changed: 6 additions & 5 deletions

Original file line number	Diff line number	Diff line change
`@@ -167,12 +167,16 @@ pub fn ensure_version_or_cargo_install(`
`167`	`167`	`bin_name: &str,`
`168`	`168`	`version: &str,`
`169`	`169`	`) -> io::Result<PathBuf> {`
	`170`	`+ let tool_root_dir = build_dir.join("misc-tools");`
	`171`	`+ let tool_bin_dir = tool_root_dir.join("bin");`
	`172`	`+ let bin_path = tool_bin_dir.join(bin_name).with_extension(env::consts::EXE_EXTENSION);`
	`173`	`+`
`170`	`174`	`// ignore the process exit code here and instead just let the version number check fail.`
`171`	`175`	`// we also importantly don't return if the program wasn't installed,`
`172`	`176`	`// instead we want to continue to the fallback.`
`173`	`177`	`'ck: {`
`174`	`178`	`// FIXME: rewrite as if-let chain once this crate is 2024 edition.`
`175`		`- let Ok(output) = Command::new(bin_name).arg("--version").output() else {`
	`179`	`+ let Ok(output) = Command::new(&bin_path).arg("--version").output() else {`
`176`	`180`	`break 'ck;`
`177`	`181`	`};`
`178`	`182`	`let Ok(s) = str::from_utf8(&output.stdout) else {`
`@@ -182,12 +186,10 @@ pub fn ensure_version_or_cargo_install(`
`182`	`186`	`break 'ck;`
`183`	`187`	`};`
`184`	`188`	`if v == version {`
`185`		`- return Ok(PathBuf::from(bin_name));`
	`189`	`+ return Ok(bin_path);`
`186`	`190`	`}`
`187`	`191`	`}`
`188`	`192`
`189`		`- let tool_root_dir = build_dir.join("misc-tools");`
`190`		`- let tool_bin_dir = tool_root_dir.join("bin");`
`191`	`193`	`eprintln!("building external tool {bin_name} from package {pkg_name}@{version}");`
`192`	`194`	`// use --force to ensure that if the required version is bumped, we update it.`
`193`	`195`	`// use --target-dir to ensure we have a build cache so repeated invocations aren't slow.`
`@@ -213,7 +215,6 @@ pub fn ensure_version_or_cargo_install(`
`213`	`215`	`if !cargo_exit_code.success() {`
`214`	`216`	`return Err(io::Error::other("cargo install failed"));`
`215`	`217`	`}`
`216`		`- let bin_path = tool_bin_dir.join(bin_name).with_extension(env::consts::EXE_EXTENSION);`
`217`	`218`	`assert!(`
`218`	`219`	`matches!(bin_path.try_exists(), Ok(true)),`
`219`	`220`	`"cargo install did not produce the expected binary"`

0 commit comments

Comments

(0)

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Commit 5e9cab3

File tree

1 file changed

1 file changed

`‎src/tools/tidy/src/lib.rs‎`

0 commit comments