[CI] Enable pre-commit testing on Windows #8980
Conversation
| uses: ./.github/workflows/sycl_windows_build_and_test.yml | ||
| with: | ||
| lts_matrix: ${{ needs.test_matrix.outputs.lts_wn_matrix }} | ||
| build_ref: ${{ github.event.pull_request.head.sha }} |
There was a problem hiding this comment.
I see that it follows Linux path, but maybe github.ref could be a better choice:
The fully-formed ref of the branch or tag that triggered the workflow run. For workflows triggered by push, this is the branch or tag ref that was pushed. For workflows triggered by pull_request, this is the pull request merge branch. For workflows triggered by release, this is the release tag created. For other triggers, this is the branch or tag ref that triggered the workflow run. This is only set if a branch or tag is available for the event type. The ref given is fully-formed, meaning that for branches the format is refs/heads/<branch_name>, for pull requests it is refs/pull/<pr_number>/merge, and for tags it is refs/tags/<tag_name>. For example, refs/heads/feature-branch-1.
Anyway, could/should be a separate PR if at all.
There was a problem hiding this comment.
Agree. Perhaps a separate PR for fixes to both Linux and Windows jobs would be better.
| if: github.repository == 'intel/llvm' | ||
| uses: ./.github/workflows/sycl_gen_test_matrix.yml | ||
| with: | ||
| lts_config: "l0_gen9;win_l0_gen12" |
There was a problem hiding this comment.
Why do we have Linux task ("l0_gen9") here?
There was a problem hiding this comment.
Perhaps, we don't need it. I've fixed it.
There was a problem hiding this comment.
Having both configurations: Windows pre-commit and Windows testing on demand doesn't make sense to me.
We added on-demand Windows testing because according to our estimates CI doesn't have enough resources to handle the load added by Windows pre-commit. Both jobs do the same thing, but pre-commit launches automatically for each commit. On-demand testing is not needed if each commit is tested.
I have no objections to run an experiment with enabling Windows in pre-commit to check how CI will handle the load.
There should be only on configuration, so if you go with pre-commit, please, remove or disable "on-demand" testing.
@romanovvlad, FYI.
|
@bader Agree. I've temporarily disabled the "on-demand" windows testing workflow. We can remove it completely, let's say after a week, if the Windows CI handles the load as we expect. |
|
@intel/llvm-gatekeepers The PR is ready! |
|
Don't let me hold anything up in this space, but one thing to consider for Windows-specific precommit testing is to build a debug configuration so that we get checked & debug iterator testing (https://learn.microsoft.com/en-us/cpp/standard-library/safe-libraries-cpp-standard-library?view=msvc-170). That helps catch quite a few mistakes (I just fixed one this morning that turned out to be |
This PR (1) enables running Windows Build + LIT + E2E tests during pre-commit testing, (2) Fix windows_test_comment_trigger workflow after #8656