Skip to content

Build: Correct LICENSE paths for wheels and document the bundled Cython runtime - #3893

Open
shaynhornik wants to merge 1 commit into
apache:mainfrom
shaynhornik:fix/whl-license-paths
Open

Build: Correct LICENSE paths for wheels and document the bundled Cython runtime#3893
shaynhornik wants to merge 1 commit into
apache:mainfrom
shaynhornik:fix/whl-license-paths

Conversation

@shaynhornik

Copy link
Copy Markdown

Closes #3877

Rationale for this change

Follow-up to the 0.12.0 release vote thread findings on the wheel's LICENSE documentation:

  1. Vendored paths were wrong in binary distributions. setup.py maps vendor/fb303 and vendor/hive_metastore to top-level fb303 and hive_metastore packages via package_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 that license-files ships into both artifacts is accurate in both.

  2. The bundled Cython runtime was undocumented. The compiled pyiceberg.avro.decoder_fast extension 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/ and hive_metastore/ are at the wheel root (no vendor/ directory), matching the new wording.
  • The updated LICENSE lands at pyiceberg-0.12.0.dist-info/licenses/LICENSE with both fixes present.
  • strings on the built decoder_fast .so confirms 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.

…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>
@ebyhr

ebyhr commented Sep 3, 2026

Copy link
Copy Markdown
Member

@jbonofre Could you review this PR?

@kevinjqliu kevinjqliu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@shaynhornik

Copy link
Copy Markdown
Author

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 (python -m build --sdist --wheel):

  • sdist (pyiceberg-0.12.0.tar.gz): the Thrift and Hive code is under vendor/fb303/ and vendor/hive_metastore/, matching "located in vendor/… in the source distribution".
  • wheel (pyiceberg-0.12.0-cp311-…whl): setup.py's package_dir remaps them, so they land as top-level fb303/ and hive_metastore/ with no vendor/ directory at all, matching "shipped as the top-level … package in binary distributions".
  • The identical LICENSE text is at the sdist root and at pyiceberg-0.12.0.dist-info/licenses/LICENSE in the wheel.
  • The Cython entry is scoped to "binary distributions" because the sdist excludes the generated decoder_fast.c (MANIFEST.in) and ships only the .pyx; the wheel's compiled .so is where the Cython runtime code appears.

Happy to reword if you'd prefer a different phrasing for the two layouts.

@kevinjqliu kevinjqliu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. 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!

@kevinjqliu

Copy link
Copy Markdown
Contributor

cc @ jbonofre thoughts on the cython based on the above comment?

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.

0.12 follow up license fixes

3 participants