Skip to content

[test] Fix get_firefox_version() on macOS Homebrew Cask installs - #28194

Open
dschuff wants to merge 2 commits into
emscripten-core:mainfrom
dschuff:firefox
Open

dschuff wants to merge 2 commits into
emscripten-core:mainfrom
dschuff:firefox

Conversation

@dschuff

@dschuff dschuff commented Nov 6, 2026 •

Copy link
Copy Markdown
Member

When Firefox is installed via Homebrew Cask on macOS (such as on GitHub Actions macos-15 runners), shutil.which('firefox') returns a wrapper shim at /opt/homebrew/bin/firefox while the app bundle lives in /Applications/Firefox.app. Resolve symlinks on exe_path, search standard macOS Firefox.app locations alongside the existing Linux /usr/lib/firefox* directories when platform.ini is not adjacent to the executable, and fall back to OLDEST_SUPPORTED_FIREFOX with a warning on all platforms instead of raising FileNotFoundError.

When Firefox is installed via Homebrew Cask on macOS (such as on GitHub Actions macos-15 runners), shutil.which('firefox') returns a wrapper shim at /opt/homebrew/bin/firefox while the app bundle lives in /Applications/Firefox.app. Resolve symlinks on exe_path, search standard macOS Firefox.app locations alongside the existing Linux /usr/lib/firefox* directories when platform.ini is not adjacent to the executable, and fall back to OLDEST_SUPPORTED_FIREFOX with a warning on all platforms instead of raising FileNotFoundError.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates test/browser_common.py to improve Firefox version detection. It resolves the real path of the browser executable and searches standard installation directories on Linux and macOS for platform.ini if it is not found next to the executable, which helps support wrapper scripts and Homebrew Cask shims. There are no review comments, so I have no feedback to provide.

@dschuff

dschuff commented Nov 6, 2026 •

Copy link
Copy Markdown
Member Author

@sbc100 (or @juj ) I'm curious if we ever do firefox tests on Mac?
In testing the GHA mac runners this issue caused a crash on test runner startup, but we don't actually need to fix it if we just set EMTEST_BROWSER=0 on those runners (which I figured I would because we don't run browser tests on Mac on CI).
So we can land this if it seems potentially useful, or not.

Comment thread test/browser_common.py Resolved
Comment thread test/browser_common.py Resolved
@juj

juj commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator

@sbc100 (or @juj ) I'm curious if we ever do firefox tests on Mac?

I run full firefox test suite on multiple Macs on my CI. Though I use emsdk install firefox-*-64bit to acquire firefox, and not homebrew. So the lookup can result in different rules. LGTM on this in general.

@sbc100 sbc100 changed the title Fix get_firefox_version() on macOS Homebrew Cask installs [test] Fix get_firefox_version() on macOS Homebrew Cask installs Nov 6, 2026
Comment thread test/browser_common.py Resolved

@sbc100 sbc100 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm if juj is ok with it

- Restore prefix guards (/usr/bin/ on Linux and /opt/homebrew/ or /usr/local/ on macOS) so custom EMTEST_BROWSER paths do not fall back to unrelated system Firefox installs.

- Unify candidate platform.ini lookup into a single search_dirs loop and split out the inline MACOS conditional.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants