Skip to content

[UR] Split the UNIFIED_RUNTIME_TAG to its own file #15809

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

Merged
merged 5 commits into from
Oct 22, 2024
Merged

[UR] Split the UNIFIED_RUNTIME_TAG to its own file #15809

merged 5 commits into from
Oct 22, 2024

Conversation

jsji
Copy link
Contributor

@jsji jsji commented Oct 22, 2024

With current repo setup, we need to update UNIFIED_RUNTIME_TAG
constantly. This is creating conflicts when doing merge from other
branches. To faciliate the automation to handle such conflicts,
we would prefer to split the tag into into own file, the automation
will then be triggereed whenever the file changes (implying UR tag
change).

With current repo setup, we need to update UNIFIED_RUNTIME_TAG
constantly. This is creating conflicts when doing merge from other
branches. To faciliate the automation to handle such conflicts,
we would prefer to split the tag into into own file, the automation
will then be triggereed whenever the file changes (implying UR tag
change).
@jsji jsji self-assigned this Oct 22, 2024
@omirochn
Copy link

What about the second separate file for the same set for the xisa build?

@jsji jsji temporarily deployed to WindowsCILock October 22, 2024 15:03 — with GitHub Actions Inactive
@jsji
Copy link
Contributor Author

jsji commented Oct 22, 2024

What about the second separate file for the same set for the xisa build?

Will be done in downstream branch only.

@jsji jsji marked this pull request as ready for review October 22, 2024 16:15
@jsji jsji requested review from a team as code owners October 22, 2024 16:15
@jsji jsji requested a review from uditagarwal97 October 22, 2024 16:15
Copy link
Contributor

@uditagarwal97 uditagarwal97 left a comment

Choose a reason for hiding this comment

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

Can we also update the CODEOWNERS file to make unified-runtime-reviewers owners of the newly created file sycl/cmake/modules/UnifiedRuntimeTAG.cmake?

Copy link
Contributor

@kbenzie kbenzie left a comment

Choose a reason for hiding this comment

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

I think this change makes sense. As mentioned by @uditagarwal97 the .github/CODEOWNERS file must be updated, for example as follows which includes my suggested filename change:

diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS
--- a/.github/CODEOWNERS
+++ b/.github/CODEOWNERS
@@ -36,6 +36,7 @@ sycl/doc/extensions/ @intel/dpcpp-specification-reviewers

 # Unified Runtime
 sycl/cmake/modules/FetchUnifiedRuntime.cmake @intel/unified-runtime-reviewers
+sycl/cmake/modules/FetchUnifiedRuntimeTag.cmake @intel/unified-runtime-reviewers
 sycl/include/sycl/detail/ur.hpp @intel/unified-runtime-reviewers
 sycl/source/detail/posix_ur.cpp @intel/unified-runtime-reviewers
 sycl/source/detail/ur.cpp @intel/unified-runtime-reviewers

Copy link
Contributor

@kbenzie kbenzie Oct 22, 2024

Choose a reason for hiding this comment

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

I think this filename should have the Tag suffix rather than TAG.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

@jsji jsji requested a review from a team as a code owner October 22, 2024 16:36
@jsji
Copy link
Contributor Author

jsji commented Oct 22, 2024

I think this change makes sense. As mentioned by @uditagarwal97 the .github/CODEOWNERS file must be updated, for example as follows which includes my suggested filename change:

diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS
--- a/.github/CODEOWNERS
+++ b/.github/CODEOWNERS
@@ -36,6 +36,7 @@ sycl/doc/extensions/ @intel/dpcpp-specification-reviewers

 # Unified Runtime
 sycl/cmake/modules/FetchUnifiedRuntime.cmake @intel/unified-runtime-reviewers
+sycl/cmake/modules/FetchUnifiedRuntimeTag.cmake @intel/unified-runtime-reviewers
 sycl/include/sycl/detail/ur.hpp @intel/unified-runtime-reviewers
 sycl/source/detail/posix_ur.cpp @intel/unified-runtime-reviewers
 sycl/source/detail/ur.cpp @intel/unified-runtime-reviewers

Thanks @kbenzie and @uditagarwal97 .

@jsji jsji requested review from kbenzie and uditagarwal97 October 22, 2024 16:37
Co-authored-by: Udit Agarwal <[email protected]>
@jsji jsji temporarily deployed to WindowsCILock October 22, 2024 18:18 — with GitHub Actions Inactive
@jsji jsji requested a review from uditagarwal97 October 22, 2024 18:18
@jsji jsji temporarily deployed to WindowsCILock October 22, 2024 19:28 — with GitHub Actions Inactive
@jsji
Copy link
Contributor Author

jsji commented Oct 22, 2024

@intel/llvm-gatekeepers Can someone help to merge this. Thanks.

@againull againull merged commit 439ec9e into sycl Oct 22, 2024
12 checks passed
@sarnex
Copy link
Contributor

sarnex commented Oct 22, 2024

@jsji I must be missing it, but I don't see how this solves the problem. Won't merge conflict problem just be moved to sycl/cmake/modules/UnifiedRuntimeTag.cmake now?

@jsji
Copy link
Contributor Author

jsji commented Oct 22, 2024

@jsji I must be missing it, but I don't see how this solves the problem. Won't merge conflict problem just be moved to sycl/cmake/modules/UnifiedRuntimeTag.cmake now?

Yes, you are right, this is not trying to remove the conflict in the tag itself, but to facilitate the automation to help with resolving the conflicts.

@bader bader deleted the urtagfile branch October 30, 2024 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants