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

fix: restore environment before launching external programs #707

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mashed5894
Copy link

Issue

By default, any subprocesses will inherit environment variables from its parent. Pyinstaller will specify a custom LD_LIBRARY_PATH, which causes issues when the subprocess attempts to load its own dynamically linked libraries.
https://web.archive.org/web/20240824170055/https://github.com/TagStudioDev/TagStudio/issues/210

Solution

  1. Implemented suggested solutions for linux and windows from https://pyinstaller.org/en/stable/common-issues-and-pitfalls.html#launching-external-programs-from-the-frozen-application. Doesn't really seem like a solution for MacOS is necessary based on the docs.

  2. Renamed and slightly reworked promptless_Popen into silent_Popen (better name, and congruent with the actual file name) so future features can take advantage of the same solution.

Tested on linux and briefly on windows (but could not figure out a way of getting hold of an outdated dll). At least it shouldn't be worse.

@mashed5894 mashed5894 force-pushed the fix-popen-environment branch from e7b8790 to 5965872 Compare January 16, 2025 12:58
@mashed5894 mashed5894 force-pushed the fix-popen-environment branch from 5965872 to bff3b07 Compare January 16, 2025 15:19
@CyanVoxel CyanVoxel added Type: Installation Installing, building, and/or launching the program Status: Review Needed A review of this is needed System: Windows For Microsoft Windows System: Linux For Linux/BSD distributions labels Jan 16, 2025
Copy link
Collaborator

@Computerdores Computerdores left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't really have a way to test this, but it seems to follow the documentation well and I see no other issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review Needed A review of this is needed System: Linux For Linux/BSD distributions System: Windows For Microsoft Windows Type: Installation Installing, building, and/or launching the program
Projects
Status: 🏓 Ready for Review
Development

Successfully merging this pull request may close these issues.

3 participants