-
Notifications
You must be signed in to change notification settings - Fork 798
[CI] Refactor toolchain-artifact related inputs/outputs #19643
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
Conversation
* Group all artifact-related inputs in `sycl-linux-build.yml`. I plan on refactoring artifacts for E2E tests/release toolchain in a subsequent PR. * Unify inputs/outputs names between `sycl-*-build.yml` and `sycl-*-run-tests.yml`. Windows still has less flexibility, but for the inputs that exist there the names still match Linux and the inputs are ordered similarly to Linux. * Add `${{ needs.build.outputs.toolchain_artifact }}` instead of duplicating the artifact name. Aligns usage with actual filename/decompress command.
value: ${{ inputs.toolchain_artifact }} | ||
toolchain_artifact_filename: | ||
value: ${{ jobs.build.outputs.toolchain_artifact_filename }} | ||
toolchain_decompress_command: |
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.
not directly related but do we actually need toolchain_decompress_command
? If I remember correctly there were only like two cases we actually use, maybe it was tar.gz
and tar.zst
. If that's true could we get rid of this var everywhere and just have an if
statement when we actually run the decompression to handle both cases? I don't think we need to support arbitrary decompression commands specified by higher level workflows
Maybe we could also just pass the filename blindly to 7z
or something which probably figures out what to do internally without us having to have conditionals.
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.
Yes, we only have two. However, doing the if
at the use-case would mean that every user (well, it's only sycl-hardening-check.yml
and sycl-linux-run-tests.yml
for now, but still) will have to duplicate it, which I'd dislike even more than this.
Ideally, we could have a single (JSON?) output that contained all the data. That is something I'm leaving for future/somebody else :)
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.
to be honest i think we could just slap the files to 7z
but yeah we can leave it to someone else
@@ -281,7 +288,7 @@ jobs: | |||
if: ${{ always() && !cancelled() && steps.build.conclusion == 'success' }} | |||
uses: actions/upload-artifact@v4 | |||
with: | |||
name: sycl_linux_${{ inputs.build_artifact_suffix }} |
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.
lol why did we ever do this
Failures in Pre/Post-commit tasks are on individual tests and not due to a regression in CI itself, unrelated to this PR. Nightly run: https://github.com/intel/llvm/actions/runs/16630331081
was a separate regression and has been addressed in #19641 Benchmarks workflow: https://github.com/intel/llvm/actions/runs/16630629189 |
sycl-linux-build.yml
. I plan on refactoring artifacts for E2E tests/release toolchain in a subsequent PR.sycl-*-build.yml
andsycl-*-run-tests.yml
. Windows still has less flexibility, but for the inputs that exist there the names still match Linux and the inputs are ordered similarly to Linux.${{ needs.build.outputs.toolchain_artifact }}
instead of duplicating the artifact name. Aligns usage with actual filename/decompress command.