To have a downloaded wasm-opt work correctly on MacOS, multiple files need
to be extracted (the binary and the binaryen dylib) and those files
have to extracted into different directories. Specifically the binaryen
archive needs to be extracted into an adjacent lib directory. This
seems to necessitate having get_wasm_opt return a cache path that
would be an ancestor of the path to the executable file. Logic in
run_or_download relies on the path to the binary and the cache path
being the same to detect if the path to the tool has been overridden.
Switching get_wasm_opt to return an ancestor path of the executable
would break this check.
What this PR does
To simplify upcoming PRs, I'd like to refactor the stack of code between
get_tool and download_and_run logic to explicitly track whether the
path to the executable has been overridden. Replacing the current
requested != cache_path check will be necessary if cache_path will
have to be different than requested in a situation where there isn't
an override.
Why this approach
The cache_path doesn't seem to be used outside the check to see if the
path has been overridden. So replacing it with a boolean simplifies the
data being passed around.
Alternatives I've considered
Changing wasm-opt's cache_path from wasm-opt to
wasm-opt/bin/wasm-opt and walking back a couple directories in
install_wasm_opt when passing a path to download. That seemed to
spread out knowledge about how the tar file is structured and what
should be extracted where from other alternatives.
Using an enum to represent something like <OverriddenPath<PathBuf> | CachePath<PathBuf>> and then using an if let or matches! check to
detect if the path is overridden instead of (PathBuf, bool). It seemed
like a bit of overhead for what the code is doing but I'd be okay
with switching to this approach.
Context
To have a downloaded
wasm-opt
work correctly on MacOS, multiple files need to be extracted (the binary and the binaryen dylib) and those files have to extracted into different directories. Specifically the binaryen archive needs to be extracted into an adjacentlib
directory. This seems to necessitate havingget_wasm_opt
return a cache path that would be an ancestor of the path to the executable file. Logic inrun_or_download
relies on the path to the binary and the cache path being the same to detect if the path to the tool has been overridden. Switchingget_wasm_opt
to return an ancestor path of the executable would break this check.What this PR does
To simplify upcoming PRs, I'd like to refactor the stack of code between
get_tool
anddownload_and_run
logic to explicitly track whether the path to the executable has been overridden. Replacing the currentrequested != cache_path
check will be necessary ifcache_path
will have to be different thanrequested
in a situation where there isn't an override.Why this approach
The
cache_path
doesn't seem to be used outside the check to see if the path has been overridden. So replacing it with a boolean simplifies the data being passed around.Alternatives I've considered
wasm-opt
'scache_path
fromwasm-opt
towasm-opt/bin/wasm-opt
and walking back a couple directories ininstall_wasm_opt
when passing a path todownload
. That seemed to spread out knowledge about how the tar file is structured and what should be extracted where from other alternatives.<OverriddenPath<PathBuf> | CachePath<PathBuf>>
and then using anif let
ormatches!
check to detect if the path is overridden instead of(PathBuf, bool)
. It seemed like a bit of overhead for what the code is doing but I'd be okay with switching to this approach.