1529278 - Support for macOS application bundle paths

Status: RESOLVED FIXED

(Testing :: Mozbase Rust, enhancement)

Description

[Andreas Tolfsen ❲:ato❳]

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.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

For reference mozinstall which is written in Python uses this technique to find the binary:
https://searchfox.org/mozilla-central/rev/93905b660fc99a5d52b683690dd26471daca08c8/testing/mozbase/mozinstall/mozinstall/mozinstall.py#69-77

Comment 3

[vihavira]

Would I be able to work on this one?

Comment 4

[Andreas Tolfsen ❲:ato❳]

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.

Comment 5

[Nupur Baghel]

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?

Comment 6

[Andreas Tolfsen ❲:ato❳]

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?

Comment 7

[Nupur Baghel]

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

Comment 8

[Nupur Baghel]

Attachment: Bug 1529278 - Add support for macOS application bundle paths. r=ato

Comment 9

[Nupur Baghel]

Attachment: Bug 1529278 - Add third_party crates due to plist. r=ato

Depends on D23468

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/505cad15904e
https://hg.mozilla.org/mozilla-central/rev/b06e155361ff

Comment 12

[Nupur Baghel]

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

Comment 13

[Andreas Tolfsen ❲:ato❳]

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.

Comment 14

[Nupur Baghel]

Alright, awesome :D