Skip to content

[CI][CTS] Add fixed ref for cts #17074

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

Merged
merged 5 commits into from
Feb 26, 2025
Merged

[CI][CTS] Add fixed ref for cts #17074

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9810e5b
[CI][CTS] Add fixed ref for cts
KornevNikita Feb 19, 2025
bac35ea
apply suggestion
KornevNikita Feb 20, 2025
28c7483
apply suggestion attempt 2
KornevNikita Feb 20, 2025
83da408
apply suggestion
KornevNikita Feb 24, 2025
599deaf
Update sycl-linux-run-tests.yml
KornevNikita Feb 25, 2025
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
9 changes: 6 additions & 3 deletions .github/workflows/sycl-linux-run-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,11 @@ on:
By default we checkout the devops directory from "inputs.ref" branch.
devops_ref may be specified to checkout the devops dir from different
branch.
Note: it doesn't affect ./devops/actions/run-tests/* as these actions
call checkout again and therefore override the devops directory, so
configs/dependecies from input.ref are used.
tests_ref:
description: "Commit SHA or branch to checkout tests"
type: string
default: "main"
required: False

sycl_toolchain_artifact:
type: string
Expand Down Expand Up @@ -314,3 +316,4 @@ jobs:
sycl_cts_artifact: ${{ inputs.sycl_cts_artifact }}
target_devices: ${{ inputs.target_devices }}
retention-days: ${{ inputs.retention-days }}
tests_ref: ${{ inputs.tests_ref }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs more alignment with e2e still. Something has to be done with line 302. I think the devops/actions/run-tests/* should accept single ref input (as in line 302). And the job of the .yml file is to translate tests_ref/ref/etc. into such an argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, right, likely we don't need a new parameter, we can just use ref for this purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I get it right? 28c7483

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer

ref:  #maybe rename to llvm_ref/repo_ref/toolchain_ref or something
tests_ref:
  required: False  # no default
  type: string

jobs:
...
   e2e:
     ref: ${{ tests_ref || ref || "main" }}

   cts:
     ref: ${{ tests_ref || "main" }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

83da408
Let's rename ref to repo_ref in a follow-up patch as it requires changes for all workflows that use sycl-linux-run-tests.yml.

I can't really imagine a good case when we need different branch for the compiler build and e2e tests, and ref is required: True, therefore "main" is never used. So I left this string as is. I can change it if you insist.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with delayed renaming, but e2e branch has to check for tests_ref first, even if it isn't set anywhere currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated 599deaf

7 changes: 5 additions & 2 deletions devops/actions/run-tests/cts/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ inputs:
required: true
retention-days:
required: false
tests_ref:
description: "Commit SHA or branch to checkout tests"
required: false
default: "main"

runs:
using: "composite"
Expand All @@ -21,8 +25,7 @@ runs:
with:
path: khronos_sycl_cts
repository: 'KhronosGroup/SYCL-CTS'
ref: 'main'
default_branch: 'main'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to mention - deleting this as it's not used at all.

ref: ${{ inputs.tests_ref }}
cache_path: "/__w/repo_cache/"
- name: SYCL CTS GIT submodules init
if: inputs.cts_testing_mode != 'run-only'
Expand Down
Loading