712464 - SDK fails to find nightly build with new FirefoxNightly.app filename

Status: RESOLVED FIXED

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

Description

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

Firefox nightly builds on Mac are now called FirefoxNightly.app, but the SDK fails to find them.  Presumably that's because mozrunner is looking for Nightly.app.

Comment 1

[Dave Townsend [:mossop]]

Brian, can you take a look at this for 1.4. I'm assuming the mozrunner guys will themselves have to do some fix here so maybe it is just cherrypicking that.

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

As a guess, probably here: https://github.com/mozilla/addon-sdk/blob/master/python-lib/mozrunner/__init__.py#L294

On a somewhat unrelated note, couldn't we drop "shiretoko" from the list? That was the codename for the 3.5 branch while in development, and I'm pretty sure we no longer support 3.5. :)

Comment 3

[Dave Townsend [:mossop]]

(In reply to Wes Kocher (:KWierso) (Jetpack Bugmaster) from comment #2)
> As a guess, probably here:
> https://github.com/mozilla/addon-sdk/blob/master/python-lib/mozrunner/
> __init__.py#L294
> 
> On a somewhat unrelated note, couldn't we drop "shiretoko" from the list?
> That was the codename for the 3.5 branch while in development, and I'm
> pretty sure we no longer support 3.5. :)

This file comes straight from mozrunner (developed by the automation team). I'm guessing we want to keep it as close to in sync with what they have as possible.

Comment 4

[Brian Warner [:warner :bwarner]]

It's not failing for me when I do "cfx testall -b /Applications/FirefoxNightly.app", but maybe you're talking about a system that has *only* FirefoxNightly.app installed. What's the STR?

re diverging from mozrunner, we're already pretty far along our own little fork. Upstream was talking about removing search-for-firefox support altogether. See Bug 703646 for some of the discussion.

I'll attach a tiny patch that adds "firefoxnightly" to the list that Wes found.. can someone who can reproduce this problem test it?

Comment 5

[Brian Warner [:warner :bwarner]]

Attachment: add "firefoxnightly" to the list of names that we look for

This seems like a plausible fix, but I don't currently know how to reproduce this one, and I haven't yet traced how this list of names is used in mozrunner, so this is a blind attempt at a fix. If it works, great. If not, let me know and I'll dig deeper.

Comment 6

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

Comment on attachment 583979 [details] [diff] [review]
add "firefoxnightly" to the list of names that we look for

Unfortunately, this is insufficient, as the application bundle is called *FirefoxNightly.app*, and this patch makes Mozrunner look for *Firefoxnightly.app* (lowercase "n"), because it calls `str.capitalize()` on the name, which converts *firefoxnightly* to *Firefoxnightly*.

Nor would it be sufficient to add *firefoxNightly* to the list, since `str.capitalize()` lowercases the second and subsequent characters in a string on which it is called.

In its logic for finding a binary on Mac OS X <https://github.com/mozilla/addon-sdk/blob/master/python-lib/mozrunner/__init__.py#L406-419>, Mozrunner overloads `FirefoxRunner.names` to mean both application bundle names and binary names, which complicates matters, since bundle names vary, while binary names stay the same (because bundles are named after update channels like Nightly and Aurora, while binaries are all called "firefox-bin").

But it looks like it would be sufficient to make `FirefoxRunner.names` return studlily-capitalized bundle names and then not alter the cases of its values when looking for the corresponding binaries, since Mozrunner looks for them at "firefox-bin" if it doesn't find them at ${name}-bin.

Note: you changed `FirefoxProfile.names`, but `FirefoxRunner.names` seems to be the property being consulted, although presumably it's worth changing both in case the other is consulted at some point.

Note: Windows and Linux don't seem to have this problem, as the names of the directories (the equivalent of application bundles on those platforms) for their nightly builds haven't changed.

Comment 7

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

Attachment: patch v2: addresses issues in first patch

Here's a patch that addresses the issue in the first patch.

Note that it lowercases the name when looking for the application binary even though application binaries aren't named after their bundles (anymore?), just in case there is some edge case I'm unaware of in which binaries are named after their bundles (although I suspect this is obsolete behavior rather than an edge case).

With this patch, Mozrunner finds /Applications/FirefoxNightly.app when it is installed and /Applications/Firefox.app if it is installed and /Applications/FirefoxNightly.app isn't.

Comment 8

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

Note: we should also remove Shiretoko and add Aurora and Firefox Beta to the list of names; but that's a separate bug.

Comment 9

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

Attachment: patch v3: addresses issues in second patch

Brian identified some other uses of self.names that the previous patch would break.

The more we dug into it, the more it seemed like `self.names` is intended to list the names of binaries rather than bundles, and changing its meaning causes problems.  So this patch adds a separate list of bundle names that is only consulted on Mac OS X.

The old logic looped over binary names, looking for a bundle with each (capitalized) name that has a binary with either the same name or *firefox*.  It continues looking for binaries even after it finds one, so it loops over names in reverse order of preference (so as to find the most preferred binary last).

The new logic loops over bundle names, looking for a bundle with each name that has a binary with any of the possible names, including *firefox*.  It stops looking for binaries after it finds one, so it loops over bundles in order of preference.

The only potential difference in the outcome is that the new logic could theoretically find a binary whose name is different from the name of the bundle and is also not *firefox*, f.e. it could find a binary named *shiretoko* inside a bundle named *Firefox.app*.  But that should never occur in the wild; and if it did, it wouldn't necessarily be a bad thing to find it.  Binary finding is, at worst, expanded with this change; it isn't constrained.


> 12:11:15 - warner: also, I'm pretty sure that the @property approach isn't necessary: sys.platform and os.name are known when the class is evaluated, so you could use 'if xxx:\n names = [..]\n etc' instead of '@property\ndef names(self):\n if xxx:\n return [..]\n'

Given the risk of making the necessary changes, I decided to leave out this unnecessary one, although I see lots of opportunity for cleanup in this code!

Comment 10

[Brian Warner [:warner :bwarner]]

Comment on attachment 584477 [details] [diff] [review]
patch v3: addresses issues in second patch

Looks good. You could be a bit more consistent with the use of os.path.join versus hard-coded slashes.. all this code is only used under darwin, so you don't actually need the genericness of os.path.open, but it reads a bit funny to use e.g. os.path.join(appdir, 'Contents/MacOS',..) .

The only thing I'd actually change is to replace os.path.expanduser('~/') with just os.path.expanduser('~'), since the former will get you a doubled slash, which won't affect open(), but every once in a while I encounter a tool which treats /a/b//c/d the same as /c/d , which would be bad.

How about appdir = os.path.expanduser("~/Applications/%s.app" % bundle_name) ? And likewise appdir = "/Applications/%s.app" % bundle_name , and binpath = os.path.join(appdir, "Contents/MacOS/%s-bin" % binname .

Works for me here. r+ with at least the ~/ change.

Comment 11

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

(In reply to Brian Warner [:warner :bwarner] from comment #10)
> How about appdir = os.path.expanduser("~/Applications/%s.app" % bundle_name)
> ? And likewise appdir = "/Applications/%s.app" % bundle_name , and binpath =
> os.path.join(appdir, "Contents/MacOS/%s-bin" % binname .

Seems fine to me; will pushu will all three changes.

Comment 12

[[github robot]]

Commit pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/cd5224e29eec038e879db3652e57f63e7ed66bc2
fix bug 712464 - SDK fails to find nightly build with new FirefoxNightly.app filename; r=@warner

Comment 13

[[github robot]]

Commit pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/eb9bbd44565ce7b0fd94eac8545400b82c434876
fix bug 712464 - SDK fails to find nightly build with new FirefoxNightly.app filename; r=@warner
(cherry picked from commit cd5224e29eec038e879db3652e57f63e7ed66bc2)

Comment 14

[[github robot]]

Commit pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/eb9bbd44565ce7b0fd94eac8545400b82c434876
fix bug 712464 - SDK fails to find nightly build with new FirefoxNightly.app filename; r=@warner