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

Installing a new version leaves the old version #735

Closed
ghost opened this issue Sep 8, 2016 · 27 comments · May be fixed by qcif/data-curator#563
Closed

Installing a new version leaves the old version #735

ghost opened this issue Sep 8, 2016 · 27 comments · May be fixed by qcif/data-curator#563

Comments

@ghost
Copy link

ghost commented Sep 8, 2016

  • Version: 6.5.2
  • Target: Windows

Steps to reproduce:

  1. Run an installer created with the 5.x series Electron builder, placing files in \Program Files\Something\
  2. Run an installer created with the 6.x series Electron builder, placing files in \Program Files\Something\{version}\

Expected result:
The files from the old installer should be removed

Observed result:
The old version is still there, and the new version is also there
There are two uninstallers listed in "Add/Remove Programs"
The desktop shortcut points at the right version

If you start the new installer with the old version still running, it does recognize that, and offer to kill the running application.

The main risk is that users who created their own shortcuts will continue to run the old version. The presence of two uninstallers will also be confusing to users.

My guess: The new code that moves the uninstaller out of the way #722 is not passing the right folder to the uninstaller when it invokes it.

@develar
Copy link
Member

develar commented Sep 8, 2016

Known expected issue. But now I realised that is not ok because previously app was installed not per version (no version folder) and such files are not removed. But must be removed.

@develar develar added the nsis label Sep 8, 2016
@develar
Copy link
Member

develar commented Sep 8, 2016

Error because we require UninstallerPath registry key and such key created only by new version.

@ghost
Copy link
Author

ghost commented Sep 9, 2016

When I run the old installer, it creates this registry key:
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\{application name}\UninstallString and the path of the old uninstaller is there.

@develar
Copy link
Member

develar commented Sep 10, 2016

I don't have currently time to test all possible cases, auto test planned to finish. This case should be fixed.

@develar develar added the bug label Sep 10, 2016
@develar
Copy link
Member

develar commented Sep 11, 2016

Another fix was pushed (yep, now we have integration tests for it).

@ghost
Copy link
Author

ghost commented Sep 12, 2016

Wait. Maybe disregard the below, because I'm looking in /usr/local/lib/node_modules/electron-builder/templates/nsis/installSection.nsh and I don't see any of those changes from that commit. I guess that means this isn't in 6.7.3, right?

I just repeated the steps from my report using the 6.7.3 version and the problem is still there.

Looking at the code, I think this is the issue:

33+    ReadRegStr $R1 SHCTX "${INSTALL_REGISTRY_KEY}" InstallLocation

There is no such registry entry on my machine after running the 5.x installer.

In fact, the only thing I see in the registry that says where the program is installed, is the path of the uninstaller, which you already have in $R0. Could you just use something like the GetParent function here: http://www.1014.org/code/nullsoft/nsis/functions.htm to compute $R1 based on the value of $R0?

@ghost
Copy link
Author

ghost commented Sep 13, 2016

When will this change be in the NPM registry so I can try it?

@develar
Copy link
Member

develar commented Sep 13, 2016

@joshua-smith 6.7.3 is published, your note use GetParent is not yet addressed (planned, tomorrow).

@ghost
Copy link
Author

ghost commented Sep 13, 2016

I'm confused by this whole GIT/NPM thing. Sorry. When I look at that commit, it's 140 lines of code, starting with

InitPluginsDir

And I installed on my local machine with

sudo npm install electron-builder@next -g

And it says I have 6.7.3:

/usr/local/lib
└─┬ electron-builder@6.7.3 

but when I look at /usr/local/lib/node_modules/electron-builder/templates/nsis/installSection.nsh it's only 111 lines, and the first line is:

# http://stackoverflow.com/questions/24595887/waiting-for-nsis-uninstaller-to-finish-in-nsis-installer-either-fails-or-the-uni

So it appears that while I have 6.7.3, I don't have the code you committed.

@develar
Copy link
Member

develar commented Sep 13, 2016

@joshua-smith Your version is correct, master version has 111 lines of code — https://github.com/electron-userland/electron-builder/blob/master/templates/nsis/installSection.nsh

Latest change to file installSection.nsh is not linked to this issue, but linked to #722.

@ghost
Copy link
Author

ghost commented Sep 13, 2016

But #722 is also closed. I'm just trying to understand where we are on the development path, because the people waiting on me are kind of freaking out about still not having an installer they can use. If you do that getParent thing tomorrow morning, how long until I see that in NPM?

(I just want to emphasize that I'm not complaining about the process here. What you are doing is great. I'm just trying to understand the relationship between you closing bugs and those fixes showing up in NPM.)

@develar
Copy link
Member

develar commented Sep 13, 2016

how long until I see that in NPM

You will get next version in 2 * build time on Travis agent using build configuration 1. i.e. 2 * ~10 minutes i.e. ~20 minutes. Multiplied by 2 because direct push to master is prohibited, commit is checked before in some branch.

Publish is automatic — every fix/feat commit triggers new version.

next version is available on NPM, but to install it, you have to explicitly specify version using @ — e.g. npm install electron-builder@next
next marked as latest (and, so, available for all by default) when it checked and considered stable.

the relationship between you closing bugs and those fixes showing up in NPM

No strict rule here — yes, I pushed new commit and linked it to #722 but I didn't reopen this issue — because my fix was just a better and more correct version of fix. Why not refactor, but fix? Because commit was more like fix :)

@develar
Copy link
Member

develar commented Sep 14, 2016

I just repeated the steps from my report using the 6.7.3 version and the problem is still there.

Cannot reproduce perMachine one-click installer. InstallLocation was always written. Anyway, getParent will be used to make uninstaller more robust.

@ghost
Copy link
Author

ghost commented Sep 14, 2016

This problem is still present in 6.7.4. I see the call to GetFileParent, so I'm definitely running the new code.

One thing I noticed looking at the registry is that you are generating a proper GUID automatically. The previous version just used the app name as the GUID. As a result, the old installer and the new installer have different GUIDs. I'm wondering if that's an issue?

@develar
Copy link
Member

develar commented Sep 14, 2016

GUID

GUID logic was not changed. Please see https://github.com/electron-userland/electron-builder/wiki/NSIS#guid-vs-application-name

I built per-machine installer using 5.30.0 and 6.7.3 correctly deletes it.

@develar
Copy link
Member

develar commented Sep 14, 2016

Do you build the same app using different versions of electron-builder? Maybe appId or name was changed in your app?

@ghost
Copy link
Author

ghost commented Sep 14, 2016

The original installer was built using electron-builder and the config.json file I used didn't actually have a "name" attribute in it. It had a "title" attribute, which I suppose it used as the name. When I look at the old installer.nsi.tpl, it used ${APP_NAME} the way your new code uses ${PROGRAM_FILENAME}.

When I went to the new version, it insisted that I needed a "name" attribute, however, it did not allow me to use the title as the name because the title had spaces. So now I'm setting the "name" attribute to the title, but with spaces converted to underscores.

Looks like I was using electron-builder 2.7.1 before I upgraded.

So that's probably why it's not working. The 2.7.1 version probably just installed things very, very differently than the 5.x version did. I'll do some experiments and look at the code, and get back to you...

@ghost
Copy link
Author

ghost commented Sep 14, 2016

I can work around the GUID issue by setting nsis.guid to the application Title. That allows your code to find the uninstaller.

However, you are invoking the uninstaller incorrectly. Per the documentation:

_?= sets $INSTDIR. It also stops the uninstaller from copying itself to the temporary directory and running from there. It can be used along with ExecWait to wait for the uninstaller to finish. It must be the last parameter used in the command line and must not contain any quotes, even if the path contains spaces.

So this line:

      ExecWait '"$PLUGINSDIR\old-uninstaller.exe" "_?=$R1" /S /KEEP_APP_DATA $0'

Needs to be:

      ExecWait '"$PLUGINSDIR\old-uninstaller.exe" /S /KEEP_APP_DATA $0 _?=$R1'

I moved the _? to the end of the line and deleted the quotes.

I tested this in a cmd window and it works correctly. (Note that the way you had it, uninstaller would actually delete the plugins folder!)

develar added a commit that referenced this issue Sep 14, 2016
"the last parameter used in the command line and must not contain any quotes, even if the path contains spaces"
@develar
Copy link
Member

develar commented Sep 14, 2016

I missed this doc, thanks a lot, but actually, why does it work for me? Anyway — change pushed.

@develar
Copy link
Member

develar commented Sep 14, 2016

The 2.7.1 version probably just installed things very, very differently than the 5.x version did. I

Yes. Completely differently. Because NSIS support was dropped in 3 version and restored in the 4 (completely rewritten).

@ghost
Copy link
Author

ghost commented Sep 14, 2016

It is puzzling why it would work for you. The uninstaller I'm using, of course, is pretty old, since it's contemporary with 2.7.1 of electron-builder. Perhaps older uninstallers are more particular than the one you are including now?

@ghost
Copy link
Author

ghost commented Sep 14, 2016

One last bug. This line:

  ReadRegStr $R0 ${ROOT_KEY} "${UNINSTALL_REGISTRY_KEY}" UninstallString

Finds a string that doesn't have quotes around it. So when you call:

    Push $R0
    Call GetInQuotes
    Pop $R0

That leaves an empty string in $R0.
Simple fix:

    Push $R0
    Call GetInQuotes
    Pop $R1
    ${if} $R1 != ""
      Push $R1
      Pop $R0
    ${endif}

(No idea if that push/pop is a sane way of doing a variable assignment. I'm just guessing here.)

@develar
Copy link
Member

develar commented Sep 15, 2016

UninstallString always contains " Or do you mean old installer (2.x)?

@develar
Copy link
Member

develar commented Sep 15, 2016

UninstallString always contains " Or do you mean old installer (2.x)?

If not quoted, so, it is incorrect (corrupted?) string and we just skip it.

@ghost
Copy link
Author

ghost commented Sep 15, 2016

screen shot 2016-09-15 at 10 13 01 am

In the 2.x series, the `UninstallString` is there but just has the path to the Uninstaller, without quotes. So I need that change for the uninstall of the old app to happen correctly.

develar added a commit to develar/electron-builder that referenced this issue Sep 16, 2016
@develar
Copy link
Member

develar commented Sep 16, 2016

@joshua-smith Fixed.

@ghost
Copy link
Author

ghost commented Sep 16, 2016

Confirmed that this is completely fixed in 6.7.7. Thank you!

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

Successfully merging a pull request may close this issue.

1 participant