[v2] NSIS installer support for Windows#1184
Conversation
leaanthony
left a comment
There was a problem hiding this comment.
Thanks for this - it's awesome stuff! Just a couple of comments to discuss and I think we're good to go.
|
|
||
| SetRegView 64 | ||
| # If this key exists and is not empty then webview2 is already installed | ||
| ReadRegStr $0 HKLM "SOFTWARE\WOW6432Node\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}" "pv" |
There was a problem hiding this comment.
There are 2 keys to check for installation depending on whether it was installed by an admin or not:
HKEY_LOCAL_MACHINE\SOFTWARE\WOW6432Node\Microsoft\EdgeUpdate\Clients{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}
HKEY_CURRENT_USER\Software\Microsoft\EdgeUpdate\Clients{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}
There was a problem hiding this comment.
As the installer per default is run elevated it makes sense to only check against the admin key. If we wouldn't do that, an installer which is run elevated and installs the app for all users, the app would only work for that specific user and not for all user. Or does an installation by a non admin user work for other users on the same machine?
This check is also just a fastpath, if it doesn't work the bootstrapper will detect the runtime is installed and won't do anything. So it would be just a waste of time and probably bandwidth during the installation.
I think the best would be to do the following:
- If the installer is run elevated just check against the admin key.
- If the installer is not run elevated, first check against the admin key and then the current user key.
There was a problem hiding this comment.
The reason I bring it up is that running it elevated was a temporary work around as MS did a breaking change (they created a user install key but didn't publish it). Installing the runtime with user privileges is the best scenario as it'll work in all environments (eg corporate). I reverted the "admin install" change today as we don't need it any more (we detect installation by asking webview2loader.dll what version is installed. I agree with your summary at the end.
There was a problem hiding this comment.
Yeah, I agree with that for the embedded install case. For the installer it's slightly different, and for corporate environments the installation must almost always be performed by an admin.
So I'll go ahead and change the NSIS tools accordingly.
There was a problem hiding this comment.
I didn't find a reliable way to determine the execution level of the installer that satisfied me. Therefore I've introduced a new define !define REQUEST_EXECUTION_LEVEL "admin" that user can use to change the execution level. This define is then taken into account in the wails_tools.nsh to also check the user specific key if needed.
| Info: projectData.Info, | ||
| } | ||
|
|
||
| data.Internal.Webview2RuntimeSetupPath = filepath.Join(fs.RelativePath("../../"), "internal", "webview2runtime", "MicrosoftEdgeWebview2Setup.exe") |
There was a problem hiding this comment.
We're moving away from assuming the Wails CLI is installed as a Go module so we don't want to reference files relative to the source. I think we should copy this setup file into a temp directory. I've added a method to the webview2runtime package to do this. Perhaps we could use os.TempDir()?
There was a problem hiding this comment.
Oh yes, didn't have that in mind. Thanks for the WriteInstaller method.
I would like to keep the property that the installer could be created outside of wails for debugging and test purposes after a first wails build --nsis. So I'll probably just write it into build/windows/installer/tools/ instead of having it in a TempDir and leave the file leaking space there.
What do you think?
There was a problem hiding this comment.
Yeah, I thought that initially but then it'd be easy to check in with your project. The reason I suggested temp dir was because I assumed it would be deleted anyway. If you really want it in the build dir, lets add a .gitignore to the default tempalates.
There was a problem hiding this comment.
Well we have to define if we wanna keep the property of being able to run makensis outside of wails for debugging purposes or not. If we want to get rid of that property I'm fine doing it completely in a temp dir and deleting it afterwards.
I also agree with you to add a .gitignore, I also wanted to do that for the "wails_tools.nsh" which also get's auto generated and written back into build/windows/installer/ to support this property.
There was a problem hiding this comment.
What do you think about the property of being able to run makensis outside of wails for debugging purposes?
There was a problem hiding this comment.
Given everything you've said, I'd suggest dumping it in the build dir and add a .gitignore is fine 👍
There was a problem hiding this comment.
Great, I'll update the PR on monday
There was a problem hiding this comment.
.gitignore entries have been added.
Currently only supports build-level hooks
… generation The manifest asset files are now a go template and data will be resolved before they are included into the build output. Breaking Change: Windows manifest file must be named “wails.exe.manifest” and doesn’t depend on the project name anymore.
8bfba89 to
c7b965a
Compare
c7b965a to
a236563
Compare
|
Thanks for all the hard work on this 🙏 |
* [v2] Add support for post build hooks Currently only supports build-level hooks * [v2] Improve build assets handling and use single source for manifest generation The manifest asset files are now a go template and data will be resolved before they are included into the build output. Breaking Change: Windows manifest file must be named “wails.exe.manifest” and doesn’t depend on the project name anymore. * [v2, windows] NSIS installer generation
|
Awesome, thanks so much for merging. |
* [v2] Add support for post build hooks Currently only supports build-level hooks * [v2] Improve build assets handling and use single source for manifest generation The manifest asset files are now a go template and data will be resolved before they are included into the build output. Breaking Change: Windows manifest file must be named “wails.exe.manifest” and doesn’t depend on the project name anymore. * [v2, windows] NSIS installer generation
wails.jsonby leveraging go templating.Closes #1124
Closes #1135
Closes #1155