Hooks: Fix sysconfig and distutils.sysconfig.
#5218
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bwoodsend
force-pushed
the
fix-sysconfig
branch
2 times, most recently
from
43c74bd
to
dc8e7fc
Compare
rokm
reviewed
Oct 4, 2020
bwoodsend
force-pushed
the
fix-sysconfig
branch
from
dc8e7fc
to
827add6
Compare
htgoebel
suggested changes
Oct 6, 2020
There was a problem hiding this comment.
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.
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. |
bwoodsend
force-pushed
the
fix-sysconfig
branch
2 times, most recently
from
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... |
Legorooj
approved these changes
Mar 7, 2021
|
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. |
rokm
approved these changes
Mar 9, 2021
SomberNight
added a commit
to SomberNight/electrum
that referenced
this pull request
Mar 20, 2022
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.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix or remove the
pyconfig.handmakefileforsysconfigand 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.sysconfigwith 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.