Skip to content
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

Build against sytem dependancies if available #97

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Calandracas606
Copy link
Contributor

If CLHPP, ICD-Loader, and Headers are installed on the system, use those instead of the sub-modules. This is beneficial when packaging the SDK for distribution, as the sub-modules can be built as their own separate packages.

While packaging for Void Linux, I ran into the issue of the ICD-Loader, Headers, and CLHPP being sub-modules. Since those dependencies can all be build and packaged independently, of the SDK, it is desirable to keep the separate.

Additionally, the release tarball does not contain the sub modules, so they must be fetched separately when packaging.

This PR is a very simple workaround that adds a check to see if those dependencies are already installed on the system, and to use the system packages instead if they are available.

Possible improvements that could be made to this PR if desired:

  1. Control this behaviour with CMake flags
  2. Check if a CMakelists.txt exists in the external/OpenCL-* folders (when building from a tarball, those folders are empty)
  3. Add CMake external project functionality for fetching the CLHPP, ICD-Loader, and Headers if they are missing (such as when building from tarball)

@CLAassistant
Copy link

CLAassistant commented Jan 18, 2024

CLA assistant check
All committers have signed the CLA.

If CLHPP, ICD-Loader, and Headers are installed on the system, use
those instead of the submodules. This is beneficial when packaging
the SDK for distribution, as the submodules can be built as their
own seperate packages.
@MathiasMagnus
Copy link
Collaborator

Hi @Calandracas606, apologies for the gravely belated reaction. I recall seeing this PR but replying fell off the stack. I have one issue with this change: it's going to break some (non-exotic).

Most commonly: you're driving Ubuntu and you have the ocl-icd-dev package, or any other package installed that let's you detect an ICD loader, but still want to develop the SDK (and another important use-case coming soon, which involves system packages conflicting with development which I'm unauthorized to disclose at this point). That becomes much harder, because find_package(OpenCLHeaders) at this point will always succeed and it will never pick up the source tree that you want to develop with. Using a new clone of the SDK but 2 year old snapshots of components (Ubuntu LTS) is asking for trouble. In these cases, when a vanilla find_package always succeeds, one has to opt-out of this feature by using CMAKE_FIND_USE_CMAKE_SYSTEM_PATH which globally affects all find_package() invocations, including those of the samples.

Additionally, the release tarball does not contain the sub modules, so they must be fetched separately when packaging.

We've gone around this issue by releasing our own source packages. Does this cover your use case? If you are really looking to get source-code using the default naming scheme, consider contributing to the issue/discussion here, we're at the mercy of GitHub features here.

@Calandracas606
Copy link
Contributor Author

Would adding an option such as -DUSE_SYSTEM_{OPENCL_HEADERS,OPENCL_HPP,OCL_ICD} be a possible solution?

That would keep the default behavior the same, but gives distributions the option to use system dependencies if preferred.

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.

3 participants