-
Notifications
You must be signed in to change notification settings - Fork 777
[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
Conversation
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).
What about the second separate file for the same set for the xisa build? |
Will be done in downstream branch only. |
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.
Can we also update the CODEOWNERS file to make unified-runtime-reviewers owners of the newly created file sycl/cmake/modules/UnifiedRuntimeTAG.cmake
?
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.
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
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.
I think this filename should have the Tag
suffix rather than TAG
.
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.
Sure.
Thanks @kbenzie and @uditagarwal97 . |
Co-authored-by: Udit Agarwal <[email protected]>
@intel/llvm-gatekeepers Can someone help to merge this. Thanks. |
@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 |
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. |
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).