Closed Bug 757411 Opened 8 years ago Closed 8 years ago

Add Installer/Uninstaller improvements from Mozmill automation scripts to mozInstall

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file)

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.
Depends on: 758864
Attached file Patch (v1)
Pointer to Github pull-request
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!
Attachment #628495 - Attachment description: Pointer to Github pull request: https://github.com/mozilla/mozbase/pull/16/files → Patch (v1)
Attachment #628495 - Flags: review?(jhammel)
Attachment #628495 - Flags: feedback?(robert.bugzilla)
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?
(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 on attachment 628495 [details]
Patch (v1)

This is a fairly major refactor but it looks good to me.
Attachment #628495 - Flags: review?(jhammel) → review+
Blocks: 760375
Blocks: 760377
Summary: Move Installer/Uninstaller code from Mozmill automation scripts to mozInstall → Add Installer/Uninstaller improvements from Mozmill automation scripts to mozInstall
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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 762287
Duplicate of this bug: 698849
Comment on attachment 628495 [details]
Patch (v1)

Should be fine
Attachment #628495 - Flags: feedback?(robert.strong.bugs)
You need to log in before you can comment on or make changes to this bug.