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): fix per-machine installs #6450

Merged
merged 4 commits into from Nov 30, 2021
Merged

fix(nsis): fix per-machine installs #6450

merged 4 commits into from Nov 30, 2021

Conversation

robertpatrick
Copy link
Contributor

Add @krisdages changes from PR #6438 to elevate silent per-machine installs.
Remove PR #6073 that incorrectly elevates all per-machine installs, breaking interactive per-machine installs.

Closes #6425, #5468

Add @@krisdages changes from PR #6438 to elevate silent per-machine installs.
Remove PR #6073 that incorrectly elevates all per-machine installs, breaking interactive per-machine installs.

Closes #6425, #5468
@changeset-bot
Copy link

changeset-bot bot commented Nov 28, 2021

🦋 Changeset detected

Latest commit: 862fe85

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

This PR includes changesets to release 9 packages
Name Type
app-builder-lib Patch
electron-updater 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

@robertpatrick
Copy link
Contributor Author

@mmaietta Not sure what to do about these tests since I am unable to get the tests to execute even without these changes.

@robertpatrick
Copy link
Contributor Author

@mmaietta I am far from an NSIS scripting expert but the errors seem to indicate that the variable hasPerMachineInstallation is not always defined when the install script tries to access it.

In looking at the place referenced by the comment in PR #6438 that I copied over, the variable is defined like this:

!ifndef INSTALL_MODE_PER_ALL_USERS
  !ifndef ONE_CLICK
    Var hasPerUserInstallation
    Var hasPerMachineInstallation
  !endif
  Var PerUserInstallationFolder
  ...

but in the changes in the installer.nsi file, it is protected only by:

  !ifndef BUILD_UNINSTALLER
    ${if} $hasPerMachineInstallation == "1" # set in onInit by initMultiUser
    ...

I don't feel like I understand the exact implications of these various ifndef variables but I am experimenting with changes that will both allow those 15 failing tests (which thankfully, I am able to run one-by-one from IntelliJ even though the full pnpm test is still failing with dozens of errors even when run directly on master) and my installer to work properly. I will update the PR if/when I figure out the magic combination that fixes the failing unit tests and allows my installer to work properly...

Add @krisdages changes from PR #6438 to elevate silent per-machine
installs and add ifdefs to limit scope so that unit tests run correctly.
Remove PR #6073 that incorrectly elevates all per-machine installs,
breaking interactive per-machine installs.

Closes #6425, #5468
@robertpatrick
Copy link
Contributor Author

@mmaietta OK, this looks like a winner. All unit tests seem to be passing and the 4 installer scenarios are working with my application:

  1. per-machine, quitAndInstall()
  2. per-machine, autoInstallOnAppQuit
  3. per-user, quitAndInstall()
  4. per-user, autoInstallOnAppQuit

@mmaietta
Copy link
Collaborator

This is awesome.

Thank you @robertpatrick and @krisdages for your great work here!

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.

electron-updater@next breaks NSIS "per machine" quitAndInstall() when not using silent mode
3 participants