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

New package: OpenCL-SDK #48261

Closed
wants to merge 3 commits into from

Conversation

Calandracas606
Copy link

@Calandracas606 Calandracas606 commented Jan 17, 2024

Testing the changes

  • I tested the changes in this PR: briefly

New package

Local build testing

  • I built this PR locally for my native architecture, x86_64-musl
  • I built this PR locally for these architectures (if supported. mark crossbuilds):
    • x86_64-glibc
    • aarch64-musl (cross)
    • armv7l (cross)
    • armv6l-musl (cross)

@Calandracas606 Calandracas606 force-pushed the OpenCL-SDK branch 3 times, most recently from 29d30ac to c05955c Compare January 18, 2024 03:56
@ahesford
Copy link
Member

Please tag maintainers when you propose to restructure their packages, especially if "restructure" means "kidnap".

What benefit accrues from the SDK as packaged? The major bits are available as standalone packages and could be built separately as independent packages. The SDK itself just makes for an awkward template and adds a utility library that doesn't seem to be used anywhere. It's usually desired to wait until some "end-user" application to be packaged for Void requires libraries (including header-only libraries) before packaging the libraries themselves, so CLHPP and the utility library could just as well wait until they are needed.

The ICD loader may have some value, but what is the argument for replacing the existing loader with this? Yes, it comes from Khronos, but yanking an existing package out from users and replacing it with another may not be the right choice.

@Calandracas606
Copy link
Author

Please tag maintainers when you propose to restructure their packages, especially if "restructure" means "kidnap".

My apologies, I should have contacted you prior to submitting this PR

What benefit accrues from the SDK as packaged? The major bits are available as standalone packages and could be built separately as independent packages. The SDK itself just makes for an awkward template and adds a utility library that doesn't seem to be used anywhere. It's usually desired to wait until some "end-user" application to be packaged for Void requires libraries (including header-only libraries) before packaging the libraries themselves, so CLHPP and the utility library could just as well wait until they are needed.

You're right, it isn't currently needed by any packages. Existing void packages only use OpenCL-Headers, so CLHPP and utility libraries are only useful as a development tool.

The ICD loader may have some value, but what is the argument for replacing the existing loader with this? Yes, it comes from Khronos, but yanking an existing package out from users and replacing it with another may not be the right choice.

There is nothing wrong with the existing ICD loader. I only changed it because this one comes from Khronos

Closing because It is clear that this PR is overzealous, and isn't currently a good fit for inclusion in void.

@ahesford
Copy link
Member

ahesford commented Jan 18, 2024

I wouldn't go that far.

The Khronos ICD loader might be superior to ocl-icd (or at least a desirable alternative and we might still want to package it, but we should consider consider whether 1) if ocl-icd will live on in its own right, 2) if so, whether we want to continue support it, and 3) if so, whether we should move all of its dependents to the new option at this point.

If we offer both ICD loaders, they would either need to conflict or present alternatives. If they conflict, we should build all of our packages with whatever the preferred loader is (otherwise, some combination of consumers would be uninstallable). If they offer alternatives, we will need to alter build process.

As for the utilities offered by the SDK repo, my preference for ease of packaging would be to convince upstream to allow building of the extras against "system" versions of its git submodules, allowing us to build the SDK as an add-on. Submodules are kind of a pain in xbps-src, and hard-requiring them as most of the substance of a project seems like strange project management.

If upstream is recalcitrant but we still find a need or desire for the SDK utilities, then I suppose we bite the bullet and subsume the headers and ICD loader into one template as you've proposed.

@Calandracas606
Copy link
Author

Reopening to allow to further discussion.

I'll update this PR later to address some of the concerns with the build system. A quick glance suggests it should be straightforwards to patch CMakelists to use system libraries, even with a vsed

@Calandracas606
Copy link
Author

I've restructured the packages into separate templates. This required a small patch for OpenCL-SDK to use the system dependencies rather than the submodules.

I will need to work on a more thorough patch if I am to submit upstream

The question remains, what to do with ocl-icd? My assumption is that it predates the Khronos OpenCL ICD Loader

@Calandracas606
Copy link
Author

Closing in favor of just adding OpenCL-CLHPP #48710

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-package This PR adds a new package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants