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.

Sign up for GitHub

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

Jump to bottom

Declutter onedir bundles #7713

Merged
merged 4 commits into from Jul 13, 2023
Merged

Declutter onedir bundles #7713

merged 4 commits into from Jul 13, 2023

Conversation

bwoodsend
Copy link
Member

@bwoodsend bwoodsend commented Jun 17, 2023 •
edited

When building in onedir mode, move everything except the executable into a subdirectory so the executable is easily findable.

I'm deliberately not making this optional for now because the new if/else path arithmetic bootloader logic it would require will multiply with all the other if/else path arithmetic bootloader logic for macOS .app bundles and the too many variants of MERGE() leading to a testing/debugging hell hole which far exceeds my motivation to implement.

This is all rebased on top of #7619 to avoid the merge conflicts it would otherwise lead to.

@bwoodsend bwoodsend force-pushed the new-onedir branch 2 times, most recently from fc30bde to 7b114e2 Compare June 17, 2023 23:48
@bwoodsend bwoodsend marked this pull request as ready for review June 17, 2023 23:49
@bwoodsend bwoodsend marked this pull request as draft June 17, 2023 23:50
@bwoodsend
Copy link
Member Author

I don't understand why CI is not running on this.

@rokm
Copy link
Member

rokm commented Jun 18, 2023

I think it's due to merge conflict. I'll rebase the symlinks branch, and then you can rebase this on top of it...

@bwoodsend
Copy link
Member Author

Ahh yes, https://github.com/orgs/community/discussions/26304 says as much.

@bwoodsend
Copy link
Member Author

I suppose there's no hurry for this PR given that it has to wait until after #7619 which in turn has to wait until after the next release.

@bwoodsend
Copy link
Member Author

Well that's CI gummed up for the rest of the day...

@bwoodsend bwoodsend mentioned this pull request Jun 18, 2023
Merged
@bwoodsend bwoodsend force-pushed the new-onedir branch 2 times, most recently from da12802 to ec09c8e Compare June 20, 2023 19:46
@rokm
Copy link
Member

rokm commented Jun 20, 2023

The numpy test is failing because we are calling SetDllDirectory with original homepath instead of the modified one (archive_status->homepath), as seen in the debug output.

This means that we should probably switch from homepath to archive->homepath at least here and possibly also here. And any subsequent homepath uses should probably also be re-evaluated and changed as necessary.

@bwoodsend
Copy link
Member Author

Reckon we need some form of new --add-data-like option to place files outside of the _internal directory? It would be nothing more than a convenience feature given that anyone can run cp README dist/application or append shutil.copy() to the spec file.

@rokm
Copy link
Member

rokm commented Jun 22, 2023

Reckon we need some form of new --add-data-like option to place files outside of the _internal directory? It would be nothing more than a convenience feature given that anyone can run cp README dist/application or append shutil.copy() to the spec file.

I think it would just make things more confusing, since it wouldn't map to onefile builds nor to onedir macOS app bundles...

@bwoodsend
tests: ctypes: Fix dylib compilation on macOS M1.
aef9cb9
@bwoodsend
onedir: Move internal files into an _internal sub-directory.
d50e778 
When building with BUNDLE(), all files par the executable and (for
side-loaded pkgs) and the .pkg are moved into a directory with
non-configurable name `_internal`. When running the application, set
MEIPASS to the `_internal` directory rather than the directory
containing the executable. Since, MEIPASS is used to derive
library/resource search paths, those adjust automatically and in theory,
only people using `dirname(sys.executable)` should have new issues
finding resources at runtime after this change.
@bwoodsend bwoodsend marked this pull request as ready for review July 10, 2023 22:02
@bwoodsend
Copy link
Member Author

I'm really not sure about the names for the new options.

@rokm
Copy link
Member

rokm commented Jul 10, 2023

I'm really not sure about the names for the new options.

Hmmm, yeah. It's currently called internals_prefix in user-facing options and meipass_prefix in the lower-level parts, and neither is particularly helpful in terms of descriptiveness. I suppose I would push for some (shorter?) variant on contents_directory_name (maybe just contents_directory?).

This was linked to issues Jul 10, 2023
Put all data into a sub-directory? #2702
Closed
Improvement in OneDir structure #5575
Closed
Please add a clean folder mode #7687
Closed
@bwoodsend
Copy link
Member Author

bwoodsend commented Jul 10, 2023 •
edited

I suppose contents_directory is fairly non-ambiguous. Not sure that it would spring to mind whenever I'm trying to remember its name.

rokm
rokm reviewed Jul 11, 2023
View reviewed changes
bootloader/src/pyi_archive.c Outdated Show resolved Hide resolved
rokm
rokm reviewed Jul 11, 2023
View reviewed changes
bootloader/src/pyi_launch.c Outdated Show resolved Hide resolved
@rokm
Copy link
Member

rokm commented Jul 11, 2023

One thing I'm not particularly happy about is that pyi-meipass-prefix now always needs to be set (which kind of makes it not-an-option...). But the alternative would be having the default hard-coded in bootloader (and thus again specified in two places), and I cannot decide which of the two options seems worse...

@bwoodsend
Copy link
Member Author

One thing I'm not particularly happy about is that pyi-meipass-prefix now always needs to be set (which kind of makes it not-an-option...).

Do you do run things with the raw bootloaders in PyInstaller/bootloader? Even when debugging, I can't see why this would matter otherwise.

@rokm
Copy link
Member

rokm commented Jul 11, 2023

One thing I'm not particularly happy about is that pyi-meipass-prefix now always needs to be set (which kind of makes it not-an-option...).

Do you do run things with the raw bootloaders in PyInstaller/bootloader? Even when debugging, I can't see why this would matter otherwise.

Yeah, it's more of a philosophical/conceptual concern. Definitely not a blocker, though.

@bwoodsend
Make the name of onedir mode's _internal directory configurable.
f80d0d5 
Add a --contents-directory CLI option and corresponding
contents_directory spec file option to change the otherwise hard-coded
name of the directory in which all a onedir's non EXE contents are
hidden.

Note that this, whilst this spec file option would make the most sense
as a parameter to BUNDLE(), it instead has to be an option to EXE()
which BUNDLE() then fishes out of the EXE()'s configuration due to EXE()
being the only place where OPTION TOC entries can go.
@bwoodsend
bootloader: Replace references to original homepath.
7b8916f
@bwoodsend bwoodsend merged commit 3a3aa2f into pyinstaller:develop Jul 13, 2023
18 checks passed
@bwoodsend bwoodsend deleted the new-onedir branch July 13, 2023 18:55
@HaujetZhao HaujetZhao mentioned this pull request Jul 16, 2023
Open
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Legorooj Legorooj Legorooj left review comments

@rokm rokm rokm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Please add a clean folder mode Improvement in OneDir structure Put all data into a sub-directory?
3 participants
@bwoodsend @rokm @Legorooj