[CI] Prebuild E2E tests as part of build workflow#16682
Conversation
dadb6af to
2060b2a
Compare
2060b2a to
902d98d
Compare
902d98d to
c79e005
Compare
c79e005 to
52ab1c0
Compare
52ab1c0 to
3169ec2
Compare
3169ec2 to
b5cf3bb
Compare
b5cf3bb to
ca90f35
Compare
ca90f35 to
6b3c47e
Compare
77d41e2 to
fb70bbf
Compare
fb70bbf to
63c84c2
Compare
63c84c2 to
1416929
Compare
| - name: Source OneAPI TBB vars.sh | ||
| shell: bash | ||
| run: | | ||
| # https://github.com/actions/runner/issues/1964 prevents us from using | ||
| # the ENTRYPOINT in the image. | ||
| env | sort > env_before | ||
| if [ -e /runtimes/oneapi-tbb/env/vars.sh ]; then | ||
| source /runtimes/oneapi-tbb/env/vars.sh; | ||
| elif [ -e /opt/runtimes/oneapi-tbb/env/vars.sh ]; then | ||
| source /opt/runtimes/oneapi-tbb/env/vars.sh; | ||
| else | ||
| echo "no TBB vars in /opt/runtimes or /runtimes"; | ||
| fi | ||
| env | sort > env_after | ||
| comm -13 env_before env_after >> $GITHUB_ENV | ||
| rm env_before env_after |
There was a problem hiding this comment.
Does it mean that whenever we call .github/workflows/sycl-linux-build.yml, we also build E2E tests? IIRC, we don't use pre-built test binaries in post-commit and nightly.
There was a problem hiding this comment.
Yes, we do, and no, we don't. That said I imagine we can start doing so relatively soon. Alternatively, we can make this addition optional, but I thought that build-only mode of e2e tests isn't that different from check-sycl that we do unconditionally everywhere (well, if sycl is modified in pre-commit). Writing this, I can even imagine re-unification of sycl/test and sycl/test-e2e now!
There was a problem hiding this comment.
I would personally be of the opinion that we make this addition optional - don't build E2E tests by default when sycl-linux-build.yml is called, unless the caller explicitly asks for it.
Once the "split-test" mode is mature enough that we use it for all workflows and other runners as well, we can build E2E tests by default, just like sycl/test.
There was a problem hiding this comment.
I think it will just complicate implementation with very little benefit... Nightly/post traffic is several times smaller than pre-commit (because PR usually take several iterations). Also, unlike pre-commit, nightly/post don't skip any check-* at build stage, so having e2e built there takes less percentage of the overall duration.
There was a problem hiding this comment.
i agree with udit if the implementation cost is low, buf it's high as andrei says im fine with enabling it always
There was a problem hiding this comment.
I think it will just complicate implementation with very little benefit...
If it complicates the implementation, I'm fine with the current PR.
There was a problem hiding this comment.
Thanks. I'll plan to work on it the coming days/weeks. Either make sure the e2e artifacts get used more or make optional, don't believe we need to delay progress by making extra changes and extra testing. Too easy to make a typo and too long to run the full CI cycle. Very much would like to merge while it's green :)
sarnex
left a comment
There was a problem hiding this comment.
lgtm just minor kinda unrelated questions
| build_cache_suffix: "default" | ||
| # Docker image has last nightly pre-installed and added to the PATH | ||
| build_image: "ghcr.io/intel/llvm/sycl_ubuntu2204_nightly:build" | ||
| build_image: "ghcr.io/intel/llvm/sycl_ubuntu2404_nightly:latest" |
There was a problem hiding this comment.
can we just delete the build image?
There was a problem hiding this comment.
discussed offline, we only create *nightly:latest now, the :build is getting increasingly stale/outdated.
| e2e_testing_mode: build-only | ||
| target_devices: all | ||
| artifact_suffix: default | ||
| cxx_compiler: $GITHUB_WORKSPACE/toolchain/bin/clang++ |
There was a problem hiding this comment.
if we dont pass this, it falls back to which clang++, so i assume the built clang++ isn't on the path?
There was a problem hiding this comment.
discussed offline, we need to make sure CMake sees the same CXX compiler at build/run stages.
Reduce number of input parameters and make the logic a bit cleaner (IMO). This PR also uses that updated logic to make building E2E tests optional in `sycl-linux-build.yml` and makes enabled in pre-commit only for now, effectively fixing the regression in Nightly CI introduced in #16682.
) Reduce number of input parameters and make the logic a bit cleaner (IMO). This PR also uses that updated logic to make building E2E tests optional in `sycl-linux-build.yml` and makes enabled in pre-commit only for now, effectively fixing the regression in Nightly CI introduced in #16682.
This will allow us to save time on runner setup and toolchain download/unpacking.