takluyver / pynsist

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

The wrong directories are added in the script template of commands #199

Closed ThiemoVanEngelen closed 4 years ago

ThiemoVanEngelen commented 4 years ago

The script template for commands adds installdir/pkgs to the sys.path and as site dir. The commands are however installed in bin/, so the installdir points to bin/. This means that the script template adds bin/pkgs/ instead of pkgs/.

This was found while creating the installer of git-cola.

takluyver commented 4 years ago

Can you provide more detail? This is the line that finds the installdir from command scripts:

https://github.com/takluyver/pynsist/blob/fa69cd31b15d59544bd9f1d280a637916028b749/nsist/commands.py#L11

If a command is in blah/bin/script.exe, calling dirname twice should get us back to blah/.

(BTW, I think the preamble for git-cola can be simplified a bit following PR #149.)

takluyver commented 4 years ago

Oh, maybe __file__ gets set to something different now? If so, perhaps it makes sense to use sys.executable as the starting point for finding the install dir instead.

ThiemoVanEngelen commented 4 years ago

I printed some stuff in the git-cola preamble:

__file__: C:\Users\tvanengelen\AppData\Local\git-cola-3.7.1\bin\git-cola.exe\__main__.py
os.path.dirname(__file__): C:\Users\tvanengelen\AppData\Local\git-cola-3.7.1\bin\git-cola.exe
installdir: C:\Users\tvanengelen\AppData\Local\git-cola-3.7.1\bin
sys.executable: C:\Users\tvanengelen\AppData\Local\git-cola-3.7.1\Python\python.exe

So __file__ indeed does not point to the .exe

takluyver commented 4 years ago

So commands have probably been broken since we switched to the distlib launchers which put the script in an appended zipfile, rather than next to the exe. :-(

Are you interested in making a PR for this?

ThiemoVanEngelen commented 4 years ago

I can try, but you are welcome to fix it yourself.

takluyver commented 4 years ago

I started looking at this. There's a test which builds an installer, runs it and then checks the resulting application. It should already catch this problem. Specifically, this line should fail:

https://github.com/takluyver/pynsist/blob/fa69cd31b15d59544bd9f1d280a637916028b749/nsist/tests/test_main.py#L91-L92

But it's passing on CI. I thought maybe it was to do with a newer version of distlib, so I prodded it to run again, and it still passes. Then I thought maybe it depended on the Python version, so I tweaked the test to use 3.8.3, but it still passes.

We could just change it anyway, but I'd like to understand what's going on - is the test broken somehow and not pointing out problems it should detect?

takluyver commented 4 years ago

Ah, I see it now - there are two separate things ensuring that the pkgs/ dir goes on sys.path, and this code modifying a ._pth file works. But I guess that doesn't make it a site directory, so we still need the extra handling in the wrapper script to do that and ensure .pth files there are processed.

takluyver commented 4 years ago

I've released Pynsist 2.5.1 with the fix for this.