Skip to content

Conversation

@Kerilk
Copy link
Contributor

@Kerilk Kerilk commented Oct 10, 2022

This adds pkg-config support to the OpenCL ICD Loader package.
This fixes #187

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a pkgconfig expert so please bear with my (probably basic) questions.

Should we also have a dependency on a similar OpenCL headers package?


Name: OpenCL
Description: Khronos OpenCL ICD Loader
Version: 3.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming: It's OK if this version number is different than our SOVERSION?

If so, this is another place that would be great to automate with #182!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming: It's OK if this version number is different than our SOVERSION?

This mimics what ocl-icd does here: https://github.com/OCL-dev/ocl-icd/blob/master/OpenCL.pc.in where @OPENCL_VERSION@ is currently evaluated to 3.0

If so, this is another place that would be great to automate with #182!

Indeed

@Kerilk
Copy link
Contributor Author

Kerilk commented Oct 10, 2022

I'm not a pkgconfig expert so please bear with my (probably basic) questions.

My knowledge is very basic as well.

Should we also have a dependency on a similar OpenCL headers package?

Excellent question. Here, the .pc file is for both library and headers assuming both are installed using the same prefix. This is the same in ocl-icd. I agree that conceptually, the headers and the loader should be kept separate. In practice, installing the two alongside each other makes sense, as building and linking requires both.
In debian the the .pc file is packaged in the ocl-icd-opencl-dev package:
https://packages.debian.org/bullseye/amd64/ocl-icd-opencl-dev/filelist
Which contains the .so symlink and the .pc file.

I assume we could split it in two, one for the headers and one for the library and make the latter one depend on the former. I'll try experimenting with this.

@bashbaug
Copy link
Contributor

Probably a good idea to get input from @MathiasMagnus before we go too far. I'm not sure if it makes sense to have multiple pkgconfig files for each of the OpenCL components, or a single pkgconfig file for the OpenCL SDK, or both.

@Kerilk
Copy link
Contributor Author

Kerilk commented Oct 10, 2022

Probably a good idea to get input from @MathiasMagnus before we go too far. I'm not sure if it makes sense to have multiple pkgconfig files for each of the OpenCL components, or a single pkgconfig file for the OpenCL SDK, or both.

Indeed. I tested and split the .pc file between the Headers and Loader, and it worked as expected. I made a PR for the Header as well should we want to take this route: KhronosGroup/OpenCL-Headers#213

Co-authored-by: Markus Mützel <[email protected]>
@MathiasMagnus
Copy link
Contributor

I have less experience with pkg-config (because I mostly use Windows), but I do understand some users would appreciate this.

My only input here is that whatever type of consumption we envision using .pc files, we should test in CI. There are consumption tests in CI where we test that we don't forget to install anything that users will be looking for inside build trees, and that the contents of the install tree are usable. If we start deploying .pc files, I think we should test that their contents are usable and they are deployed in the proper places (and that it stays that way as the repo progresses).

I would feel comfortable approving once CI is in place for this. We can do so without it and say it's a "best effort" basis, but that may invite lots of issues from people wanting to use it in territories which we could otherwise cover in CI.

@mmuetzel
Copy link
Contributor

A test in CI could maybe look similar to something like this:

PKG_CONFIG_PATH="/path/to/where/OpenCL.pc/was/installed" pkg-config --libs OpenCL
PKG_CONFIG_PATH="/path/to/where/OpenCL.pc/was/installed" pkg-config --cflags OpenCL

It might also be worth to check what those commands return. But that might depend a lot on the platform, install prefix, ...

Copy link
Contributor

@MathiasMagnus MathiasMagnus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally would've aimed at tests which don't just run a regular expression on the output of pkg-config, but actually try to compile something with them. The same pkgconfig.c source file can be used for that which we have both here and in the OpenCL-Headers repo. pkg-config also exists on Windows, albeit nobody uses it, so the lack of support for that shouldn't be a dealbreaker. We may want to open an issue so we don't lose sight of that feature missing.

Otherwise I wouldn't want to block this, as it's already useful to some, and we do have deployment tests for the .pc files so at least that won't be broken by mistake, and the c

@jenatali
Copy link
Contributor

pkg-config also exists on Windows, albeit nobody uses it

I do just want to point out that Meson, which is typically used for Linux projects, does also have pretty decent Windows support, and does use pkg-config as one of its core dependency lookup methods. So, yeah we do actually use pkg-config on Windows for a few dependencies, specifically libclc from LLVM, and SPIRV-Tools. That said, I don't have a stake in whether this project supports it on Windows or not.

@Kerilk
Copy link
Contributor Author

Kerilk commented Oct 18, 2022

@MathiasMagnus, @jenatali thanks for the feedback. As I see it we have 3 remaining questions:

  1. Should we support it on Windows? I am all in favor but need help doing it right and testing it.
  2. Do we want the two .pc file or the single .pc file approach? I reached out to Andreas Beckmann who is very active in Debian OpenCL packaging for feedback on this issue. I am worried about causing issues for distribution maintainers.
  3. Do we want to add a build/run test to validate the content of the pkg-config file. I will try. Added the test, it requires a symlink for now as headers are installed in another location. This will be replaced by adding to the PKG_CONFIG_PATH if we chose to split.

@jenatali
Copy link
Contributor

Sure, I can help test it.

@Kerilk Kerilk force-pushed the pkg-config branch 3 times, most recently from 7b522eb to edd5223 Compare October 18, 2022 19:05
@Kerilk
Copy link
Contributor Author

Kerilk commented Oct 18, 2022

@MathiasMagnus any idea of what could be wrong with the cmake minimum? I don't see anything in the documentation that implies a change in the variables I am using.

@MathiasMagnus
Copy link
Contributor

MathiasMagnus commented Oct 18, 2022

@Kerilk Take a look at this PR to your branch. I believe it should solve the issue.

Mind you, I went ahead and tested in a container where I cloned the Headers working branch of pkg-config support. I don't know if merging all the components into the OpenCL pkg-config (including the include dir, which is set to CMAKE_INSTALL_INCLUDEDIR will make sense outside the context of installing everything to a common directory. If we are to require the same module hierarchy as the CMake parts, we should keep the headers and the loader be separate modules. The SDK could still be a super-project that installs .pc files to allow consuming everything all at once (should we want to do that in pkg-config land too.)

@StuartDBrady
Copy link

StuartDBrady commented Nov 29, 2022

I haven't followed the detail of the discussion here, but see gl.pc on a typical Linux distribution for an example of how this is done for GL, which typically has a dispatch layer similar to the ICD for OpenCL. Note that there is generally a consistent libdir and includedir under a single prefix, specified under a common pkgconfig file. I would strongly recommend talking to package maintainers before doing otherwise for OpenCL.

If this isn't sufficient for OpenCL, then I would ask why that is:

  • why isn't this a problem for GL and Vulkan too?
  • if it is a problem for GL and Vulkan, does this point at a more general problem with pkgconfig?
  • if it is a general problem with pkgconfig's design, then how is this worked around for other libraries? Why should the OpenCL ICD and headers be the one special case where we don't expect a common prefix? As stated, GL has a similar architecture.

@Kerilk
Copy link
Contributor Author

Kerilk commented Nov 29, 2022

@StuartDBrady Thanks for the insights. I have reached out to the Debian OpenCL maintainer mailing list: pkg-opencl-devel . I someone with ties to other Linux distributions want to reach out as well, please do so.

If this isn't sufficient for OpenCL, then I would ask why that is:

* why isn't this a problem for GL and Vulkan too?

* if it is a problem for GL and Vulkan, does this point at a more general problem with pkgconfig?

* if it is a general problem with pkgconfig's design, then how is this worked around for other libraries?  Why should the OpenCL ICD and headers be the one special case where we don't expect a common prefix?  As stated, GL has a similar architecture.

I think in this case the difference comes from two aspects:

  • There exists 2 OpenCL ICD Loaders which, as they share library names cannot be installed in the same prefix. On distributions, you can solve the issue with update-alternatives if you wish them to coexist, or just rely on the package manager to switch them for you. If using environment modules this becomes more complicated.
  • OpenCL is used in the HPC world (more so than OpenGL and Vulkan for now) and it relies heavily on environment modules.

Other libraries have usually a single source for headers and libraries, or ship their own headers with their implementation (see MPI). If we wanted to take this route, then the headers should become a sub-module of the loader and always be installed side by side. This is not the choice we have currently made, and even not how distributions handle OpenCL headers and loaders as headers are shared between loaders (see https://packages.debian.org/stretch/opencl-dev for how it used to be done in debian, only ocl-icd remains nowadays)

@bashbaug
Copy link
Contributor

Merging as discussed in the OpenCL teleconferences. This will give time for testing before the next release.

@bashbaug bashbaug merged commit 6ceb5d2 into KhronosGroup:main Jan 10, 2023
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.

Please add OpenCL.pc so pkg-config can find this package

7 participants