-
Notifications
You must be signed in to change notification settings - Fork 608
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.
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
Test Windows runtime on presubmit, merge build/test runtime jobs. #11032
Conversation
fd8cbad
to
55beccb
Compare
--env IREE_VULKAN_DISABLE=1 \ | ||
gcr.io/iree-oss/base@sha256:5d43683c6b50aebe1fca6c85f2012f3b0fa153bf4dd268e8767b619b1891423a \ | ||
./build_tools/cmake/ctest_all.sh \ | ||
"${BUILD_DIR}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make this a single docker run
call as in our other build&test workflows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like having build and test in separate steps, so we get a nice timing breakdown in the action summary:
Duplicating the docker command in each step does repeat many characters though. Can we put our Docker image in uses
? https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-using-a-docker-hub-action Or set a container
? https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idcontainer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not just characters repeating: it's also tearing down and relaunching the container (although I'm not sure how long that takes, in practice. Apparently less than 3s). I've steered away from GitHub actions specific ways of using Docker containers because they're more tied to the CI and therefore harder to reproduce locally or migrate to other CI systems. That includes translating all the docker run
options into the GitHub Actions syntax. The script for doing this does additional setup (like creating directories to mount), that would have to be sharded out elsewhere. Overall, it just seems like a false simplification. One option we have would be to start the container running in one step and then use docker exec
to run various commands in it.
Regardless, what you've got here is fine for now, though I do still have a slight preference for doing it in a single step to match how other jobs work and avoid launching a container multiple times (although that doesn't seem to have meaningful latency overhead). If you wanted to follow-up by splitting all of them, then that would be fine too :-P
55beccb
to
03ec4bd
Compare
Okay, it's no longer Friday afternoon and I've synced with |
--env IREE_VULKAN_DISABLE=1 \ | ||
gcr.io/iree-oss/base@sha256:5d43683c6b50aebe1fca6c85f2012f3b0fa153bf4dd268e8767b619b1891423a \ | ||
./build_tools/cmake/ctest_all.sh \ | ||
"${BUILD_DIR}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not just characters repeating: it's also tearing down and relaunching the container (although I'm not sure how long that takes, in practice. Apparently less than 3s). I've steered away from GitHub actions specific ways of using Docker containers because they're more tied to the CI and therefore harder to reproduce locally or migrate to other CI systems. That includes translating all the docker run
options into the GitHub Actions syntax. The script for doing this does additional setup (like creating directories to mount), that would have to be sharded out elsewhere. Overall, it just seems like a false simplification. One option we have would be to start the container running in one step and then use docker exec
to run various commands in it.
Regardless, what you've got here is fine for now, though I do still have a slight preference for doing it in a single step to match how other jobs work and avoid launching a container multiple times (although that doesn't seem to have meaningful latency overhead). If you wanted to follow-up by splitting all of them, then that would be fine too :-P
Progress on #11009, depends on #11048
Changes:
build_runtime
+test_runtime
->build_test_runtime
(overhead from repository cloning, artifact upload, and artifact download was taking longer than just running the tests from the same job)build_runtime_windows
->build_test_runtime_windows
managed-windows-cpu
(larger build machine)Sample run: https://github.com/iree-org/iree/actions/runs/3412369869/jobs/5677798847