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

Add pkgs to *._pth files. #142

Merged
merged 2 commits into from Jan 28, 2018
Merged

Add pkgs to *._pth files. #142

merged 2 commits into from Jan 28, 2018

Conversation

@ntoll
Copy link
Contributor

@ntoll ntoll commented Jan 28, 2018

When launching new child processes from an application packaged by pynsist the pkgs directory is NOT in sys.path causing import errors in the child process. The rule about ._pth files applies (from the CPython source here: https://github.com/python/cpython/blob/3.6/PC/getpathp.c#L49):

In the presence of this ._pth file, no other paths are added to the search path, the registry finder is not enabled, site.py is not imported and isolated mode is enabled. The site package can be enabled by including a line reading "import site"; no other imports are recognized. Any invalid entry (other than directories that do not exist) will result in immediate termination of the program.

This PR fixes the problem by ensuring the pkgs directory is referenced in the ._pth files that may be installed in the Python directory.

Copy link
Owner

@takluyver takluyver left a comment

This looks good; I've got just one small request about the comment.

@@ -197,6 +200,15 @@ def fetch_python_embeddable(self):
with zipfile.ZipFile(str(cache_file)) as z:
z.extractall(python_dir)

# Manipulate any *._pth files so the default paths AND pkgs directory
# ends up in sys.path.

This comment has been minimized.

@takluyver

takluyver Jan 28, 2018
Owner

Let's have a link in the comment to the docs describing this file: https://docs.python.org/3/using/windows.html#finding-modules

@@ -197,6 +200,15 @@ def fetch_python_embeddable(self):
with zipfile.ZipFile(str(cache_file)) as z:
z.extractall(python_dir)

# Manipulate any *._pth files so the default paths AND pkgs directory
# ends up in sys.path.
pth_files = [f for f in os.listdir(python_dir)

This comment has been minimized.

@takluyver

takluyver Jan 28, 2018
Owner

Pynsist 2 requires Python >= 3.5, so you could use os.scandir() here if you want. It's not performance critical, though, so feel free to stick with listdir() if you'd rather.

@ntoll
Copy link
Contributor Author

@ntoll ntoll commented Jan 28, 2018

Hi @takluyver, I've added the comment, but left the listdir since it doesn't make much of a difference. Please do let me know if there's anything else I can do.

Looking forward to using it with Mu. :-)

@takluyver takluyver merged commit 2fa615d into takluyver:master Jan 28, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@takluyver
Copy link
Owner

@takluyver takluyver commented Jan 28, 2018

Thanks! Do you want to work on any of the other enhancements you mentioned? Cutting a release takes just a few minutes, but if there are more PRs incoming, I'll wait for them first.

@ntoll
Copy link
Contributor Author

@ntoll ntoll commented Jan 28, 2018

A quick release would be good simply because it fixes Mu's automated builds on appveyor. I'll take a look at other tidy-ups tomorrow (specify an icon, specify a custom image for the welcome screen and display/agree to a licence). These could be wrapped into a single "UI customisation enhancements" PR. Thoughts..?

@takluyver
Copy link
Owner

@takluyver takluyver commented Jan 28, 2018

I just realised that I don't understand how this will interact with #138, which ensured that we read and respect .pth files in the pkgs directory. The Python docs about ._pth say that:

Note that .pth files (without leading underscore) will be processed normally by the site module.

But they also say that "site is not imported unless one line in the file specifies import site", which seems contradictory.

Any ideas? I think I might need to read some code.

@takluyver
Copy link
Owner

@takluyver takluyver commented Jan 28, 2018

Opened https://bugs.python.org/issue32699 to investigate it.

Regarding the customisation changes: I'm a little wary of adding too much customisability, because I don't want to fall into the 'easy wrapper' trap. I see a common pattern where A is a wrapper around B to make common tasks easier (as Pynsist is a wrapper around NSIS). A succession of people find that they like using A, but they wish it exposed just one more option or setting from B. Everyone loves flexibility, so a stream of small changes add each person's 'one more option'. Eventually A is a confusing layer of cruft exposing much of the functionality of B in different ways, and it's grown to such a thick layer that no-one has the time to cut through it and use B directly, so there's even more pressure to add features to A.

Obviously refusing all new options isn't the answer either, but I'm always conscious of that trap when people propose extra customisability. I think I'm probably happy with a way to set the installer icon, but I might suggest that the welcome image and the license text should be done with a custom NSI template for now.

@ntoll
Copy link
Contributor Author

@ntoll ntoll commented Jan 29, 2018

Completely understand, and I think, in the long run, it's a good decision for all the reasons you give.

I have just one request, can we consider the license agreement..? ;-)

Basically, as someone writing code to be used by kids and educational establishments, I want to be protected by the disclaimer / warranty section of the GPL3. If a license=PATH/TO/LICENSE.txt is included in the config file I'd like there to be a license acceptance step. I want to be certain that whoever installs the software, as accepted the license. I hope you agree this is rather an important aspect of an installer, especially given the potential for problems for developers missing such an option may entail. Also, the default behaviour means there's no change in the current behaviour.

Thoughts..?

@takluyver
Copy link
Owner

@takluyver takluyver commented Jan 29, 2018

Fair enough, that sounds like a reasonable addition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants