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

NSI Script Bugfix and Enhancements #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

strayptr
Copy link

@strayptr strayptr commented Jun 4, 2015

Hiya! Currently, pynsist generates an installer which ends up creating a broken start menu shortcut. The start menu shortcut's target is set to "C:\Users%USERNAME%\Desktop\py", which fails to run.

My setup: OS X Yosemite, installed nsis via brew install makensis, installed pynsist via pip install pynsist, created an installer using pynsist examples/console/installer.cfg, ran installer on Win7 x64, observed that Start Menu's Guess the Number shortcut fails to run.

I decided to fix the bug and make some enhancements to the installer script.

Apologies that this isn't a great pull request, but I'm out of time. I figured I may as well submit it in case you were interested in manually incorporating any of the modifications, such as the bugfix.

This pull request makes the following changes:

  • Bugfix: Start menu shortcut targets now use an absolute path, e.g. "C:\Windows\py.exe". (Using an absolute path prevents NSIS from generating an installer which creates a shortcut like "C:\Users%USERNAME%\Desktop\py".)

(Sidenote on bugfix: The correct fix is probably to use CreateShortcut /NoWorkingDir, however the /NoWorkingDir option is not supported on my version of NSIS. If there's a better way of installing NSIS on OS X than brew install makensis, please let me know. Currently the bug will bite anyone on OS X who installed NSIS via brew.)

  • Changed installer behavior: all start menu shortcuts will now be placed within a separate directory, even if only one shortcut is being installed.
  • Added uninstaller shortcut: "Uninstall ${PRODUCT_NAME}.lnk" -> "$INSTDIR\uninstall.exe".
  • Changed uninstaller behavior: The uninstaller now pops up a confirmation dialog to ask whether the user really wishes to uninstall. Previously there was no confirmation, making it a little risky that a user might accidentally uninstall the program.

Anyway, regardless of whether this PR is accepted, thank you very much for this wonderful project!

…h for 'py.exe' and 'pyw.exe' files.

added start menu shortcut: "Uninstall ${PRODUCT_NAME}.lnk" -> "$INSTDIR\uninstall.exe"
changed installer: all start menu shortcuts will now be placed within a separate directory, even if only one shortcut is being installed.
changed uninstaller: added confirmation prompt.
@takluyver
Copy link
Owner

Hmm, that's strange - when I've generated installers from Linux or Windows, it seems to behave correctly, with the shortcut target just being py.exe or pyw.exe. The SearchPath thing looks totally reasonable in principle, but unfortunately I don't think it will work for installing the first Python 2 application, because the py and pyw launchers are installed in a separate step after that, so they won't be present for SearchPath to find them. Maybe that was already the difference that caused the problem, and CreateShortcut was implicitly searching the path anyway.

I don't think uninstallers belong in the start menu; people can use add/remove programs to uninstall it, and pynsist installers set up the correct registry keys so the application shows up there. This largely removes the need for a confirmation on uninstall, because it involves a more deliberate action to trigger the uninstaller in the first place. I try to avoid 'are you sure' dialogs in general.

I also like the behaviour of not creating a directory when there's only one shortcut.

I'm glad you're finding pynsist useful! Can I ask what kind of project you're using it for? It's always interesting to see the different ways people use a tool like this.

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

Successfully merging this pull request may close these issues.

2 participants