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(nsis): Ensure application name sub-folder on fresh installs. #7552

Merged
merged 2 commits into from
May 31, 2023

Conversation

p2004a
Copy link
Contributor

@p2004a p2004a commented Apr 20, 2023

During the first program install when the selected directory doesn't exist it's not enforced that path contains application name as sub-folder. On subsequent installs e.g. on update, that condition is enforces though, which creates a new installation in sub-folder.

This tries to address #6885

During the first program install when the selected directory doesn't
exist it's not enforced that path contains application name as
sub-folder. On subsequent installs e.g. on update, that condition is
enforces though, which creates a new installation in sub-folder.

This tries to address electron-userland#6885
@changeset-bot
Copy link

changeset-bot bot commented Apr 20, 2023

🦋 Changeset detected

Latest commit: 92e9c00

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
app-builder-lib Patch
dmg-builder Patch
electron-builder-squirrel-windows Patch
electron-builder Patch
electron-forge-maker-appimage Patch
electron-forge-maker-nsis-web Patch
electron-forge-maker-nsis Patch
electron-forge-maker-snap Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Apr 20, 2023

Deploy Preview for car-park-attendant-cleat-11576 ready!

Name Link
🔨 Latest commit 92e9c00
🔍 Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/6474eedcefb5030008a77c5f
😎 Deploy Preview https://deploy-preview-7552--car-park-attendant-cleat-11576.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@mmaietta
Copy link
Collaborator

@p2004a, thanks for the contribution! Just to confirm, how were you able to test this fix? Do you have a patch-package
(or something similar) that you were applying locally to your builds to test with?

@p2004a
Copy link
Contributor Author

p2004a commented May 2, 2023

@mmaietta Sorry for slow response: I've tested it by applying the change locally in the node_modules of the electron package I'm building.

Here is the behavior tested in Windows Sandbox:

@mmaietta
Copy link
Collaborator

mmaietta commented May 24, 2023

Looks great! But something about this makes me feel like it is a Breaking change? Something doesn't feel right to me about making this change. As it's deliberately reverting 958a7ae#diff-8a6dcd3ec142c996b1c93e06daa9cca068fb00476fbba98df6ce315afffb1a03
Which closed #1301 and #1298

@p2004a
Copy link
Contributor Author

p2004a commented May 24, 2023

It's not reverting that change, but amending/fixing it. 958a7ae made sure that when user installs application in location of their choosing AND that location is non-empty, it's appending the ${APP_FILENAME} to install path.

That resolved #1298 because without that check, user could pick non-empty e.g. C:\games, program would be installed directly there, and when when uninstalling everything in C:\games would be dropped, not only application's files.

My change is dropping the "AND that location is non-empty" from the 958a7ae because guess what? When update happens, the previous install directory is non empty, so installer creates a new subdirectory.

Is it a breaking change? In a way yes, but IMHO not really. Installation files on fresh install no longer will be directly in the user's chosen directory if it didn't exist, but a subdirectory with application name, but IMHO that's better then current behavior when reinstalling and updating just creates a second copy of application inside the previous install, potentially breaking install altogether.

Much cleaner approach would be to somehow figure out whatever the selected directory with files in it is a existing previous install of the application, and if yes, allow installation in it without creating of the sub-directory. I'm personally not willing to learn NSIS scripting language and dive into this installer to implement it this way.

@mmaietta
Copy link
Collaborator

Great explanation. LGTM

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

Successfully merging this pull request may close these issues.

2 participants