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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0255998
miri-script: Transform Windows paths to unix.
osiewicz May 5, 2023
ca60799
Check ./miri bench in CI
osiewicz May 5, 2023
24c8829
miri-script: Clear CARGO_EXTRA_FLAGS
osiewicz May 5, 2023
b157354
Constrain miri bench test to host platforms.
osiewicz May 6, 2023
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
3 changes: 3 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ jobs:
rustc -Vv
cargo -V

- name: Install hyperfine
run: cargo install hyperfine
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@osiewicz osiewicz May 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


- name: Test
run: bash ./ci.sh

Expand Down
3 changes: 3 additions & 0 deletions ci.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also why the CARGO_EXTRA_FLAGS=""?

Copy link
Contributor Author

@osiewicz osiewicz May 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


endgroup
}
Expand Down
2 changes: 1 addition & 1 deletion miri
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@osiewicz osiewicz May 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Do you want to create one? :)

# Used for rustc syncs.
JOSH_FILTER=":rev(75dd959a3a40eb5b4574f8d2e23aa6efbeb33573:prefix=src/tools/miri):/src/tools/miri"
# Needed for `./miri bench`.
Expand Down