Support for macOS application bundle paths

RESOLVED FIXED in Firefox 67

Status

enhancement
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: ato, Assigned: nupurbaghel)

Tracking

({good-first-bug})

Version 3
mozilla67
x86_64
macOS
Points:
---

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 months ago

It is currently not possible to to give geckodriver the path to an
application bundle, for example /Applications/Firefox.app. Instead
you must give it the absolute path to the executable binary inside
the bundle, e.g. /Applications/Firefox.app/Contents/Mac OS/firefox-bin.
It would be a considerable user experience improvement if we would
allow application bundle paths.

See https://github.com/mozilla/geckodriver/issues/1488 for an example
of how this is confusing to users.

To fix this bug, you would first have to add some code to mozrunner
to extrapolate from a given path what the real executable path
is. Specifically on macOS, this could involve a check if the given
path is an application bundle (is it a directory, and does it contain
an executable inside it called firefox-bin?), and if so return the
real path. On other systems this could be a no-op and the input
path could be returned straight.

I landed a documentation change in
https://bugzilla.mozilla.org/show_bug.cgi?id=1524586 explaining
that we don’t support bundle paths, and this would have to be
reverted as part of this change.

(Reporter)

Updated

2 months ago
Keywords: good-first-bug

You cannot easily search but would have to query the Contents/Info.plist file for the CFBundleExecutable entry.

I would suggest to use a crate like https://crates.io/crates/plist to make that happen. We would have to audit it still before we are able to include it in our tree.

Comment 3

2 months ago

Would I be able to work on this one?

(Reporter)

Comment 4

2 months ago

Certainly. See
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
for an introduction to development at Mozilla and
https://moz-conduit.readthedocs.io/en/latest/ about submitting
patches.

Assignee: nobody → vihavira
Status: NEW → ASSIGNED
(Assignee)

Comment 5

a month ago

Hi, I would like to take up this issue and have some doubts-

  • To allow support for application bundle path we need to alter the path during its creation here. But, since we want to apply this change to only mac os applications, how to figure out the system type through code? Is there any specific function for this?
  • Also, is it necessary to have above code under the mac os specific area?
(Reporter)

Comment 6

a month ago

Instead of converting the application bundle path to an absolute
path pointing to the binary at construction time, I think it would
be better to store what path given by the consumer and expand it
when ::start() is called.

To expand the path in a system-specific way, you need a function
guarded with #[cfg(target_os = "macos")] which checks if the path
is a directory, and iff so tries to locate the binary using the
plist crate. For the other systems you implement the same function
(with a #[cfg(not(target_os = "macos"))]) as a no-op, just returning
the path unaltered.

Does that help?

(Assignee)

Comment 7

a month ago

Yes, that makes it a lot clear. Thanks..!

(Reporter)

Updated

a month ago
Assignee: vihavira → nupurbaghel
(Assignee)

Comment 9

a month ago

Depends on D23468

Comment 10

a month ago
Pushed by ato@sny.no:
https://hg.mozilla.org/integration/mozilla-inbound/rev/505cad15904e
mozrunner: support macOS application bundle paths; r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/b06e155361ff
mozrunner: vendor plist crate; r=ato
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
(Assignee)

Comment 12

a month ago

Hi Ato, what happens to this patch now? What does the Resignation from Phabricator signify?

(Reporter)

Comment 13

a month ago

As I mentioned in https://phabricator.services.mozilla.com/D23468#666837,
I made some minor tweaks to get this patch to compile and then
landed it on the inbound integration tree.

Mozilla has (at least) two integration trees: inbound and autoland.
The former can be used to push patches manually, whereas the latter
is used by Phabricator/Lando. Unfortunately I’m not allowed to
make changes to other’s Phabricator reviews, so I made these changes
locally and rolled up the patches on inbound instead.

The patches landed in central on Friday, see comment #11, and this
bug is now fixed.

(Assignee)

Comment 14

a month ago

Alright, awesome :D

You need to log in before you can comment on or make changes to this bug.