Build: Correct LICENSE paths for wheels and document the bundled Cython runtime - #3893
Build: Correct LICENSE paths for wheels and document the bundled Cython runtime#3893shaynhornik wants to merge 1 commit into
Conversation
…on runtime The wheel ships fb303 and hive_metastore as top-level packages (setup.py package_dir), so the LICENSE entries pointing at vendor/ were wrong in binary distributions, and the compiled decoder_fast module embeds Cython runtime code that was not documented. Reword both vendored entries to describe each layout and add a Cython entry, raised in the 0.12.0 release vote thread. Generated-by: Claude Code (Fable 5) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@jbonofre Could you review this PR? |
kevinjqliu
left a comment
There was a problem hiding this comment.
We build 2 different kinds of artifacts, the wheels (.whl) and source distribution (tar.gz).
See https://pypi.org/project/pyiceberg/#files
I think both use the same LICENSE file, so we need to sure that the location referenced inside the LICENSE text matches both types of artifacts
|
Agreed that one LICENSE has to be right for both artifacts, which is why each entry names both layouts rather than replacing one path with the other. I built both from this branch to confirm (
Happy to reword if you'd prefer a different phrasing for the two layouts. |
kevinjqliu
left a comment
There was a problem hiding this comment.
Thanks for picking this up, @shaynhornik! I verified the path changes by building both artifacts from a local checkout, and the new wording checks out.
fb303 / hive_metastore wording: correct for both layouts
The two packages are discovered under vendor/ and then remapped by package_dir in setup.py, so the sdist keeps the on-disk path while the wheel installs them by import name.
sdist (pyiceberg-0.12.0.tar.gz), with LICENSE at the tarball root:
pyiceberg-0.12.0/LICENSE
pyiceberg-0.12.0/vendor/fb303/{__init__,constants,ttypes,FacebookService}.py
pyiceberg-0.12.0/vendor/hive_metastore/{__init__,constants,ttypes,ThriftHiveMetastore}.py
wheel (pyiceberg-0.12.0-cp313-*.whl), no vendor/ directory anywhere:
fb303/{__init__,constants,ttypes,FacebookService}.py
hive_metastore/{__init__,constants,ttypes,ThriftHiveMetastore}.py
pyiceberg-0.12.0.dist-info/licenses/LICENSE
pyiceberg-0.12.0.dist-info/top_level.txt -> fb303, hive_metastore, pyiceberg
So "vendor/fb303/ in the source distribution" is a correct relative path from LICENSE in the sdist, and "top-level fb303 package in binary distributions" matches top_level.txt in the wheel. Same for hive_metastore. 👍
Cython entry: two options
I looked into how this is handled elsewhere:
- Cython's own COPYING.txt says compiled output "is NOT considered a derivative work of Cython" and the embedded snippets "do not encumber the resulting output with any license restrictions."
- The ASF licensing howto says an ALv2 dependency with no NOTICE needs no LICENSE change. Listing is optional, "for completeness."
- No ASF project shipping Cython wheels lists it (Arrow, Beam, Flink, Kudu, nanoarrow, Fory, the Cassandra python driver), and neither do numpy, pandas, scipy, scikit-learn, lxml, or PyYAML.
- "The Cython Project Developers" isn't wording Cython uses anywhere. Its LICENSE.txt has no copyright line at all.
Given that, I see two reasonable choices:
- Drop the Cython section and keep this PR to the path fixes, with the reasoning above recorded on #3877. This matches every other project surveyed and Cython's own statement. This is my preference.
- Keep the entry for completeness, but remove the Copyright line (or use Cython's actual author list) and add a sentence citing COPYING.txt so the next release audit doesn't have to re-derive why there's no NOTICE change.
Happy to go either way. Thanks again!
|
cc @ jbonofre thoughts on the cython based on the above comment? |
Closes #3877
Rationale for this change
Follow-up to the 0.12.0 release vote thread findings on the wheel's LICENSE documentation:
Vendored paths were wrong in binary distributions.
setup.pymapsvendor/fb303andvendor/hive_metastoreto top-levelfb303andhive_metastorepackages viapackage_dir, so the LICENSE entries saying the code lives "in vendor/fb303/" were only accurate for the source distribution. Both entries now describe each layout explicitly ("located in vendor/… in the source distribution and shipped as the top-level … package in binary distributions"), so the one LICENSE file thatlicense-filesships into both artifacts is accurate in both.The bundled Cython runtime was undocumented. The compiled
pyiceberg.avro.decoder_fastextension embeds the Cython runtime code that Cython generates into every compiled module. Cython is Apache-2.0 licensed with no NOTICE file, so a LICENSE entry (no NOTICE change) documents it, following the format of the existing entries.Are these changes tested?
Verified by building a wheel from this branch and inspecting it:
fb303/andhive_metastore/are at the wheel root (novendor/directory), matching the new wording.pyiceberg-0.12.0.dist-info/licenses/LICENSEwith both fixes present.stringson the builtdecoder_fast.soconfirms embedded Cython runtime code.dev/check-license(RAT) checks source headers only, so it is unaffected.Are there any user-facing changes?
No code changes; LICENSE documentation only.
AI disclosure: this change was developed with the assistance of Claude Code, per the repository's agent workflow (AGENTS.md). I reviewed the analysis and the wording, and verified the built wheel myself; I take responsibility for the contribution.