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

Firefox browser install now uses mozdownload and mozinstall #9748

Merged
merged 5 commits into from Mar 5, 2018

Conversation

Projects
None yet
5 participants
@Cactusmachete
Copy link
Contributor

Cactusmachete commented Mar 2, 2018

mozinstall will refuse to install if firefox is already installed
(when checked on OSX). I've currently only set it to print an error message
when this happens - it'll require the user to uninstall firefox in _venv and run
the command again as uninstall in mozinstall doesn't seem to work very well.

Closes #7996


This change is Reviewable

Cactusmachete
Firefox browser install now uses mozdownload and mozinstall
mozinstall will refuse to install if firefox is already installed
(when checked on OSX). I've currently only set it to print an error message
when this happens - it'll require the user to uninstall firefox in _venv and run
the command again as uninstall in mozinstall doesn't seem to work very well.

Closes #7996

@wpt-pr-bot wpt-pr-bot added the infra label Mar 2, 2018

@wpt-pr-bot wpt-pr-bot requested review from gsnedders and jgraham Mar 2, 2018

@jgraham
Copy link
Contributor

jgraham left a comment

This is a very exciting change; I love all the deleted code :) I think a little cleanup is required to land it though.

resp = get(nightly_link)
resp.raise_for_status()
untar(resp.raw, dest=dest)
filename = FactoryScraper('daily', branch='mozilla-central').download()

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 2, 2018

Contributor

I think we should set a destination here so that it ends up inside the venv directory.

try:
#When tested in OSX, mozinstall will refuse to install if firefox is already installed
mozinstall.install(filename, dest)
except Exception as e:

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 2, 2018

Contributor

It would be nice to catch a more specific exception type here.

#When tested in OSX, mozinstall will refuse to install if firefox is already installed
mozinstall.install(filename, dest)
except Exception as e:
# Instead of printing an error message we should uninstall the currently

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 2, 2018

Contributor

Can we just figure out what to delete based on the path, or does something more complex happen on OSX here?

This comment has been minimized.

Copy link
@Cactusmachete

Cactusmachete Mar 2, 2018

Author Contributor

Messed around a bit, turns out it's a simple fix. I'll push to the branch in a bit.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 2, 2018

Build PASSED

Started: 2018-03-05 21:07:32
Finished: 2018-03-05 21:12:23

View more information about this build on:

Cactusmachete added some commits Mar 3, 2018

Cactusmachete
Added support for uninstalling on OSX
Wasn't sure why this was failing on travis. Added mozdownload and mozinstall
to firefox_requirements hoping that would help.
Cactusmachete
Removed test_browser.py as it was causing travis ci to fail
test_browser was calling dead code in browser.py, so travis failed.
@@ -4,3 +4,5 @@ mozprocess == 0.26
mozcrash == 1.0
mozrunner == 6.14
mozleak == 0.1
mozinstall == 1.15

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 5, 2018

Contributor

This should't be both in here and in the commands.json file; here is better but you might need to move the moz* imports you added into ttthe function where they're actually used.

This comment has been minimized.

Copy link
@Cactusmachete

Cactusmachete Mar 5, 2018

Author Contributor

If I remove these requirements from the commands.json file, the code crashes when I try to set up a new venv because mozdownload/install don't get installed there in it.
When I remove them from this file, it could cause the build to fail in Travis if anything calls browser.py because they don't get installed there.

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 5, 2018

Contributor

So I think this is possible, but it involves ensuring that we install the product-specific requriements later on. Let's do that in a followup.

"Darwin": "mac"
}.get(uname[0])

if platform == "mac":

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 5, 2018

Contributor

if uname[0] == "Darwin" seems simpler if we really only need this on one platform. But it's unclear why macOS is special cased here. Does it really work differently on mac? If it does a comment explaining what's going on seems helpful, otherwise we should support all for Windows/Linux/Mac here

This comment has been minimized.

Copy link
@Cactusmachete

Cactusmachete Mar 5, 2018

Author Contributor

Alright!
I checked it on all three OSes. mozinstall only throws an error on Mac.
Afaict this is because mozdownload gets a .dmg on Mac, but zipped folders in Lin/Win which it simply unzips there.
I'll need to get my hands on a Windows machine to make sure I'm right though, will be able to confirm in an hour or two.

else:
raise

os.remove(filename)
return find_executable("firefox", os.path.join(dest, "firefox"))

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 5, 2018

Contributor

Should we consider mozinstall.get_binary here?

This comment has been minimized.

Copy link
@Cactusmachete

Cactusmachete Mar 5, 2018

Author Contributor

We could, but I have no way of testing this on a Mac anymore.

Cactusmachete added some commits Mar 5, 2018

@jgraham

jgraham approved these changes Mar 5, 2018

@@ -4,3 +4,5 @@ mozprocess == 0.26
mozcrash == 1.0
mozrunner == 6.14
mozleak == 0.1
mozinstall == 1.15

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 5, 2018

Contributor

So I think this is possible, but it involves ensuring that we install the product-specific requriements later on. Let's do that in a followup.

@jgraham

jgraham approved these changes Mar 5, 2018

@jgraham jgraham merged commit c7c6f86 into web-platform-tests:master Mar 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@csnardi

This comment has been minimized.

Copy link
Member

csnardi commented Mar 5, 2018

@Cactusmachete Cactusmachete deleted the Cactusmachete:ahilyasinha branch Mar 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.