-
Notifications
You must be signed in to change notification settings - Fork 6
Include zstd license in distributions #43
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
Conversation
|
🔗 Link to GitHub Actions run building the wheels as artifacts Example with |
|
LGTM, although you only seem to be including one of ztsd's license. Is this intentional? |
|
Thanks for checking out! My understanding is that I have the right to choose one of the two licenses of zstd library, and so I choose the one which is the same as pyzstd's license. This avoids shenanigans on the value of the license mentioned in several metadata of the sdist and wheels. |
Fair enough. Thanks for the fix! |
|
In NixOS/nixpkgs#449557 I'm seeing: Full trace
Because there is no I assume because we package Either way, it seems like a bug that the build script would fail with a FileNotFoundError here. We can apply patches in nixpkgs (or fetch patches from upstream PRs/commits), so as soon as I know the correct approach we can deal with it. |
|
Hello @MattSturgeon, thank you for reporting the issue. After giving it some thoughts, I think that this is a side-effect of this PR indeed. More precisely, that your build script was working before but needs to be changed slightly due to it. I think that what happens is that when the git clone is performed, the git submodules are not initialized and cloned. As a result, the zstd source code is not available in the |
|
Thanks for the explanation. That makes sense. We can simply pass However, seeing as we're building pyzstd with
Therefore I'd prefer to avoid fetching the submodules unnecessarily. I can solve (1) by doing I would expect
Unless you're telling me we still need the zstd license in this scenario, I'll try and come up with a PR to exclude the zstd license when built with a dynamically linked zstd. (PR: #51) |
I think this is correct
Even with I am not a layer, and I have no clue if this is enough to require the license to be included next to the artifact. The fuzzy wording on top of the generated
Thank you for the PR. As mentioned there, I will need to think this some more. In any case, I will not make a release in the coming week due to other priorities, so if you want a faster resolution, I suggest to go with alternatives like one of those you mentioned. Another possibility would be to get the source distribution from PyPi instead of cloning from github (you would not have the issue there because the sdist ships the necessary files from zstd). |
Fixes #42.