Skip to content

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.

7 participants