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

[Bug]: LICENSE_QHULL is included in wheel only the second time #25212

Closed
ret2libc opened this issue Feb 14, 2023 · 10 comments · Fixed by #25364
Closed

[Bug]: LICENSE_QHULL is included in wheel only the second time #25212

ret2libc opened this issue Feb 14, 2023 · 10 comments · Fixed by #25364
Labels
Build Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Milestone

Comments

@ret2libc
Copy link

ret2libc commented Feb 14, 2023

Bug summary

The first time I run python3.10 setup.py bdist_wheel on both 3.6.0 and 3.7.0, the resulting wheel does not include matplotlib-0.0.0.dist-info/LICENSE_QHULL. The second time the LICENSE_QHULL file is included.

Code for reproduction

$ python3.10 setup.py bdist_wheel
$ unzip -l dist/matplotlib-0.0.0-cp310-cp310-linux_aarch64.whl  | grep LICENSE_QHULL

Actual outcome

$ python3.10 setup.py bdist_wheel
$ unzip -l dist/matplotlib-0.0.0-cp310-cp310-linux_aarch64.whl  | grep LICENSE_QHULL

Expected outcome

$ python3.10 setup.py bdist_wheel
$ unzip -l dist/matplotlib-0.0.0-cp310-cp310-linux_aarch64.whl  | grep LICENSE_QHULL
     1720  02-14-2023 10:54   matplotlib-0.0.0.dist-info/LICENSE_QHULL

Additional information

No response

Operating system

quay.io/pypa/manylinux_2_28_aarch64

Matplotlib Version

3.6.0 and 3.7.0

Matplotlib Backend

No response

Python version

3.8 and 3.10

Jupyter version

No response

Installation

git checkout

@oscargus oscargus added the Build label Feb 14, 2023
@oscargus
Copy link
Contributor

oscargus commented Feb 14, 2023

I cannot reproduce it locally (although with Python 3.9). Does the log print that it adds the license files? I get this at the end of the log and it seems to be in the wheel as well

adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/LICENSE'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/LICENSE_AMSFONTS'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/LICENSE_BAKOMA'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/LICENSE_CARLOGO'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/LICENSE_COLORBREWER'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/LICENSE_COURIERTEN'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/LICENSE_JSXTOOLS_RESIZE_OBSERVER'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/LICENSE_QHULL'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/LICENSE_QT4_EDITOR'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/LICENSE_SOLARIZED'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/LICENSE_STIX'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/LICENSE_YORICK'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/METADATA'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/WHEEL'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/namespace_packages.txt'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/top_level.txt'
adding 'matplotlib-3.7.0.dev1620+g76dd07e.dist-info/RECORD'
removing build/bdist.linux-x86_64/wheel

Any other license files missing?

@oscargus oscargus added the status: needs clarification Issues that need more information to resolve. label Feb 14, 2023
@ret2libc
Copy link
Author

Did you already have the Qhull code/license downloaded? The problem I see happens only the first time, so a git clean -dxff . should help reproduce the problem. I get the same wrong behaviour with py3.9 on a clean manylinux container.

@tacaswell tacaswell added this to the v3.7.1 milestone Feb 14, 2023
@tacaswell tacaswell removed the status: needs clarification Issues that need more information to resolve. label Feb 14, 2023
@tacaswell
Copy link
Member

This is a race condition between the qhull custom build step (which puts the license file in the right place) in setupext.py and the manifest generation (which seems to be controlled via setup.cfg).

Hopefully there is some nice setuptools hook we can call as part of the copying to re-discover the files (or just manually update the manifest metadata).

@ksunden
Copy link
Member

ksunden commented Feb 14, 2023

The following wheels from the 3.7.0 final release DO NOT include LICENSE_QHULL:

matplotlib-3.7.0-cp310-cp310-manylinux_2_17_aarch64.manylinux2014_aarch64.whl
matplotlib-3.7.0-cp310-cp310-manylinux_2_17_i686.manylinux2014_i686.whl
matplotlib-3.7.0-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
matplotlib-3.7.0-cp311-cp311-manylinux_2_17_aarch64.manylinux2014_aarch64.whl
matplotlib-3.7.0-cp311-cp311-manylinux_2_17_i686.manylinux2014_i686.whl
matplotlib-3.7.0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
matplotlib-3.7.0-cp311-cp311-win32.whl
matplotlib-3.7.0-cp38-cp38-manylinux_2_12_i686.manylinux2010_i686.whl
matplotlib-3.7.0-cp38-cp38-manylinux_2_12_x86_64.manylinux2010_x86_64.whl
matplotlib-3.7.0-cp38-cp38-manylinux_2_17_aarch64.manylinux2014_aarch64.whl
matplotlib-3.7.0-cp39-cp39-manylinux_2_17_aarch64.manylinux2014_aarch64.whl
matplotlib-3.7.0-cp39-cp39-manylinux_2_17_i686.manylinux2014_i686.whl
matplotlib-3.7.0-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
matplotlib-3.7.0-pp38-pypy38_pp73-manylinux_2_17_i686.manylinux2014_i686.whl
matplotlib-3.7.0-pp38-pypy38_pp73-manylinux_2_17_x86_64.manylinux2014_x86_64.whl

i.e. all of the manylinux wheels except pypy 3.9 plus one 32 bit windows wheel

@tacaswell
Copy link
Member

We should fix this for 3.7.1 (which I expect to be with in the month) and going forward.

@ksunden
Copy link
Member

ksunden commented Feb 14, 2023

Same results for 3.7.0rc1 wheels, but not for wheels generated on the 3.6.x branch, which had all of the qhull license files (I just pulled the zip from the "zenodo doi" run shortly after 3.6.3 was released)

I spot checked a couple manylinux 3.6.3 wheels from pypi and found they had LICENSE_QHULL.
So this does seem to be relatively recent.

@ret2libc
Copy link
Author

I spot checked a couple manylinux 3.6.3 wheels from pypi and found they had LICENSE_QHULL. So this does seem to be relatively recent.

Actually the problem for me happens also in 3.6.0, when built from source. PyPI does have the LICENSE_QHULL file, but it doesn't if you re-build it (from clean env) from the git tag v3.6.0.

@ksunden
Copy link
Member

ksunden commented Feb 14, 2023

Well, it may be true that newly built wheels have the same problem, it may still be caused by a relatively recent toolchain update, which is useful information.

@ksunden
Copy link
Member

ksunden commented Feb 15, 2023

diff --git a/setup.py b/setup.py
index bbaa56f3d6..f3b3b29c5b 100644
--- a/setup.py
+++ b/setup.py
@@ -42,12 +42,13 @@ from setupext import print_raw, print_status
 
 
 # These are the packages in the order we want to display them.
+qhull_setup = setupext.Qhull()
 mpl_packages = [
     setupext.Matplotlib(),
     setupext.Python(),
     setupext.Platform(),
     setupext.FreeType(),
-    setupext.Qhull(),
+    qhull_setup,
     setupext.Tests(),
     setupext.BackendMacOSX(),
     ]
@@ -267,6 +268,8 @@ if not (any('--' + opt in sys.argv
             package_data.setdefault(key, [])
             package_data[key] = list(set(val + package_data[key]))
 
+qhull_setup.download()
+
 setup(  # Finally, pass this all along to setuptools to do the heavy lifting.
     name="matplotlib",
     description="Python plotting package",
diff --git a/setupext.py b/setupext.py
index 7cd5216a59..acba7ea70e 100644
--- a/setupext.py
+++ b/setupext.py
@@ -756,7 +756,7 @@ class Qhull(SetupPackage):
         else:
             cls._extensions_to_update.append(ext)
 
-    def do_custom_build(self, env):
+    def download(self):
         if options.get('system_qhull'):
             return
 
@@ -767,6 +767,12 @@ class Qhull(SetupPackage):
         )
         shutil.copyfile(toplevel / "COPYING.txt", "LICENSE/LICENSE_QHULL")
 
+    def do_custom_build(self, env):
+        if options.get('system_qhull'):
+            return
+
+        self.download()
+
         for ext in self._extensions_to_update:
             qhull_path = Path(f'build/qhull-{LOCAL_QHULL_VERSION}/src')
             ext.include_dirs.insert(0, str(qhull_path))

A super quick and dirty workaround, not sure I like it, but it seems to work.

Essentially I copied the download/copy license file from do_custom_build and put an additional method which I call before handing off to setuptools at all (but after any config parsing, so the license file is not included if system_qhull is set to True)

One small difference (which may be important) is that the sdist includes the QHULL license file (which it didn't before) and is not actually distributing qhull at all.

Have had no success at pinning down what local version of build deps may have changed this behavior, but I did notice that the 3.6.x branch uses cibuildwheel 0.11.2, whereas the main branch (and 3.7.x) uses 0.12.0, so that could have something to do with it... though nothing jumped at me in looking at the changelog there, so not sure that is relevant (and certainly would expect something lower level to be the actual root cause even if that is the proximal cause)

@jklymak jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 23, 2023
QuLogic added a commit to QuLogic/matplotlib that referenced this issue Mar 2, 2023
We couldn't work out what exactly changed to stop the license being
included, so work around that by pre-downloading it.

Fixes matplotlib#25212
QuLogic added a commit to QuLogic/matplotlib that referenced this issue Mar 2, 2023
We couldn't work out what exactly changed to stop the license being
included, so work around that by pre-downloading it.

Fixes matplotlib#25212
QuLogic added a commit to QuLogic/matplotlib that referenced this issue Mar 2, 2023
We couldn't work out what exactly changed to stop the license being
included, so work around that by pre-downloading it.

Fixes matplotlib#25212
QuLogic added a commit to QuLogic/matplotlib that referenced this issue Mar 2, 2023
We couldn't work out what exactly changed to stop the license being
included, so work around that by pre-downloading it.

Fixes matplotlib#25212
@QuLogic
Copy link
Member

QuLogic commented Mar 3, 2023

Note that this is only fixed for our CI-built wheels. We are still not sure what changed in what package that caused 3.7 wheels to drop the license while 3.6 wheels were previously fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants