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

[robertpatrick]

• Electron-Builder Version: 22.14.7

• Node Version: 16.13.0

• Electron Version: 15.3.1
• Electron Type (current, beta, nightly): current

• Electron-Updater Version: 4.6.2

• Target: Windows NSIS

When we move from electron-updater latest to next to pick up the autoInstallOnAppQuit fix for MacOS, it breaks the previously working auto-updater for NSIS when installing for "All Users" (works fine if installed using the "Only for Me" option).

What I am seeing is a little strange in that the very first time I run the installer (unsigned and built locally), the quitAndInstall() call works as expected. However, if I uninstall the application and then install again, the quitAndInstall() call gets to the point of causing Windows to pop up the UAC dialog asking for permission but then nothing else happens (that is, the Installer dialog does not appear). If I run the app again and call quitOnInstall(), it works as expected. After that first time, this behavior is repeatable every single time.

In looking at the log file (produced by our logger that we pass onto the autoUpdater), the log entries are always the same:

info: Checking for update
info: Found version 1.0.0 (url: WebLogic-Kubernetes-Toolkit-UI-Setup-1.0.0.exe)
info: Downloading update from WebLogic-Kubernetes-Toolkit-UI-Setup-1.0.0.exe
debug: updater cache dir: C:\Users\rhpat\AppData\Local\wktui-updater
info: Update has already been downloaded to C:\Users\rhpat\AppData\Local\wktui-updater\pending\WebLogic-Kubernetes-Toolkit-UI-Setup-1.0.0.exe).
info: Download complete, install type: now
info: Install on explicit quitAndInstall
info: Install: isSilent: false, isForceRunAfter: true, installPathRequiresElevation: true
info: isAdminRightsRequired is set to true, run installer using elevate.exe

If I don't change anything other than to drop back to electron-updater@latest, everything works as expected the first time, every time. If it will help, I can pass you an installer built with electron-updater@next that reproduces the bad behavior. Please let me know what I can do to help move this forward.

[robertpatrick]

@mmaietta OK, so digging in and adding some extra logging, I figured out why electron-updater@4.3.9 is working and electron-updater@4.6.2 is not when the old version of the app is installed for all users.

In 4.3.9, NsisUpdater.doInstall() is called like this (notice isAdminRightsRequired is false) and quitAndInstall() works as expected:

info: Entering doInstall() with options = {"installerPath":"C:\\Users\\rhpat\\AppData\\Local\\wktui-updater\\pending\\WebLogic-Kubernetes-Toolkit-UI-Setup-1.0.0.exe","isSilent":false,"isForceRunAfter":true,"isAdminRightsRequired":false}

In 4.6.2, NsisUpdater.doInstall() is called like this and quitAndInstall() fails the first time and succeeds when I restart the app and call it the second time (more digging required on my part to try to figure out the problem):

info: Entering doInstall() with options = {"installerPath":"C:\\Users\\rhpat\\AppData\\Local\\wktui-updater\\pending\\WebLogic-Kubernetes-Toolkit-UI-Setup-1.0.0.exe","isSilent":false,"isForceRunAfter":true,"isAdminRightsRequired":true}

Is there a reason the behavior of quitAndInstall() changed (e.g., to try to fix the autoInstallOnAppQuit option when installed for all users)? Whatever the reason, that change seems to have broken explicit calls to quitAndInstall().

[robertpatrick]

@mmaietta @hvuntokrul This commit breaks the Windows install for All Users (at least when not using the one-click installer and not using silent mode). By modifying the NsisUpdater.doInstall() to ignore the isAdminRightsRequired option when this.quitAndInstallCalled is set to true as shown here, this resolves my issue.

        if (this.quitAndInstallCalled) {
            this._logger.info('quitAndInstallCalled is true so ignoring isAdminRightsRequired and skipping callUsingElevate()');
        } else if (options.isAdminRightsRequired) {
            this._logger.info("isAdminRightsRequired is set to true, run installer using elevate.exe");
            callUsingElevation();
            return true;
        }

Since I am not sure what problem the commit was addressing, I don't feel like I have enough information to create the necessary PR that both resolves my issue and doesn't break whatever issue the original commit was fixing. How can I help in resolving this issue so that I am not stuck with electron-updater@4.3.9 forever?

[robertpatrick]

As it turns out, it looks like a better test is to only use elevate if the installer is running in silent mode (found this by testing autoInstallOnAppQuit use case since it always runs the installer in silent mode and requires elevate to work properly).

        if (!options.isSilent) {
            this._logger.info('isSilent is false so ignoring isAdminRightsRequired and skipping callUsingElevate()');
        } else if (options.isAdminRightsRequired) {
            this._logger.info("isAdminRightsRequired is set to true, run installer using elevate.exe");
            callUsingElevation();
            this._logger.info('Exiting doInstall()');
            return true;
        }

[robertpatrick]

@mmaietta Please see my comment on PR #6438. I suspect that #6438 is the right solution but will only work if PR #6073 is rolled back. Am I missing something?

[mmaietta]

@robertpatrick thanks for the detailed investigation and for reviewing the PR! I honestly do not have expertise with nsis scripting, but using nsis install script for determining elevation certainly seems like the proper route instead of a js write-file attempt.

If we're to move forward with #6438, which I'm in favor of, then I think the commit from #6073 should be reverted as part of the new PR. Thoughts?

I currently don't have a manner to test the PR though as my Windows machine decided to go out-of-office on me

[robertpatrick]

@mmaietta Makes sense to me. I will make these changes and test them for you tomorrow and report back.

[robertpatrick]

@mmaietta OK, I applied PR #6438 to, and removed PR #6073 from, the next versions of both electron-builder and electron-updater. I tested the following scenarios using a fork of my actual application.

1. Per Machine quitAndInstall()
2. Per Machine autoInstallOnAppQuit
3. Per User quitAndInstall()
4. Per User autoInstallOnAppQuit

All four scenarios are working properly with these two changes. Let me know when I can get a release with these two changes!

[mmaietta]

That's excellent news!

Would you be willing to open a PR with your changes? Just would need to also mention credit to @krisdages for the work in PR #6438

[robertpatrick]

@mmaietta Yes, I am willing and working on it.

In following your CONTRIBUTING.md guide, it seems that pnpm test fails on the master branch--even before making any changes--on both Windows and MacOS. I suspect there may be some missing instructions/configuration in the guide for running the tests locally. See attached output on MacOS.

Given that I am unable to run the tests locally, I am forced to submit the PR without running the test suites.

electron-builder-tests.txt

[mmaietta]

The tests are also OS-specific, so many of them will fail locally if they're designed to be run on mac.

It's been one of the confusing aspects of this project, but I don't know of another way to have snapshot tests without it being OS-specific. Linux tests can run on mac, but they won't have the same snapshot data.

Thanks for the PR. Took a look, it looks solid.

Waiting on all CI tests to run now.

[robertpatrick]

@mmaietta i get that but the test framework already seems to have a mechanism (isMac, etc) to deal with that. The tests I saw failing were ones either looking for something in @develar ‘s GitHub space using the GitHub API that needed an API token or looking for something in S3 (that likely also needs a token). It would be great if we could either provide instructions for what tokens are required and how to provide them for the tests when running locally or mark those tests as CI-only tests so they don’t interfere with contributors trying to get feedback on their changes before submitting PRs. I would be willing to help if you all think it is something that should be done and are willing to help work toward that goal.

[mmaietta]

Newer tests are leveraging ifEnv
https://github.com/electron-userland/electron-builder/blob/v22.14.10/test/jestSetup.js#L18-L20
Used like:

electron-builder/test/src/ArtifactPublisherTest.ts

Lines 86 to 91 in a945321

	test.ifEnv(process.env.AWS_ACCESS_KEY_ID != null && process.env.AWS_SECRET_ACCESS_KEY != null)("S3 upload", async () => {
	const publisher = createPublisher(publishContext, "0.0.1", { provider: "s3", bucket: "electron-builder-test" } as S3Options, {}, {} as any)!
	await publisher.upload({ file: iconPath, arch: Arch.x64 })
	// test overwrite
	await publisher.upload({ file: iconPath, arch: Arch.x64 })
	})

That should be enough for marking those tests effectively as CI-only, unless otherwise explicitly working on one of those env-specific tests locally, no?

[robertpatrick]

@mmaietta That sounds right but all that I can say is that pnpm test generates a dozen or more failures when creating an environment as specified in CONTRIBUTING.md. It seems like making pnpm test work locally would help people like me that want to contribute and not submit PRs that cause unit tests to fail. Clearly, some tests won't run locally but at least we should get pnpm test to run clean locally for the tests that can run.

[adamcjm]

I find a way to solve this problem:

1. You can try to add a line code with the content CRCCheck off to bottom of the file node_modules/app-builder-lib/templates/nsis/common.nsh.

2. And then rebuild and install the application to windows, and try to uninstall or update.

these two step fixed my problem.