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

Package Khronos OpenCL ICD loader for Windows #1763

Merged
merged 10 commits into from
Nov 3, 2016

Conversation

inducer
Copy link
Contributor

@inducer inducer commented Oct 13, 2016

I did this just to see what would happen. Don't blame me.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/khronos-opencl-icd-loader) and found it was in an excellent condition.

@inducer
Copy link
Contributor Author

inducer commented Oct 15, 2016

Surprisingly: Looks good from my end. Ready to review.

@inducer inducer changed the title Package Khronos OpenCL ICD loader for Windows [WIP] Package Khronos OpenCL ICD loader for Windows Oct 15, 2016
@inducer
Copy link
Contributor Author

inducer commented Oct 18, 2016

This package is reasonably straightforward with the one caveat that they have cleverly decided to manufacture their own license. The license itself is probably not technically DFSG-free, but it does not impose any unreasonable restrictions on end users:

https://github.com/KhronosGroup/OpenCL-ICD-Loader/blob/master/LICENSE.txt

What can I do to help this get merged? Having this merged would (likely) allow me to have Windows builds of pyopencl.

@inducer
Copy link
Contributor Author

inducer commented Oct 24, 2016

(bump)

@inducer
Copy link
Contributor Author

inducer commented Nov 2, 2016

@conda-forge/core Could anyone take a look at this PR? It's ready to go as far as I can tell.


build:
number: 0

Copy link
Member

Choose a reason for hiding this comment

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

The comment below and the extra space seems unnecessary, but feel free to keep it if you disagree as that does not prevent us from merging this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather keep the comment--and the empty line for visual separation, to help explain why this is Windows-only.


requirements:
build:
- toolchain
Copy link
Member

Choose a reason for hiding this comment

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

This is unnecessary. At the end of the day the toolchain only changes, and in an irreproducible way since it is not versioned, the builds on OS X.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Should be fine to drop as Windows only. toolchain is a no-op there.

That being said, I disagree with @ocefpaf on his statements w.r.t. the toolchain generally.

about:
home: https://www.khronos.org/registry/cl/
dev_url: https://github.com/KhronosGroup/OpenCL-ICD-Loader
license: Potentially non-free semi-copyleft dumpster fire
Copy link
Member

Choose a reason for hiding this comment

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

Oh Oh...

Copy link
Contributor Author

@inducer inducer Nov 2, 2016

Choose a reason for hiding this comment

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

Commented on this above:

The license itself is probably not technically DFSG-free, but it does not impose any unreasonable restrictions on end users:

https://github.com/KhronosGroup/OpenCL-ICD-Loader/blob/master/LICENSE.txt

What's conda-forge's policy regarding weird (but not necessarily malicious) licenses?

Copy link
Member

Choose a reason for hiding this comment

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

In theory all we need is permission to package and re-distribute binaries. But people usually stay away from non-conventional because we are not lawyers 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, makes sense. IMO, the license is a bit wacky, but it does not prevent packaging and binary redistribution. It also doesn't expose mere users to any risk, so I think this should be OK.

Copy link
Member

Choose a reason for hiding this comment

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

We have packaged things with non-standard licenses. Though I completely understand the concern. From what I can tell this seems like a modified BSD 3-Clause license though IANAL.

Though maybe we should call it "The Khronos Group License" or something else. We can have this discussion in the feedstock though.

Copy link
Member

Choose a reason for hiding this comment

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

The only lines that stood out to me were these.

If the binary is used as part of an OpenCL(TM) implementation, whether binary
is distributed together with or separately to that implementation, then
recipient must become an OpenCL Adopter and follow the published OpenCL
conformance process for that implementation, details at:
http://www.khronos.org/conformance/;

However, these seem to be obligations on the maintainer.

@jakirkham jakirkham merged commit 1d28c97 into conda-forge:master Nov 3, 2016
@inducer
Copy link
Contributor Author

inducer commented Nov 3, 2016

Thanks for reviewing this PR, @jakirkham and @ocefpaf!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants