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

Fixed firefox binary path for Windows and Mac in browser.py #9900

Merged
merged 1 commit into from Mar 15, 2018

Conversation

Projects
None yet
4 participants
@Cactusmachete
Copy link
Contributor

Cactusmachete commented Mar 7, 2018

find_executable() earlier returned None for non-linux systems, causing
firefox browser install to fail. Added the different binary paths for each OS.
Also removed platform_string() which used to be called in install() before nightly
download depended on mozdownload, and is now dead code.
Removed xfail for firefox install and run on MacOS in test_wpt.py as it will work now.


This change is Reviewable

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

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

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 7, 2018

Build PASSED

Started: 2018-03-15 11:29:25
Finished: 2018-03-15 11:44:51

View more information about this build on:


if platform == "linux":
bin_path = os.path.join(install_dest, "firefox")
elif platform == "win":

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 7, 2018

Contributor

I wonder if we should add firefox to the dest in non-linux cases, to ensure we aren't overwriting existing files.

This comment has been minimized.

Copy link
@Cactusmachete

Cactusmachete Mar 7, 2018

Author Contributor

Afaik it shouldn't overwrite any files besides the firefox ones in non-linux cases.
On mac, it installs the .dmg to make the firefox nightly app, and in windows, the installer sets it up so that the firefox build is within a new folder named with the nightly version and language it installed.
Not sure if these can lead to us overwriting anything, but then again I don't have much experience with the two OSes.

}.get(uname[0])

if platform is None:
# This was earlier checked in platform_string, which isn't needed anymore since we switched

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 7, 2018

Contributor

I think this comment should explain why the code is as it is rather than explain the historical context (I'm not actually sure a comment is needed here at all).

This comment has been minimized.

Copy link
@Cactusmachete

Cactusmachete Mar 7, 2018

Author Contributor

Alright! Added it just to clarify why it was here instead of in platform_string rather than anywhere else. Will remove it

@jgraham
Copy link
Contributor

jgraham left a comment

This is going in the right direction and dI"m very pleased to be getting such a silly and non-obvious bug fixed, but I think we need a slightly larger refactor to be confident it won't break in the future.

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

dest = os.path.join(os.getcwd(), "_venv")

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 8, 2018

Contributor

Instead of this I think we should figure out how to pass the virtualenv object into find_binary. I know that's a slightly larger refactor but it won't break if we change the venv name.

This comment has been minimized.

Copy link
@Cactusmachete

Cactusmachete Mar 8, 2018

Author Contributor

Alright, I'll see what I can do about this.


def find_binary(self):
return find_executable("firefox")
platform = {

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 8, 2018

Contributor

I think we ended up with some duplicated code working out the binary path in the virtualenv, is that possible to factor into a function?

This comment has been minimized.

Copy link
@Cactusmachete

Cactusmachete Mar 8, 2018

Author Contributor

Something like find_binary_path is definitely doable. We can pass the virtualenv object to it (instead of to find_binary) and have find_binary use it to ensure that we return the firefox on path if its not installed in _venv

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Mar 8, 2018

@Cactusmachete I think the find_binary will be fine if it doesn't have to search the entire virtualenv. So if we can arrange things so that the path always start with _venv/firefox I think that would work well.

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Mar 12, 2018

Please rebase this, and squash the commits together into something with meaningful history (probably just one commit for the whole thing). Please ask if that doesn't make sense.

@Cactusmachete Cactusmachete force-pushed the Cactusmachete:cactus2 branch from 80ad551 to 6b197a2 Mar 12, 2018

@wpt-pr-bot wpt-pr-bot requested review from halindrome and joanmarie Mar 12, 2018

@Cactusmachete Cactusmachete force-pushed the Cactusmachete:cactus2 branch 2 times, most recently from dacd252 to bb8baf3 Mar 12, 2018

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Mar 12, 2018

This branch now contains a merge commit that introduces a whitespace error :) There shouldn't be an merge commits on the branch at all (you should use something like git rebase -i origin/master to rebase onto master).

@Cactusmachete Cactusmachete force-pushed the Cactusmachete:cactus2 branch 6 times, most recently from 12461b2 to 019cd02 Mar 12, 2018

Cactusmachete
Fixed firefox binary path
Created find_binary_path to return binary path in _venv, earlier find_binary
only returned firefox installed to path.
Changed nightly uninstallation conditional.
Moved nightly to _venv/browsers for easier search when we use
mozinstall.get_binary()
Removed xfail for firefox run/installtion from test_wpt as it doesn't
fail anymore.

@Cactusmachete Cactusmachete force-pushed the Cactusmachete:cactus2 branch from 019cd02 to b10e8e5 Mar 15, 2018

@jgraham jgraham merged commit df9060d into web-platform-tests:master Mar 15, 2018

1 check passed

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

@Cactusmachete Cactusmachete deleted the Cactusmachete:cactus2 branch Mar 18, 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.