"x86 + x64" installer #44

chipitsine · 2016-11-15T20:26:56Z

No description provided.

mattock · 2016-11-16T09:14:04Z

There were some spaces intermixed with tabs which I fixed. You can apply this patch on top of this PR to fix them. I'll test this next.

mattock · 2016-11-16T09:38:45Z

windows-nsis/build-complete

@@ -42,8 +45,7 @@ main() {
 			--sign-pkcs12="${SIGN_PKCS12}" \
 			--sign-pkcs12-pass="${SIGN_PKCS12_PASS}" \
 			--sign-timestamp="${SIGN_TIMESTAMP_URL}" \
-			|| die "pack ${arch}"
-	done
+			|| "pack installer"


Shouldn't this say || die "pack installer"?

mattock · 2016-11-16T09:52:16Z

This PR basically worked, except for one rather major thing: on 64-bit Windows it puts all the files to C:\Program Files (x86)\OpenVPN, not to C:\Program files\OpenVPN. This is problematic for two reasons:

It changes behavior compared to old installers (causes confusion, breaks scripts,etc.)
It is confusing, because 64-bit executables are installed under 32-bit system directory

I took a quick look at openvpn.nsi, and it seems that you may need to adapt the .onInit function to fix this. Simply changing InstallDir based on bitness probably will not work, as the registry view also needs to be change, and that is done in .onInit.

Once this issue is fixed, I will rebuild installers and send a notification to openvpn-devel and openvpn-users maling lists, so that more people can test it. Hopefully we can get this functionality merged with 2.4_rc1 installers, as it is genuinely useful.

mattock · 2016-11-16T13:35:07Z

windows-nsis/build-complete

@@ -45,7 +45,7 @@ main() {
 			--sign-pkcs12="${SIGN_PKCS12}" \
 			--sign-pkcs12-pass="${SIGN_PKCS12_PASS}" \
 			--sign-timestamp="${SIGN_TIMESTAMP_URL}" \
-			|| "pack installer"
+			|| "die pack installer"


Actually, die is the function name and "pack installer" a parameter, so it should say die "pack installer" (notice location of quotes).

chipitsine · 2016-11-16T17:46:10Z

I tested installer on x64. it is ok.

I'll rebase as a single commit

mattock · 2016-11-17T09:30:21Z

I'm testing the installer right now.

There is one issue, though... While removing the InstallDir line seems seems harmless, it actually breaks silent installation to non-standard directories. That is a bit esoteric, but I know about it because I fixed that problem myself in commit a73a75. More details are available in Trac#249.

chipitsine · 2016-11-17T09:44:40Z

ok, I'll test silent installation

mattock · 2016-11-17T10:58:01Z

I tested installer based on this PR on Windows 7 Pro 64-bit, and @cron2 tested it on Windows 7 32-bit. In both cases the installer seemed to work as expected. We only tested manual installations.

mattock · 2016-11-17T11:04:26Z

Oh, and this won't make it to OpenVPN 2.4_beta1 which is due in a few hours. However, RC1 is not far away, so the delay will be only a few weeks. This will also give independent tests more time to verify the functionality on various Windows versions.

chipitsine · 2016-11-17T18:49:44Z

I tested, it works with custom install directory now.
I think we can test that behaviour with openvpn-powershell (probably, I need to create an issue, in order to keep it somewhere)

as for the rest, I'm aware of couple of things:

installer tells "Note that the Windows version of ${PACKAGE_NAME} will only run on Windows XP, or higher", which is not true. So, we should add some check for WinXP and change that to Vista.
.net 4 if openvpnserv2 is selected

mattock · 2016-11-18T10:30:09Z

windows-nsis/openvpn.nsi

+	WriteRegDWORD HKLM "SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\${PACKAGE_NAME}" "NoRepair" 1
+	WriteRegStr HKLM "SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\${PACKAGE_NAME}" "Publisher" "${PRODUCT_PUBLISHER}"
+	WriteRegStr HKLM "SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\${PACKAGE_NAME}" "UninstallString" "$INSTDIR\Uninstall.exe"
+	WriteRegStr HKLM "SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\${PACKAGE_NAME}" "URLInfoAbout" "https://openvpn.net"



What do the "Language", "NoModify" and "NoRepair" lines do exactly? Do they affect uninstall behavior?

usually, in control panel you see "Uninstall/Change" button. As for NSIS installers, there's no "Change" or "Repair" possibility, so, using those values in registry will reduce button text to "Uninstall"

it is cosmetic, just as language.

Ah, ok, that makes sense then.

mattock · 2016-11-18T10:31:21Z

Let's put the "fail on WinXP" and .Net 4 check to a separate PR. That way we can get the combined installer tested and merged more easily.

mattock · 2016-11-18T10:33:21Z

The Travis failure is again due to build-snapshot. I committed a fix three hours ago, so it should go away on next build

mattock · 2016-11-18T10:40:05Z

A build of OpenVPN 2.4_beta1 is ongoing. Once it is finished (in <20 minutes), a combined installer should be available here. We should advertise that installer on openvpn-devel list once it passes the basic tests. The things to test, in my opinion, are:

Ensure that files are places to correct directory, that is, C:\Program Files\OpenVPN
Test silent install with default settings
Test silent install to a non-standard directory
Ensure that registry keys go to the correct (32-bit or 64-bit) registry
Test that uninstall works as expected

The installer also has openvpnserv2 v1.2.0.0, which includes the exit-event fix v2. So all the more reason to have this all tested by real users and the Powershell test suite.

Anyways, let's not add any more "stuff" to this PR at this point, so that we don't have to do any more testing (assuming the installer + openvpnserv2 work correctly).

chipitsine · 2016-11-18T12:05:15Z

yes, I saw travis failing. I didn't address that, because it will be automatically resolved after merge.

chipitsine · 2016-11-18T12:05:52Z

do you want to make more wide announce of https://build.openvpn.net/downloads/snapshots/openvpn-install-2.4_beta1-I901.exe ? I tested for myself, it is ok

chipitsine · 2016-11-18T12:06:53Z

as for uninstaller, it leaves openvpnserv2 log. I guess it is bug, but we could leave for now.

I'm going to make test like "install installer, uninstall and check for files left"

mattock · 2016-11-18T13:10:21Z

I ran openvpn-windows-test (in my case about 12 different connections with mix of IPv4 and IPv6 payloads - no issues with the installer. I will announce the installer on openvpn-devel mailing list. If nobody has found any issues by Monday I think we can merge this PR.

selvanair · 2016-11-18T14:58:55Z

Tested the snapshot installer (on windows 7: all good except

If openvpn.exe is running (in my case there were two instances started by openvpnserv2), the installer fails with message saying stop those processes and re-run the installer. Can we offer to stop the service (this will also close those processes) if found running (as done with the GUI)?
In control panel, the publisher now correctly shows up, but the installed size is empty (not that it matters)
(Publisher and size for Tap Windows still shows empty, but that has to come from tap-windows's installer).
Is .NET runtime is automatically installed if not present? My test system has it installed, so not sure.

chipitsine · 2016-11-18T17:48:36Z

Ok, I will test in every combination of running openvpn.exe, openvpn-gui.exe and service. I did not test that
Ok, will add size as well
It will be addressed in separate PR

chipitsine · 2016-11-19T18:23:12Z

@selvanair , as for stopping services, I found http://nsis.sourceforge.net/NSIS_Simple_Service_Plugin
it looks good, except it is dated 2011. it may take some more time to test it on Win10, for example

should we merge "x86 + x64" PR and address other issues (like .net 4, Windows Vista instead of WinXP, stopping services) in later PRs ?

selvanair · 2016-11-19T18:57:53Z

On Sat, Nov 19, 2016 at 1:23 PM, Ilya Shipitsin notifications@github.com
wrote:

@selvanair https://github.com/selvanair , as for stopping services, I
found http://nsis.sourceforge.net/NSIS_Simple_Service_Plugin
it looks good, except it is dated 2011. it may take some more time

should we merge "x86 + x64" PR and address other issues (like .net 4,
Windows Vista instead of WinXP, stopping services) in later PRs ?

Yes a separate PR for remaining issues may be better.

Selva

mattock · 2016-11-21T08:03:49Z

@selvanair : yes, let's address the other issues in a separate PR.

chipitsine · 2016-11-21T09:35:20Z

so, can we merge it ?

mattock · 2016-11-21T11:41:39Z

windows-nsis/openvpn.nsi

+                File "${OPENVPN_ROOT_X86_64}\bin\openvpnserv.exe"
+        ${Else}
+                File "${OPENVPN_ROOT_I686}\bin\openvpnserv.exe"
+        ${EndIf}


Here are spaces instead of tabs. We could add a client-side Git hook to prevent spaces for the openvpn-build project. Or maybe we could make Travis fail if there are spaces in certain files such as this one?

git hooks are not platform independent. it is hard to make it work on unix/osx/windows
but we can do that with either Travis-CI or https://reviewable.io

I'll push a patch (with tabs) soon

@chipitsine : I always use "git diff" to have a look at the changes before commit: in general it reveals these tabs vs. spaces quite nicely.

mattock · 2016-11-21T13:41:27Z

windows-nsis/openvpn.nsi

-		# This is required because of commit 2bb1726764344 ("Do not disconnect on suspend").
-		# Without this users will experience weired disconnections after suspend/resume.
-		WriteRegStr "HKLM" "Software\OpenVPN-GUI" "disconnect_on_suspend" "0"
-


Just a note for the posterity: we agreed on the mailing list to remove this registry string..

mattock · 2016-11-21T13:56:35Z

We already use sc.exe in openvpn.nsi, so it would be trivial to use it to shut down OpenVPNService and OpenVPNServiceLegacy when necessary. That should cover 95% of cases where openvpn.exe is running. That said, a question comes to mind:

Does the current installer restart the services on OpenVPN reinstalls (no uninstall before install)?
- If yes, then I think we can retain that behavior and restart the services behind the user's back
- If not, then I think we should we ask for user confirmation before restarting anything

As for merging this: as debbie10t had some issues I want to run a few more tests myself to figure out what is going on.

chipitsine · 2016-11-21T14:10:54Z

as for discussion of sc.exe, it looks like we have not settled to move it to separate PR and continue to discuss it here. can we merge this PR ?

as for sc.exe, actually, you make PR if you want

mattock · 2016-11-21T14:12:33Z

Let's not add anything to this PR, otherwise we have to retest it again. I will run some additional tests for this PR on my Windows laptop later today. If all goes well, I'll merge this.

mattock · 2016-11-21T15:29:57Z

So I ran some tests and the installer with this PR seemed to work as expected. I did a bunch of install/uninstall/reinstall cycles both silently (/S) to the default and non-standard directories (/D). I also tested the installer interactively to make sure behavior is consistent. Finally, I then verified that the official 2.4_beta1 installer works the same, and it did. In a nutshell I was unable to break this PR. As this PR also worked for Selva I'm confident in merging this.

I did notice a bug in registry key handling, but that is unrelated to this PR, as it was reproducible also with the official 2.4_beta1 installers.

mattock reviewed Nov 16, 2016

View reviewed changes

chipitsine changed the title ~~initial "x86 + x64" installer commit~~ "x86 + x64" installer Nov 16, 2016

mattock reviewed Nov 18, 2016

View reviewed changes

mattock reviewed Nov 21, 2016

View reviewed changes

"x86 + x64" installer

62a6b3c

mattock reviewed Nov 21, 2016

View reviewed changes

mattock merged commit feae1d5 into OpenVPN:master Nov 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"x86 + x64" installer #44

"x86 + x64" installer #44

chipitsine commented Nov 15, 2016

mattock commented Nov 16, 2016

mattock Nov 16, 2016

mattock commented Nov 16, 2016

mattock Nov 16, 2016

chipitsine commented Nov 16, 2016

mattock commented Nov 17, 2016

chipitsine commented Nov 17, 2016

mattock commented Nov 17, 2016

mattock commented Nov 17, 2016

chipitsine commented Nov 17, 2016

mattock Nov 18, 2016

chipitsine Nov 18, 2016

mattock Nov 18, 2016

mattock commented Nov 18, 2016

mattock commented Nov 18, 2016

mattock commented Nov 18, 2016 •

edited

chipitsine commented Nov 18, 2016

chipitsine commented Nov 18, 2016

chipitsine commented Nov 18, 2016

mattock commented Nov 18, 2016

selvanair commented Nov 18, 2016

chipitsine commented Nov 18, 2016

chipitsine commented Nov 19, 2016 •

edited

selvanair commented Nov 19, 2016

mattock commented Nov 21, 2016

chipitsine commented Nov 21, 2016

mattock Nov 21, 2016

chipitsine Nov 21, 2016

mattock Nov 21, 2016

mattock Nov 21, 2016

mattock commented Nov 21, 2016

chipitsine commented Nov 21, 2016

mattock commented Nov 21, 2016

mattock commented Nov 21, 2016

"x86 + x64" installer #44

"x86 + x64" installer #44

Conversation

chipitsine commented Nov 15, 2016

mattock commented Nov 16, 2016

Choose a reason for hiding this comment

mattock commented Nov 16, 2016

Choose a reason for hiding this comment

chipitsine commented Nov 16, 2016

mattock commented Nov 17, 2016

chipitsine commented Nov 17, 2016

mattock commented Nov 17, 2016

mattock commented Nov 17, 2016

chipitsine commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattock commented Nov 18, 2016

mattock commented Nov 18, 2016

mattock commented Nov 18, 2016 • edited

chipitsine commented Nov 18, 2016

chipitsine commented Nov 18, 2016

chipitsine commented Nov 18, 2016

mattock commented Nov 18, 2016

selvanair commented Nov 18, 2016

chipitsine commented Nov 18, 2016

chipitsine commented Nov 19, 2016 • edited

selvanair commented Nov 19, 2016

mattock commented Nov 21, 2016

chipitsine commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattock commented Nov 21, 2016

chipitsine commented Nov 21, 2016

mattock commented Nov 21, 2016

mattock commented Nov 21, 2016

mattock commented Nov 18, 2016 •

edited

chipitsine commented Nov 19, 2016 •

edited