-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add an option to put libraries into a library sub directory #5841
base: develop
Are you sure you want to change the base?
Conversation
Woah hoah, that's a bold one!
Well we have CI/CD for that. Github have added this approve and run feature which means that, as you're a first time contributor, every time you want to run CI I have to click yes do it. Normally this is useful but I think it's going to be a pain here. You might find it easier to enable actions on your own fork then temporarily create a pull request to your fork's develop branch. That way, you CI will run on your fork without having to wait for me to tell it that it's ok to run.
Is this specifically
They are treated weirdly but I'd rather that they weren't. Every DLL gets monkey-patched in a way I can't remember (cc @rokm help me out here) and never really understood anyway. |
This is indeed a bold move (especially for the first contribution), but the situation with shared libraries in PyInstaller is rather complicated...
That's going to wreak havoc on macOS, because library paths of all shared libraries AND extensions (including those that remain in the package directories) are rewritten to be relative to the application root. EDIT: or rather, if A (shared library or extension, potentially located anywhere) is linked against B (shared library collected in the application's root), A's link path to B is modified so that it points to B in application's root (_MEIPASS) but as relative path to the A's location. That is indeed really ugly and inconvenient, but I think it was designed this way to deal with shared libraries that get pulled from non-package directories in site-packages (e.g., I suspect it will also break And I wouldn't be surprised if there were other obscure corners of PyInstaller that depend on shared libraries being in the application's root directory... I'm not particularly fond of having this as an option, either. Having two possible deployment layouts with potentially subtly different behaviors will be a nightmare to support. So either we keep collecting stuff into _MEIPASS, or we decide to revamp that, and live with it. Point in the case: the current batch of tests on CI is probably running without the new relocation, so no new issues will be uncovered. All in all, I'd prefer if this started smaller. Perhaps with trying to relocate just the .pyd extensions on Windows. On linux and macOS, we already divert the python extensions coming from python's lib-dynload directory into _MEIPASS/lib-dynload subdirectory. Perhaps something similar could be done for .pyd extensions coming from python's DLLs directory? (Maybe move them into lib-dynload as well?) |
The code strictly follows the steps described in issue #1048. So the changes are straight forward and small. It just did:
That's all about it. As for the The option does not have to take effect on Mac if it is too much a hassle. |
And for the problem with relocating directories like pyinstaller/PyInstaller/loader/pyimod03_importers.py Lines 525 to 539 in e5d6932
That Change the
The |
This would probably be best passed through |
Update sys.path in python loader to simplify the work.
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.
By chance I stepped over this pull-request and left some minor comments.
Please also mind adding documentation for the new command-line option and for sys.pyi_runtime_options.
if (typ in ('EXTENSION', 'BINARY') and self.lib_subdir and | ||
not libpython_pat.match(inm) and not inm_dir): | ||
tofnm = os.path.join(self.name, self.lib_subdir, inm) | ||
todir = os.path.dirname(tofnm) |
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.
Same code as above, line 745
@@ -748,6 +764,7 @@ def assemble(self): | |||
upx_exclude=self.upx_exclude, | |||
dist_nm=inm) | |||
if typ != 'DEPENDENCY': | |||
logger.debug(f"Copy file: {fnm} => {tofnm}") |
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.
Don;t use f-strings for logging, as this will format the string even if the message is never logged. Use
logger.debug("Copy file: %r => %r", fnm, tofnm)
@@ -349,6 +349,8 @@ def __add_options(parser): | |||
"binaries during compression. " | |||
"FILE is the filename of the binary without path. " | |||
"This option can be used multiple times.") | |||
g.add_argument("--lib-subdir", dest="lib_subdir", | |||
help="put library into a sub directory") |
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.
library - singular?
Please be more verbose and ecplaing which libraries.
@@ -445,7 +447,7 @@ def __add_options(parser): | |||
|
|||
|
|||
def main(scripts, name=None, onefile=None, | |||
console=True, debug=None, strip=False, noupx=False, upx_exclude=None, | |||
console=True, debug=None, strip=False, noupx=False, upx_exclude=None, lib_subdir=None, |
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.
Long line.
@@ -567,6 +569,7 @@ def main(scripts, name=None, onefile=None, | |||
'upx': not noupx, | |||
'upx_exclude': upx_exclude, | |||
'runtime_tmpdir': runtime_tmpdir, | |||
'lib_subdir': '"%s"' % lib_subdir if lib_subdir else lib_subdir, |
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.
This is bound to fail as soon as the value contains a quote-character itself.
Why is the values put into quotes here at all? If this is requried for serialization, it should be done at the very place of serialization.
@@ -498,6 +531,9 @@ pyi_pylib_start_python(ARCHIVE_STATUS *status) | |||
SetErrorMode(0); | |||
#endif | |||
|
|||
VS("LOADER: Saving runtime options\n"); | |||
pyi_pylib_save_runtime_opts(status); |
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.
This is a good idea. Anyho, I suggest moving this into a pull-request of its own and — more important — have a changelog entry for it.
value = space + 1; | ||
} else { | ||
key = strdup(ptoc->name); | ||
value = ""; |
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.
AFAIR, an option without value is a boolean option. So the option being present in TOC means the option is set. Thus I suggest setting the value
to True
(or at least to a non-empty string).
path = [] | ||
module_dir = pyi_os_path.os_path_dirname(module.__file__) | ||
path.append(module_dir) |
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.
module_dir = pyi_os_path.os_path_dirname(module.__file__)
path = [module_dir]
sub_path = sys.pyi_runtime_options["pyi-lib-subdir"] | ||
sub_path = pyi_os_path.os_path_join(SYS_PREFIX, sub_path) | ||
sub_path = pyi_os_path.os_path_join(sub_path, module_dir[SYS_PREFIXLEN:]) |
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.
IMHO this is easier to understand:
sub_path = pyi_os_path.os_path_join(SYS_PREFIX,
pyi_os_path.os_path_join(sys.pyi_runtime_options["pyi-lib-subdir"],
module_dir[SYS_PREFIXLEN:])
if "pyi-lib-subdir" in sys.pyi_runtime_options: | ||
sub_path = sys.pyi_runtime_options["pyi-lib-subdir"] | ||
sub_path = pyi_os_path.os_path_join(SYS_PREFIX, sub_path) | ||
sub_lib_dynload = pyi_os_path.os_path_join(sub_path, "lib-dynload") |
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.
Same here
Is there any update on the progress of this? |
Try to do issue #1048, #2468.
Add a option
--lib-subdir libs
option to pyi-makespec.Known Issues:
numpy/
was put into the lib subdir, it cannot be properly imported. Don't know why.DYLD_LIBRARY_PATH
is not touched on MacOS. It seemed there are some special way to treat DLL loading on Mac.So basically, most dynamic modules are put into a lib sub directory. packages are left to the home directory.