miri-script: Transform Windows paths to Unix. #2877

osiewicz · 2023-05-05T16:39:37Z

python3 snippet used to fill $MIRIDIR variable returns native paths. Down the line in bench subcommand this leads to benchmark setup failure, preventing contributors from running benchmarks on Windows hosts.

This commit replaces usage of native os.path module with pathlib, which explicitly converts paths to Unix flavour.

pathlib is available from Python 3.4. There's also a nice comparison of features with os module.

On current master, running ./miri bench under MinGW results in an error from cargo-metadata about path format.

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: CargoMetadata { stderr: "error: manifest path `D:Programmingmiri/bench-cargo-miri/backtraces/Cargo.toml` does not exist\n" }', src\util.rs:224:80

With this commit, it is not an issue anymore.

python3 snippet used to fill $MIRIDIR variable returns native paths. Down the line in `bench` subcommand this leads to benchmark setup failure, preventing contributors from running benchmarks on Windows hosts. This commit replaces usage of native os.path module with pathlib, which explicitly converts paths to Unix flavour.

oli-obk · 2023-05-05T16:50:42Z

Thanks! Could we test a trivial fast benchmark in CI just to make sure it works? Or will that take too long due to compiling in release mode?

osiewicz · 2023-05-05T17:00:36Z

Running cargo clean followed by ./miri bench slice-get-unchecked takes about 90 seconds on my PC, though as you've said, majority of that time is spent compiling Miri. CI run will take a bit longer, but IMHO it looks feasible to do that anyways.
Would you like ./miri bench to run as part of build pipeline or do you have something else in mind?

Edit: It seems that miri is already compiled in release mode in CI, so yeah, it should essentially be free to do that.

oli-obk · 2023-05-05T17:54:42Z

Yea, just part of the build pipeline makes sense. We'll see on the PR CI if there's a relevant increase in time

osiewicz · 2023-05-05T20:59:44Z

So it seems like current hyperfine release doesn't work on i686-pc-windows-msvc target. The fix is available, but not released (yet). cargo install --target i686-pc-windows-msvc hyperfine fails and cargo install --target i686-pc-windows-msvc --git https://github.com/sharkdp/hyperfine.git works just fine.
I will investigate failures from linux/macos. It seems like this might be blocked by hyperfine release anyways.

RalfJung · 2023-05-06T10:19:01Z

ci.sh

@@ -81,6 +81,9 @@ function run_tests {
      cargo miri run --manifest-path bench-cargo-miri/$BENCH/Cargo.toml
    done
  fi
+  # Ensure that `./miri bench` works on all relevant platforms.
+  # `slice-get-unchecked` is the least complex of all available benchmarks.
+  CARGO_EXTRA_FLAGS="" ./miri bench slice-get-unchecked


This is the wrong place to put the check. This here will be run for each target that we check. It needs to either be gated by -z "${MIRI_TEST_TARGET+exists}" (to only run on the host) or be moved elsewhere.

Also why the CARGO_EXTRA_FLAGS=""?

Also why the CARGO_EXTRA_FLAGS=""?

Since CARGO_EXTRA_FLAGS is set to --locked, I've cleared it to fix an issue with --locked being passed twice (which manifested itself in CI run of ca60799 commit). Is there a better way to do that? Perhaps I could document why this flag is cleared?

Ah, that sounds like a bug in our miri script... maybe we just shouldn't have the --locked in the install command.

RalfJung · 2023-05-06T10:22:32Z

.github/workflows/ci.yml

@@ -78,6 +78,9 @@ jobs:
          rustc -Vv
          cargo -V

+      - name: Install hyperfine
+        run: cargo install hyperfine


This looks like it will cost us a minute. :/ Are there binary releases we could install?

Also the Linux run in this PR took almost 1h which is a lot slower than what I remember... is that a coincidence or related to this PR?

I'm not aware of binary distributions of hyperfine.

Perhaps we could install dev version, as we don't really care about measurements?
Though I'd expect hyperfine to be quick to build - even in release mode.

As for the slowdown of Linux build, let's see if reproduces on another CI run. I wouldn't expect this PR to slow down CI significantly.

It looks like Linux run is back to normal. Perhaps running miri bench only for host targets is a positive factor?

It's about a minute to build. So not too bad I guess...

OTOH we don't actually care about the benchmarking part on CI, so we could as a hack also have CI create some hyperfine shell script that just executes its last argument. At least that would work on Linux. Whether it makes sense on Windows I cannot say.

Document the reason for CARGO_EXTRA_FLAGS being cleared.

osiewicz · 2023-05-06T13:07:37Z

miri

@@ -77,7 +77,7 @@ if [ -z "$COMMAND" ]; then
 fi
 shift
 # macOS does not have a useful readlink/realpath so we have to use Python instead...
-MIRIDIR=$(python3 -c 'import os, sys; print(os.path.dirname(os.path.realpath(sys.argv[1])))' "$0")
+MIRIDIR=$(python3 -c 'import pathlib, sys; print(pathlib.Path(sys.argv[1]).resolve().parent.as_posix())' "$0")


By the way, what do you think about detecting python binary to run (similar to what's done in ci.sh)? On my Windows machine python3 does not work out of the box, so I guess it might be worthwhile to do that.

Oh no why is everything so terrible. :(

We don't have to use python for this but I found no better way to make it work on macOS.

Really we should rewrite the entire damn thing in Rust I guess if we truly want it to be portable...

Really we should rewrite the entire damn thing in Rust I guess if we truly want it to be portable...

I'd be happy to tackle that in the future PR. Is there a tracking issue for that initiative?

No. Do you want to create one? :)

RalfJung · 2023-05-31T09:37:48Z

Looks like hyperfine doesn't even build on Windows currently?

I have implemented the alternative sketched above that avoids the need for hyperfine on CI in #2908. I'll add your commit fixing Windows to that PR, and close this one. Thanks a lot for your help!

osiewicz · 2023-06-03T21:46:35Z

New version of hyperfine just dropped (with proper Windows support): https://github.com/sharkdp/hyperfine/releases/tag/v0.17.0

osiewicz force-pushed the miri-script-fix-windows-miridir branch from 1d86152 to 0255998 Compare May 5, 2023 16:40

osiewicz changed the title ~~miri-script: Transform Windows paths to unix.~~ miri-script: Transform Windows paths to Unix. May 5, 2023

Check ./miri bench in CI

ca60799

miri-script: Clear CARGO_EXTRA_FLAGS

24c8829

RalfJung reviewed May 6, 2023

View reviewed changes

Constrain miri bench test to host platforms.

b157354

Document the reason for CARGO_EXTRA_FLAGS being cleared.

osiewicz commented May 6, 2023

View reviewed changes

This was referenced May 9, 2023

'cargo install hyperfine' fails on Windows sharkdp/hyperfine#624

Closed

Rewrite miri script in Rust #2883

Closed

RalfJung mentioned this pull request May 31, 2023

CI: test ./miri bench #2908

Merged

RalfJung closed this May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

miri-script: Transform Windows paths to Unix. #2877

miri-script: Transform Windows paths to Unix. #2877

osiewicz commented May 5, 2023 •

edited

Loading

oli-obk commented May 5, 2023

osiewicz commented May 5, 2023 •

edited

Loading

oli-obk commented May 5, 2023

osiewicz commented May 5, 2023 •

edited

Loading

RalfJung May 6, 2023

RalfJung May 6, 2023

osiewicz May 6, 2023 •

edited

Loading

RalfJung May 8, 2023

RalfJung May 6, 2023

osiewicz May 6, 2023 •

edited

Loading

osiewicz May 6, 2023

RalfJung May 8, 2023

osiewicz May 6, 2023

RalfJung May 8, 2023

osiewicz May 8, 2023 •

edited

Loading

RalfJung May 8, 2023

RalfJung commented May 31, 2023

osiewicz commented Jun 3, 2023

miri-script: Transform Windows paths to Unix. #2877

miri-script: Transform Windows paths to Unix. #2877

Conversation

osiewicz commented May 5, 2023 • edited Loading

oli-obk commented May 5, 2023

osiewicz commented May 5, 2023 • edited Loading

oli-obk commented May 5, 2023

osiewicz commented May 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osiewicz May 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osiewicz May 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osiewicz May 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented May 31, 2023

osiewicz commented Jun 3, 2023

osiewicz commented May 5, 2023 •

edited

Loading

osiewicz commented May 5, 2023 •

edited

Loading

osiewicz commented May 5, 2023 •

edited

Loading

osiewicz May 6, 2023 •

edited

Loading

osiewicz May 6, 2023 •

edited

Loading

osiewicz May 8, 2023 •

edited

Loading