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
Hooks: Fix sysconfig
and distutils.sysconfig
.
#5218
Conversation
43c74bd
to
dc8e7fc
Compare
dc8e7fc
to
827add6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thansk for picking up on this.
When reviewing, I stumbled over this actually being a breaking change: We no longer support including Makefile and config.h, as this is not (as you write in a commit message), a "public API". (In my review comments I assumed this should still include these files.)
Well,.
- Since this is a breaking change, there must be a newsentry in the "breaking" section, describing we are not longer including these file, recommend using get_config_vars(), and explaining something like: "If you actually need Makefile or config.h in your application, you need to add them as datafiles into
...
yourself." - It might be worth adding a section like "Known restrictions" into the manual, containing parts of the "breaking" news entry and explainging how to work around.
- According to the docs, get_config_h_filename() and get_makefile_filename() are still supported and not deprecated. So I'm not quite sure whether we actually should omit these files.
Regarding commit messages:
- For 3. commit: Just say "Remove now unused code" (this was not "redundant", btw :-) In the commit message its not important what unused code is removed here, since full functions are removed.
- For 4. commit: This commit should be moved to the front, otherwise when bisecting, tests will fail at 3rd commit .
- for 4. commit: This is not "updating" the test, but testing a new complete different behaviour. Maybe better spilt this commit into "remove makefile" and "add test for get_config_vars", - while merging the change into the respective commit, implementing the "remove" or "add". Maybe, "add test for get_config_vars" could become a commit of its won at the very beginning, as I would expect this to work already now.
pyi_builder.test_source(""" | ||
{}import sysconfig | ||
from pprint import pprint | ||
pprint(sysconfig.get_config_vars()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I've seen this, I ask myself: This is not testing anything?!
Please add a comment like "rasies if files are not round, see doc-string".
Please also state how/whether this tests for both config.h and Makefile.
Anyway, please rethink this. The old test-script did test qite explicitly on these two files. I could imaging the Python implementation to be changed, and get_config_vars() returning some built-in data-structure.
I promise I will get back to this at some point. Please hold off reviewing for now... |
@bwoodsend Would be great to get this into 4.2, planned for 2020-12-31. |
a104661
to
4eafec6
Compare
Since this is a breaking change and you already removed 3.5 support, I'm scheduling this for PyInstaller 5.0 |
The pyconfig.h and makefile are no longer used directly. Rather configuration is baked into `.py` files.
Rebased and ready. Final round of review... |
Lint isn't happy... once all the CI is happy I am. |
As of Python 3.x, the config header and makefile are not longer used. Trying to include them causes build errors in pyenv/venv/equivalents. Applying this fixes 5018, fixes 4775, and closes 1545 (a cleanup issue). On Windows: The pyconfig.h data is hardcoded directly into sysconfig.py. And the makefile does not exist. On Unix: A Python extension module contains all the details. This module is a hidden import since 3.6 but is already marked as such in the hook. The functions to find and parse the files are still left over in sysconfig.py but they are almost fully replaced by functions to get configuration parameters directly. Should a user attempt to invoke the parser in a PyInstaller build, it will now fail. They should instead use get_config_var() which will always work (and is the recommended usage anyway).
Locating `pyconfig.h` and the `makefile` gets into a mess when using certain environment managers (pyenv-virtualenv) because PyInstaller, whilst trying to find an appropriate `dest` path to put the files in, gets confused by `sys.prefix` (or its varients) not being a parent dir of the config and makefiles. As of Python 3.6, direct parsing of `pyconfig.h` and `makefile` are replaced by a Python module generated on building Python so that these files are no longer required.
Linter appeased. |
see spesmilo#7721 (comment) ----- pyinstaller 4.2 failed (during its runtime) to create exes (worked with cpython 3.9.10, but not with cpython 3.9.11): ``` 80572 INFO: Processing module hooks... 80573 INFO: Loading module hook 'hook-certifi.py' from 'C:\\python3\\lib\\site-packages\\_pyinstaller_hooks_contrib\\hooks\\stdhooks'... 80618 INFO: Loading module hook 'hook-cryptography.py' from 'C:\\python3\\lib\\site-packages\\_pyinstaller_hooks_contrib\\hooks\\stdhooks'... 82879 INFO: Loading module hook 'hook-dns.rdata.py' from 'C:\\python3\\lib\\site-packages\\_pyinstaller_hooks_contrib\\hooks\\stdhooks'... 84147 INFO: Loading module hook 'hook-mnemonic.py' from 'C:\\python3\\lib\\site-packages\\_pyinstaller_hooks_contrib\\hooks\\stdhooks'... 84207 INFO: Loading module hook 'hook-pycparser.py' from 'C:\\python3\\lib\\site-packages\\_pyinstaller_hooks_contrib\\hooks\\stdhooks'... 84212 INFO: Loading module hook 'hook-usb1.py' from 'C:\\python3\\lib\\site-packages\\usb1\\__pyinstaller'... 84215 INFO: --- libusb1 pyinstaller hook --- 84226 INFO: Added libusb binaries: [('C:\\python3\\lib\\site-packages\\usb1\\libusb-1.0.dll', 'usb1')] 84228 INFO: Loading module hook 'hook-difflib.py' from 'C:\\python3\\lib\\site-packages\\PyInstaller\\hooks'... 84237 INFO: Excluding import of doctest from module difflib 84237 INFO: Loading module hook 'hook-distutils.py' from 'C:\\python3\\lib\\site-packages\\PyInstaller\\hooks'... Unable to find "C:\python3\Include\pyconfig.h" when adding binary and data files.This would mean your Python installation doesn't come with proper library files. This usually happens by missing development package, or unsuitable build parameters of Python installation. * On Debian/Ubuntu, you would need to install Python development packages * apt-get install python3-dev * apt-get install python-dev * If you're building Python by yourself, please rebuild your Python with `--enable-shared` (or, `--enable-framework` on Darwin) 🗯 ERROR: build-electrum-git failed ``` Looks like this might be fixed in pyinstaller 4.3 (we are using 4.2): pyinstaller/pyinstaller#5218 ----- While trying to update pyinstaller to have that fix, several issues found with versions 4.3-4.10, now reported upstream and already fixed: pyinstaller/pyinstaller#6338 pyinstaller/pyinstaller#6339 pyinstaller/pyinstaller#6686 So the pyinstaller commit pinned here is from the stable "v4" branch, some commits after the 4.10 tag, which has fixes for the above issues.
Fix or remove the
pyconfig.h
andmakefile
forsysconfig
and its near-duplicatedistutils.sysconfig
. Locating these files or working out where to put them has been causing build errors (#5018 and #4775). In almost all cases they're not needed exceptdistutils.sysconfig
with Python 3.5. So this PR removes them in all but that case and uses an alternative approach to locate the files in the case they are needed which seems to be more tolerant to the many venv-like environment managers (again see #5018).This issue appears to have been Lego under our feet for a while. Fixes #5018, fixes #4775, fixes #4523, and closes #1545 (a can we get rid of this? cleanup issue). It also fixes the second half of the two part issue #4989 (but we still need to finalise the fix for the 1st half). And we can probably close #2367 and close #4405 which I believe are no longer applicable.