642453 - Stop cfx when using Firefox 3.6 binary

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Alexandre Poirot [:ochameau]]

Attachment: Stop cfx execution and display a message

I dig into mozrunner and I didn't found any code to search on alternative paths 
(like /program files/mozilla firefox 4.0/). I didn't found code to retrieve firefox version but I use some code that retrieve platform information.

This is not the best solution ever but it can already be really helpfull for beginners!

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 519914 [details] [diff] [review]
Stop cfx execution and display a message

>diff --git a/python-lib/cuddlefish/runner.py b/python-lib/cuddlefish/runner.py

>+    platform_repo = runner.get_repositoryInfo().get('platform_repository')
>+    if platform_repo and platform_repo.find("mozilla-2") == -1:

Hmm, this is going to break the next time the platform's major version changes.  Last time it took a decade or so, but next time it'll probably happen sooner, and even if it takes a while, we should prepare for that eventuality so this code doesn't break after we've long forgotten about it.

So this should look for mozilla-[n] and make sure [n] is 2 or greater.


>+      print "  Addon SDK require Firefox 4 or greater."
>+      print "  Please install Firefox 4, or give a path to a recent firefox" \
>+        " version with:"
>+      print "    cfx -b FIREFOX_PATH"

This would be a bit better as:

  cfx requires Firefox 4 or greater and is unable to find a compatible
  binary.  Please install a newer version of Firefox or provide the path
  to your existing compatible version with the --binary flag:

      cfx --binary=PATH_TO_FIREFOX_BINARY

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Comments adressed

I didn't had time to get back into python world,
so do not hesitate to suggest better way to implement this with common python patterns!

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 521186 [details] [diff] [review]
Comments adressed

Actually, let's have Brian check how well this patch conforms to Python best practices.

Comment 5

[Brian Warner [:warner :bwarner]]

Comment on attachment 521186 [details] [diff] [review]
Comments adressed

Some recommendations:

* use four-space indent, not two (to match the rest of the file)
* re.search() returns None if the regexp didn't match, and groups() always
  returns a fixed-length tuple, and anyways zero-length tuples are False.
  Also, you should use a "raw" string for the regexp to prevent "\d" from
  being turned into a control character like "\n" or "\t". So I'd write it
  like this:

    mo = re.search(r"mozilla-(\d+)", platform_repo)
    if not mo:
        print "unable to find a mozilla version string in", platform_repo
        return
    version = mo.group(1)
    if int(version) < 2:
        print """cfx requires Firefox 4 or greater..."""
        return

("mo" stands for "match object", which is what re.search() and re.match()
return)

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: "Python approved" patch!

Comment 7

[Brian Warner [:warner :bwarner]]

Comment on attachment 523070 [details] [diff] [review]
"Python approved" patch!

Looks great! Landed in https://github.com/mozilla/addon-sdk/commit/ae6a089e89b771798b064b622baf4d4c9105183a

Comment 8

[Brian Warner [:warner :bwarner]]

oops, we had to revert this in https://github.com/mozilla/addon-sdk/commit/024c3d716a6526820d5e958c40b8e9968ac4a3c3 . The buildbot went orange (i.e. unit tests were failing).

Now that I know more about what that get_repositoryInfo() is doing, I no longer think this is a good way to calculate the version of firefox we're using. (the method is effectively retrieving the URL of the Mercurial repository from which that firefox was built, and it just so happens that ff3.6 was built from a different repo than ff4.0). On IRC we decided that running some form of "firefox -v" and parsing the output would be a better approach.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

(Note: if for some reason we wind up recommitting the existing change, we should commit https://github.com/mozilla/addon-sdk/pull/143 as well.)

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(automatic reprioritization of 1.0 bugs)

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Call ./firefox -v to check vinary version

New try ...
This time I used subprocess to get the output of "firefox -v".
There is new issues with this approach:
1/ "-v" output has various schemes:
  Mozilla Firefox 4.0.1
  Mozilla Firefox 3.6.16, Copyright (c) 1998 - 2011 mozilla.org
I didn't verified, but I'm pretty sure that Iceweasel replace Firefox by Iceaweasel!
  I tried to make a safe RegExp, but I may have missed some other pattern?

2/ We are explicitely checking firefox version instead of platform version.
(previous patch checked platform one) So we should not prohibit users playing with cfx on other mozilla applications: thunderbird, fennec, seamonkey, ... 
So I print a warning message in case the RegExp didn't match and still allow running cfx.

Comment 12

[Brian Warner [:warner :bwarner]]

Comment on attachment 540420 [details] [diff] [review]
Call ./firefox -v to check vinary version

I like it, but subprocess.check_output() was only added in Python2.7, and we need to be compatible with python2.5 . You'll need to expand this, probably by just copying the convenience function's code out of subprocess.py (it's about 9 lines). Making it a distinct function (i.e. start with 'def check_output') will help readers who are already familiar with subprocess.check_output.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Include check_output in sdk

Same patch with check_output included in SDK sources.

Comment 14

[Brian Warner [:warner :bwarner]]

Comment on attachment 541697 [details] [diff] [review]
Include check_output in sdk

Looks good! Committed in https://github.com/mozilla/addon-sdk/commit/ab154409fdbcb8a6a641ee126411478dff5b61ca with a few small changes to the comments.