Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: always use ENV executable path when present #7985

Merged
merged 1 commit into from Feb 11, 2022
Merged

fix: always use ENV executable path when present #7985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 22 additions & 17 deletions src/node/Launcher.ts
View file
Open in desktop
Expand Up @@ -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
Copy link
Contributor Author

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'

Copy link
Collaborator

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

// 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) {
Expand Down Expand Up @@ -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 ||
Expand All @@ -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' &&
Copy link
Contributor Author

@jbielick jbielick Feb 9, 2022 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jbielick jbielick Feb 9, 2022 •
edited

Choose a reason for hiding this comment

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

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.

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);
Expand All @@ -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 };
Expand Down
82 changes: 82 additions & 0 deletions test/launcher.spec.ts
View file
Open in desktop
Expand Up @@ -733,6 +733,88 @@ describe('Launcher specs', function () {
const executablePath = puppeteer.executablePath('chrome');
expect(executablePath).toBeTruthy();
});
describe('when PUPPETEER_EXECUTABLE_PATH is set', () => {
const sandbox = sinon.createSandbox();

beforeEach(() => {
process.env.PUPPETEER_EXECUTABLE_PATH = '';
sandbox
.stub(process.env, 'PUPPETEER_EXECUTABLE_PATH')
.value('SOME_CUSTOM_EXECUTABLE');
});

afterEach(() => sandbox.restore());

it('its value is returned', async () => {
const { puppeteer } = getTestState();

const executablePath = puppeteer.executablePath();

expect(executablePath).toEqual('SOME_CUSTOM_EXECUTABLE');
});
});

describe('when the product is chrome, platform is not darwin, and arch is arm64', () => {
describe('and the executable exists', () => {
itChromeOnly('returns /usr/bin/chromium-browser', async () => {
const { puppeteer } = getTestState();
const osPlatformStub = sinon.stub(os, 'platform').returns('linux');
const osArchStub = sinon.stub(os, 'arch').returns('arm64');
const fsExistsStub = sinon.stub(fs, 'existsSync');
fsExistsStub.withArgs('/usr/bin/chromium-browser').returns(true);

const executablePath = puppeteer.executablePath();

expect(executablePath).toEqual('/usr/bin/chromium-browser');

osPlatformStub.restore();
osArchStub.restore();
fsExistsStub.restore();
});
describe('and PUPPETEER_EXECUTABLE_PATH is set', () => {
const sandbox = sinon.createSandbox();

beforeEach(() => {
process.env.PUPPETEER_EXECUTABLE_PATH = '';
sandbox
.stub(process.env, 'PUPPETEER_EXECUTABLE_PATH')
.value('SOME_CUSTOM_EXECUTABLE');
});

afterEach(() => sandbox.restore());

it('its value is returned', async () => {
const { puppeteer } = getTestState();

const executablePath = puppeteer.executablePath();

expect(executablePath).toEqual('SOME_CUSTOM_EXECUTABLE');
});
});
});
describe('and the executable does not exist', () => {
itChromeOnly(
'does not return /usr/bin/chromium-browser',
async () => {
const { puppeteer } = getTestState();
const osPlatformStub = sinon
.stub(os, 'platform')
.returns('linux');
const osArchStub = sinon.stub(os, 'arch').returns('arm64');
const fsExistsStub = sinon.stub(fs, 'existsSync');
fsExistsStub.withArgs('/usr/bin/chromium-browser').returns(false);

const executablePath = puppeteer.executablePath();

expect(executablePath).not.toEqual('/usr/bin/chromium-browser');

osPlatformStub.restore();
osArchStub.restore();
fsExistsStub.restore();
}
);
});
});
});
});

Expand Down