fix: always use ENV executable path when present #7985
Conversation
| @@ -146,15 +146,9 @@ class ChromeLauncher implements ProductLauncher { | |||
|
|
|||
| chromeExecutable = executablePathForChannel(channel); | |||
| } else if (!executablePath) { | |||
| // Use Intel x86 builds on Apple M1 until native macOS arm64 | |||
There was a problem hiding this comment.
I might have missed the point, but this comment seems outdated because /usr/bin/chromium-browser isn't always an x86 build and the comment does not match the condition !== 'darwin'
There was a problem hiding this comment.
It looks it was introduced in #7099
| const ubuntuChromiumPath = '/usr/bin/chromium-browser'; | ||
| if ( | ||
| product === 'chrome' && | ||
| os.platform() !== 'darwin' && |
There was a problem hiding this comment.
It was tempting to change this to os.platform() === 'linux' because /usr/bin/chromium-browser seemed only applicable to Ubuntu from what I could tell APK (alpine) also uses this location.
There was a problem hiding this comment.
It didn't seem like os.arch() === 'arm64' should be retained anymore. Whether chromium-browser amd64 or arm64 build is installed it will be in the same location. Should it be retained so that it only behaves this way on arm64 systems because that's the way it used to behave? The current conditions here would add preference to a pre-installed /usr/bin/chromium-browser instead of downloading one on non-darwin platforms.
I've updated this to retain the os.arch() === 'arm64' so no breaking change would occur in which a pre-existing chromium installation would take automatic precedence over the normally downloaded chromium.
jbielick
force-pushed
the
executable-path-env-precedence
branch
from
e46fa4f to
eef8172
Compare
OrKoN
force-pushed
the
executable-path-env-precedence
branch
from
eef8172 to
e737705
Compare
jbielick
force-pushed
the
executable-path-env-precedence
branch
from
e737705 to
98844c4
Compare
|
Seeing the firefox test fail leads me to believe a couple of these tests should have |
OrKoN
force-pushed
the
executable-path-env-precedence
branch
2 times, most recently
from
cbe8cdb to
ceeebce
Compare
Some recent changes to allow arm64 environments (including M1 macs) to launch a chromium installation successfully before arm-compatible builds were downloadable prevented the usage of PUPPETEER_EXECUTABLE_PATH in some environments. Currently, when the platform is not darwin and the arch is arm64, an executable cannot be specified using the environment variable. Generally speaking, environment variables have highest precedence for options such as this since they depend on system configuration. These change: 1. allow the ENV variable to always be used when defined and not specified in LaunchOptions (and when not puppeteer-core) 2. Retain the existing behavior of assuming /usr/bin/chromium-browser on platforms like Ubuntu (exact if-conditions preserved to avoid any breaking changes) 3. Add some tests for this particular portion of the code.
OrKoN
force-pushed
the
executable-path-env-precedence
branch
from
ceeebce to
599dc96
Compare
|
LGTM (requesting a second review from @jrandolf) |
What kind of change does this PR introduce?
Fixes #7981
These changes:
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
No. I could add a note about precedence if desired.
Summary
#7981
On linux/arm64, puppeteer always assumes /usr/bin/chromium-browser exists. It ignores
PUPPETEER_EXECUTABLE_PATHand fails when /usr/bin/chromium-browser doesn't exist. Not all linux distros install to /usr/bin/chromium-browser, so it would be better to allow PUPPETEER_EXECUTABLE_PATHto have precedence. Debian for instance installs to /usr/bin/chromium. I can't launch chromium unless I explicitly pass an executable path inLaunchOptions`.Some recent changes to allow arm64 environments (including M1 macs) to launch a chromium installation successfully before arm-compatible builds were downloadable prevented the usage of PUPPETEER_EXECUTABLE_PATH in some environments. Currently, when the platform is not darwin and the arch is arm64, an executable cannot be specified using the environment variable.
Generally speaking, environment variables have highest precedence for options such as this since they depend on system configuration.
Does this PR introduce a breaking change?
No. An executablePath provided via LaunchOptions always gets used. If empty, the ENV var is used. The assumed default executable of
/usr/bin/chromium-browseris retained in case its being relied on (though, if PUPPETEER_EXECUTABLE_PATH is also set, it would start working differently). The assumed scope ofif not darwin and arm64was too broad, so this narrows it./usr/bin/chromium-browserwould be present if the OS was Ubuntu, but not Debian. I presume Ubuntu is more common so it was used.