KhronosGroup / OpenCL-Headers

Khronos OpenCL-Headers
Apache License 2.0
666 stars 236 forks source link

OpenCL-SDK base changes #189

Closed MathiasMagnus closed 2 years ago

MathiasMagnus commented 2 years ago

This PR supports OpenCL-SDK changes. The PR contains:

MathiasMagnus commented 2 years ago

@sl1pkn07 I can't seem to find the issue where you challanged the necessity of SYSTEM BEFORE. As it turns out (now that I revisited the issue), that was a desperate attempt at making MacOS happy, so that the C++ bindings pick up our headers and not the system ones.

Install rules have been added specific to MacOS that hide the system headers when our package is used. No more flaky SYSTEM BEFORE is required to control precedence of header paths. (Note that XCode and Ninja behave differently in this regard, as XCode adds a whole bunch of args to the command-line, so it was tricky to find one silver bullet that covers all use cases. And some are still not fully covered, but we'll improve CI coverage over time.)

sl1pkn07 commented 2 years ago

you mean on this https://github.com/KhronosGroup/OpenCL-ICD-Loader/pull/115 ?

MathiasMagnus commented 2 years ago

I don't see SYSTEM BEFORE there, but in one incarnation of the CMake Packae Config support it was there and it was because of MacOS. This PR supercedes that series of PRs. The SDK samples base changes all have Package Config support and more fleshed out CI.

sl1pkn07 commented 2 years ago

sorry, i'm completely lost. I think you mistaken of person

i'm never talking about of SYSTEM BEFORE. but i have reviews pending about other things in KhronosGroup/OpenCL-ICD-Loader#115 and https://github.com/KhronosGroup/OpenCL-Headers/pull/107 (merged)

greetings

MathiasMagnus commented 2 years ago

The OpenCL-ICD-Loader case changes PR is looking ready, the last thing which needs attending to is remove the temporary redirection to this PR's source branch. This PR needs to go through before the ICD base changes can.

This already received a LGTM from Ben, I addressed his few concerns and fixed MacOS installation. I think it's good to go, but please take a look.