Closed Bug 1529278 Opened 2 years ago Closed 1 year ago

Support for macOS application bundle paths

Categories

(Testing :: Mozbase Rust, enhancement)

Version 3
x86_64
macOS
enhancement
Not set
normal

Tracking

(firefox67 fixed)

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: ato, Assigned: nupurbaghel)

Details

(Keywords: good-first-bug)

Attachments

(2 files)

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.

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.

Would I be able to work on this one?

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

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?

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?

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

Assignee: vihavira → nupurbaghel

Depends on D23468

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
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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

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.

Alright, awesome :D

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