New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: always use ENV executable path when present #7985
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
// Chromium builds are available. | ||
if (os.platform() !== 'darwin' && os.arch() === 'arm64') { | ||
chromeExecutable = '/usr/bin/chromium-browser'; | ||
} else { | ||
const { missingText, executablePath } = resolveExecutablePath(this); | ||
if (missingText) throw new Error(missingText); | ||
chromeExecutable = executablePath; | ||
} | ||
const { missingText, executablePath } = resolveExecutablePath(this); | ||
if (missingText) throw new Error(missingText); | ||
chromeExecutable = executablePath; | ||
} | ||
|
||
if (!chromeExecutable) { | ||
|
@@ -799,9 +793,11 @@ function resolveExecutablePath(launcher: ChromeLauncher | FirefoxLauncher): { | |
executablePath: string; | ||
missingText?: string; | ||
} { | ||
const { product, _isPuppeteerCore, _projectRoot, _preferredRevision } = | ||
launcher; | ||
let downloadPath: string | undefined; | ||
// puppeteer-core doesn't take into account PUPPETEER_* env variables. | ||
if (!launcher._isPuppeteerCore) { | ||
if (!_isPuppeteerCore) { | ||
const executablePath = | ||
process.env.PUPPETEER_EXECUTABLE_PATH || | ||
process.env.npm_config_puppeteer_executable_path || | ||
|
@@ -813,22 +809,31 @@ function resolveExecutablePath(launcher: ChromeLauncher | FirefoxLauncher): { | |
: undefined; | ||
return { executablePath, missingText }; | ||
} | ||
const ubuntuChromiumPath = '/usr/bin/chromium-browser'; | ||
if ( | ||
product === 'chrome' && | ||
os.platform() !== 'darwin' && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was tempting to change this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I've updated this to retain the |
||
os.arch() === 'arm64' && | ||
fs.existsSync(ubuntuChromiumPath) | ||
) { | ||
return { executablePath: ubuntuChromiumPath, missingText: undefined }; | ||
} | ||
downloadPath = | ||
process.env.PUPPETEER_DOWNLOAD_PATH || | ||
process.env.npm_config_puppeteer_download_path || | ||
process.env.npm_package_config_puppeteer_download_path; | ||
} | ||
if (!launcher._projectRoot) { | ||
if (!_projectRoot) { | ||
throw new Error( | ||
'_projectRoot is undefined. Unable to create a BrowserFetcher.' | ||
); | ||
} | ||
const browserFetcher = new BrowserFetcher(launcher._projectRoot, { | ||
product: launcher.product, | ||
const browserFetcher = new BrowserFetcher(_projectRoot, { | ||
product: product, | ||
path: downloadPath, | ||
}); | ||
|
||
if (!launcher._isPuppeteerCore && launcher.product === 'chrome') { | ||
if (!_isPuppeteerCore && product === 'chrome') { | ||
const revision = process.env['PUPPETEER_CHROMIUM_REVISION']; | ||
if (revision) { | ||
const revisionInfo = browserFetcher.revisionInfo(revision); | ||
|
@@ -839,13 +844,13 @@ function resolveExecutablePath(launcher: ChromeLauncher | FirefoxLauncher): { | |
return { executablePath: revisionInfo.executablePath, missingText }; | ||
} | ||
} | ||
const revisionInfo = browserFetcher.revisionInfo(launcher._preferredRevision); | ||
const revisionInfo = browserFetcher.revisionInfo(_preferredRevision); | ||
|
||
const firefoxHelp = `Run \`PUPPETEER_PRODUCT=firefox npm install\` to download a supported Firefox browser binary.`; | ||
const chromeHelp = `Run \`npm install\` to download the correct Chromium revision (${launcher._preferredRevision}).`; | ||
const missingText = !revisionInfo.local | ||
? `Could not find expected browser (${launcher.product}) locally. ${ | ||
launcher.product === 'chrome' ? chromeHelp : firefoxHelp | ||
? `Could not find expected browser (${product}) locally. ${ | ||
product === 'chrome' ? chromeHelp : firefoxHelp | ||
}` | ||
: undefined; | ||
return { executablePath: revisionInfo.executablePath, missingText }; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks it was introduced in #7099