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

building: fix the references to nested resources in multipackage #5606

Merged
merged 2 commits into from Mar 16, 2021

Conversation

rokm
Copy link
Member

@rokm rokm commented Mar 5, 2021

Fix MERGE() to properly set references to nested resources, i.e., with their full shared-package-relative path instead of just basename (e.g., path/to/shared/pkg:rel/path/to/file as opposed to just path/to/shared/pkg:file). The latter breaks any resources (data files, extensions/shared libraries, etc) that are located in sub-directories of the shared package.

To demonstrate the issues with nested resources, the multipackage test scripts are extended with:

  • reading from a text file that's collected into sub-directory of _MEIPASS
  • importing numpy psutil to catch issues with nested extensions

The test scripts are also reorganized so that they all use test function from a common package, in an attempt to minimize code duplication.

@rokm rokm force-pushed the multipackage-nested-files branch 4 times, most recently from 69bc633 to 5d2433a Compare March 6, 2021 10:26
@rokm
Copy link
Member Author

rokm commented Mar 8, 2021

I'm not quite happy with code duplication in tests; it was not ideal before, and these changes made it a lot worse. Perhaps this could be worked around to an extent by creating a multipackage_test package, which would contain the actual test function (and could contain the nested data file, too). Then all the scripts would reduce back to importing that package and calling its function (so at least the test function itself would not be duplicated anymore).

Also, importing numpy is a bit heavyweight for this sort of thing. Ddoes anyone know of a lighter package that is guaranteed to contain an extension? I suppose we could try creating our own, but this means it would need to be compiled during test...

So for this changeset, I'm tempted to simply drop the test related commits and have only the fix merged...

@rokm rokm marked this pull request as ready for review March 8, 2021 10:03
@BoboTiG
Copy link
Contributor

BoboTiG commented Mar 8, 2021

Maybe psutil?

@rokm
Copy link
Member Author

rokm commented Mar 8, 2021

Maybe psutil?

Nice, and I think we already install that as part of test requirements. I'll rework those tests into a package form and swap numpy with psutil.

rokm added 2 commits March 8, 2021 21:00
Extend the multipackage test to verify references to nested resources
(data files and extensions) within the shared package in the
multipackage mode. For nested extension, the psutil package is used
(as it contains one).

Refactor the test scripts to use a common test function from a
package to avoid duplicating test code.

The extended tests shows that multipackage currently does not
properly handle resources that are collected in subdirectories of
_MEIPASS instead of _MEIPASS itself.
Fix MERGE() to properly set references to nested resources, i.e.,
with their full shared-package-relative path instead of just basename
(e.g., path/to/shared/pkg:rel/path/to/file as opposed to just
path/to/shared/pkg:file). The latter breaks any resources
(data files, extensions/shared libraries, etc.) that are
located in sub-directories of the shared package.
@rokm rokm force-pushed the multipackage-nested-files branch from 5d2433a to 9956aa0 Compare March 8, 2021 20:05
@rokm
Copy link
Member Author

rokm commented Mar 8, 2021

Here's the revised version, with numpy replaced by psutil and test function being moved into a common package.

@bwoodsend bwoodsend merged commit 227eac1 into pyinstaller:develop Mar 16, 2021
@rokm rokm deleted the multipackage-nested-files branch March 22, 2021 19:38
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants