takluyver / pynsist

Build Windows installers for Python applications
https://pynsist.readthedocs.io/
Other
896 stars 119 forks source link

Add pkgs to *._pth files. #142

Closed ntoll closed 6 years ago

ntoll commented 6 years ago

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.

ntoll commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Fair enough, that sounds like a reasonable addition.