757411 - Add Installer/Uninstaller improvements from Mozmill automation scripts to mozInstall

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As discussed in the A-team workweek we want to combine the installer/uninstaller code from the Mozmill automation repository with mozinstall. This bug will take care of it.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch (v1)

Pointer to Github pull-request

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 628495 [details]
Patch (v1)

This patch adds the uninstaller code to mozinstall and does a bit of cleanup across the whole module.

Something I'm not really sure about is the way how we handle the uninstaller on Windows. As what we have seen in the past, 'helper.exe' seems to be a wrapper only, calls another executable and exits immediately. To ensure that we really wait until the uninstaller has been finished successfully, we currently are waiting for the uninstaller folder to be removed. Rob, is there a better way in handling that? If not I would like to keep that method because it is working fine for us since ages. Otherwise I'm absolutely open for better options. Thanks!

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Clint and Jeff, given that the install() method is a public API I wonder if I can remove the apps keyword and let this method return the installation path instead of the binary. I think that the test or surrounding framework should call get_binary() itself if necessary. It should not be done by the installer automatically. That way it's hard to figure out the real installation path. What do you think? Can we change that? How many other places would be affected?

Comment 4

[Jeff Hammel]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> Clint and Jeff, given that the install() method is a public API I wonder if
> I can remove the apps keyword and let this method return the installation
> path instead of the binary. I think that the test or surrounding framework
> should call get_binary() itself if necessary. It should not be done by the
> installer automatically. That way it's hard to figure out the real
> installation path. What do you think? Can we change that? How many other
> places would be affected?

I am inclined to force passing what app you want, as if you're trying to install firefox you don't want to get a thunderbird binary.  Obviously some care should be taken to check e.g. 'firefox' vs 'Firefox', etc which could take some thought.  OTOH, I prefer the current behaviour of returning the path to the binary.  I think sorting this out is best left for another bug

Comment 5

[Jeff Hammel]

Comment on attachment 628495 [details]
Patch (v1)

This is a fairly major refactor but it looks good to me.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on master as:
https://github.com/mozilla/mozbase/commit/bfd2d9292630267ebd9aa73035a2de07d0ce1307

Rob, whenever you have time please check the code for the uninstaller on Windows as requested above. If we can make improvements we can create a follow-up bug but I had to checkin the patch, so that we can continue our work. Thanks.

Comment 8

[Robert Strong (they/them - no direct email)]

Comment on attachment 628495 [details]
Patch (v1)

Should be fine