-
Notifications
You must be signed in to change notification settings - Fork 769
[SYCL] Fix missing dependency on UR headers #16261
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
As discussed in this unrelated patch, changing the UR headers is not properly reflected in the build graph. |
sycl/CMakeLists.txt
Outdated
@@ -243,6 +243,11 @@ add_custom_target(sycl-headers | |||
sycl-device-aspect-macros-header | |||
boost_mp11-headers) | |||
|
|||
list(APPEND UR_HEADERS |
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.
It looks like UR_HEADERS
is originally defined in UR itself here and these three files are UR files, should the fix instead be in UR itself rather than SYCL? This is fine as a temporary fix though
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.
The fix is really not to copy headers to sycl's build directory in the first place, but use target_include_directories(PRIVATE
. The whole way we do dependencies isn't proper cmake, and I need to flense the whole thing. This is temporary
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 could change that variable name, though. Didn't realise it clashed
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 dont know enough about cmake anyway, i thought it was intentionally clashing :P
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.
Updated to use the delightfully quaint "UR_HEADERS_TO_COPY"
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.
lgtm as temp fix
No description provided.