Skip to content

Commit

Permalink
fix(installer): retain browsers installed via Playwrigth CLI (#5904)
Browse files Browse the repository at this point in the history
Browser registry is responsible for 3 things:
1. Remove downloaded browsers if there are no packages that refer to them
2. Install default browsers needed for the current package
3. Install browsers on-demand when used through Playwright CLI

Currently, registry relies on a single "download" field in `browsers.json`
to carry both (1) and (2). However, browsers in (3) are marked as
`download: false` so that they aren't installed automatically in (2), so
auto-remove procedure in (1) removes them on subsequent installation.

One possible approach to fix this would be modifying package's `browsers.json` to
change `download: false` to `true` when browsers are installed with
Playwright CLI. This approach was explored here:
bc04a51

We decided against this since we have a history of issues related to
package modifications after NPM installation. This breaks all
sorts of yarn/npm caching mechanisms.

Instead, this patch is a two-step refactor:
- remove the "download" field in `browsers.json`. Now, all registries
(including old ones from previously-released versions) will retain any
browsers that are mentioned in the `browsers.json`.
- add a new flag "installByDefault", that is **only used** for default
installation.

With this change, the registry tasks are done like this:
- (1) auto-removal: if browser has a back reference, it is retained,
otherwise it is removed from registry
- (2) default installation: use only `installByDefault` to carry default installations
- (3) CLI installation: simply installs browsers. Since we retain
everythings that's referenced in (1), browsers aren't removed.

Fixes #5902
  • Loading branch information
aslushnikov authored Mar 22, 2021
1 parent 6dd4d75 commit 2064d27
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 16 deletions.
8 changes: 4 additions & 4 deletions browsers.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,25 @@
{
"name": "chromium",
"revision": "857950",
"download": true
"installByDefault": true
},
{
"name": "firefox",
"revision": "1238",
"download": true
"installByDefault": true
},
{
"name": "webkit",
"revision": "1446",
"download": true,
"installByDefault": true,
"revisionOverrides": {
"mac10.14": "1443"
}
},
{
"name": "ffmpeg",
"revision": "1005",
"download": true
"installByDefault": true
}
]
}
2 changes: 1 addition & 1 deletion packages/build_package.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ if (!args.some(arg => arg === '--no-cleanup')) {
// 5. Generate browsers.json
const browsersJSON = require(path.join(ROOT_PATH, 'browsers.json'));
for (const browser of browsersJSON.browsers)
browser.download = package.browsers.includes(browser.name);
browser.installByDefault = package.browsers.includes(browser.name);
await writeToPackage('browsers.json', JSON.stringify(browsersJSON, null, 2));

// 6. Bake commit SHA into the package
Expand Down
8 changes: 3 additions & 5 deletions src/install/installer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const fsWriteFileAsync = util.promisify(fs.writeFile.bind(fs));

const PACKAGE_PATH = path.join(__dirname, '..', '..');

export async function installBrowsersWithProgressBar(browserNames: BrowserName[] = allBrowserNames) {
export async function installBrowsersWithProgressBar(browserNames: BrowserName[] = Registry.currentPackageRegistry().installByDefault()) {
// PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD should have a value of 0 or 1
if (getAsBooleanFromENV('PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD')) {
browserFetcher.logPolitely('Skipping browsers download because `PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD` env variable is set');
Expand Down Expand Up @@ -74,7 +74,7 @@ async function validateCache(linksDir: string, browserNames: BrowserName[]) {
linkTarget = (await fsReadFileAsync(linkPath)).toString();
const linkRegistry = new Registry(linkTarget);
for (const browserName of allBrowserNames) {
if (!linkRegistry.shouldDownload(browserName))
if (!linkRegistry.shouldRetain(browserName))
continue;
const usedBrowserPath = linkRegistry.browserDirectory(browserName);
const browserRevision = linkRegistry.revision(browserName);
Expand Down Expand Up @@ -105,10 +105,8 @@ async function validateCache(linksDir: string, browserNames: BrowserName[]) {
}

// 3. Install missing browsers for this package.
const myRegistry = new Registry(PACKAGE_PATH);
const myRegistry = Registry.currentPackageRegistry();
for (const browserName of browserNames) {
if (!myRegistry.shouldDownload(browserName))
continue;
await browserFetcher.downloadBrowserWithProgressBar(myRegistry, browserName).catch(e => {
throw new Error(`Failed to download ${browserName}, caused by\n${e.stack}`);
});
Expand Down
29 changes: 23 additions & 6 deletions src/utils/registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,13 @@ import { assert, getFromENV } from './utils';
export type BrowserName = 'chromium'|'webkit'|'firefox'|'ffmpeg';
export const allBrowserNames: BrowserName[] = ['chromium', 'webkit', 'firefox', 'ffmpeg'];

const PACKAGE_PATH = path.join(__dirname, '..', '..');

type BrowserPlatform = 'win32'|'win64'|'mac10.13'|'mac10.14'|'mac10.15'|'mac11'|'mac11-arm64'|'ubuntu18.04'|'ubuntu20.04';
type BrowserDescriptor = {
name: BrowserName,
revision: string,
download: boolean,
installByDefault: boolean,
browserDirectory: string,
};

Expand Down Expand Up @@ -199,9 +201,17 @@ export function isBrowserDirectory(browserDirectory: string): boolean {
return false;
}

let currentPackageRegistry: Registry | undefined = undefined;

export class Registry {
private _descriptors: BrowserDescriptor[];

static currentPackageRegistry() {
if (!currentPackageRegistry)
currentPackageRegistry = new Registry(PACKAGE_PATH);
return currentPackageRegistry;
}

constructor(packagePath: string) {
const browsersJSON = JSON.parse(fs.readFileSync(path.join(packagePath, 'browsers.json'), 'utf8'));
this._descriptors = browsersJSON['browsers'].map((obj: any) => {
Expand All @@ -212,7 +222,7 @@ export class Registry {
return {
name,
revision,
download: obj.download,
installByDefault: !!obj.installByDefault,
browserDirectory,
};
});
Expand Down Expand Up @@ -283,10 +293,17 @@ export class Registry {
return util.format(urlTemplate, downloadHost, browser.revision);
}

shouldDownload(browserName: BrowserName): boolean {
// Older versions do not have "download" field. We assume they need all browsers
// from the list. So we want to skip all browsers that are explicitly marked as "download: false".
shouldRetain(browserName: BrowserName): boolean {
// We retain browsers if they are found in the descriptor.
// Note, however, that there are older versions out in the wild that rely on
// the "download" field in the browser descriptor and use its value
// to retain and download browsers.
// As of v1.10, we decided to abandon "download" field.
const browser = this._descriptors.find(browser => browser.name === browserName);
return !!browser && browser.download !== false;
return !!browser;
}

installByDefault(): BrowserName[] {
return this._descriptors.filter(browser => browser.installByDefault).map(browser => browser.name);
}
}

0 comments on commit 2064d27

Please sign in to comment.