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.

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

feat: download browsers as TAR #34033

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
a9b40bd
add second branch
Skn0tt Dec 16, 2024
0b139f1
move chromium, chromium-tot, and webkit to brotli
Skn0tt Dec 16, 2024
98d6138
fix import
Skn0tt Dec 16, 2024
657e435
feat: download browsers as .tar.br
Skn0tt Dec 16, 2024
c5ab0c5
respect revisionOverrides
Skn0tt Dec 18, 2024
3b72c3b
update thirdparty
Skn0tt Dec 18, 2024
411cf29
update log line matcher
Skn0tt Dec 18, 2024
d880a22
update expected error codes
Skn0tt Dec 18, 2024
f39ae50
ffmpeg also has brotli builds now
Skn0tt Dec 18, 2024
f071337
remove conflicting tar-fs types
Skn0tt Dec 18, 2024
45b84b7
add workaround for manually-created ffmpeg archive
Skn0tt Dec 18, 2024
a497b6b
remove console statement
Skn0tt Dec 18, 2024
b048d2a
Merge branch 'main' into tar-download-3rd-party-lib
Skn0tt Dec 18, 2024
affff4d
Merge branch 'main' into tar-download-3rd-party-lib
Skn0tt Dec 19, 2024
879afb1
adapt to new ffmpeg roll
Skn0tt Dec 19, 2024
16ff2df
vendor modified tar-fs
Skn0tt Jan 6, 2025
eaf6a52
update thirdpartynotices
Skn0tt Jan 6, 2025
220e28b
reset package-lock
Skn0tt Jan 6, 2025
0ccbef1
reset package.json
Skn0tt Jan 6, 2025
30c5ec6
Merge branch 'main' into tar-download-3rd-party-lib
Skn0tt Jan 13, 2025
e2b1500
firefox and chrome-headless-shell have tar now
Skn0tt Jan 13, 2025
25494cb
reuse existing code better
Skn0tt Jan 13, 2025
2224fd2
continue using close
Skn0tt Jan 13, 2025
41a552f
move to third_party
Skn0tt Jan 13, 2025
dce95a5
add license/readme
Skn0tt Jan 13, 2025
2749367
remove getStreamError
Skn0tt Jan 13, 2025
7744cc5
add diff
Skn0tt Jan 13, 2025
c717419
fix _predestroy
Skn0tt Jan 13, 2025
a2d214f
add notices
Skn0tt Jan 13, 2025
5086a56
fix linter
Skn0tt Jan 13, 2025
e24a0d7
add deps
Skn0tt Jan 13, 2025
746bb68
Merge branch 'main' into tar-download-3rd-party-lib
Skn0tt Jan 21, 2025
c8b2ce0
revert test changes
Skn0tt Jan 21, 2025
6fb5355
fix test
Skn0tt Jan 21, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/playwright-core/browsers.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@
"revision": "1011",
"installByDefault": true,
"revisionOverrides": {
"mac12": "1010",
"mac12-arm64": "1010"
"mac12": "1011",
Copy link
Member

Choose a reason for hiding this comment

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

whats the motivation for changing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

1010 doesn't have .tar.br, and 1010 is identical to 1011 in functionality

"mac12-arm64": "1011"
}
},
{
Expand Down
237 changes: 120 additions & 117 deletions packages/playwright-core/src/server/registry/index.ts

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@
import { httpRequest } from '../../utils/network';
import { ManualPromise } from '../../utils/manualPromise';
import { extract } from '../../zipBundle';
import tar from '../../utils/tar';

Check failure on line 22 in packages/playwright-core/src/server/registry/oopDownloadBrowserMain.ts

View workflow job for this annotation

GitHub Actions / docs & lint

Could not find a declaration file for module '../../utils/tar'. '/home/runner/work/playwright/playwright/packages/playwright-core/src/utils/tar/index.js' implicitly has an 'any' type.
import type http from 'http';
import { pipeline } from 'stream/promises';
import { createBrotliDecompress } from 'zlib';

export type DownloadParams = {
title: string;
Expand Down Expand Up @@ -104,11 +108,72 @@
}
}

async function throwUnexpectedResponseError(response: http.IncomingMessage) {
let body = '';
try {
await new Promise<void>((resolve, reject) => {
response
.on('data', chunk => body += chunk)
.on('end', resolve)
.on('error', reject);
});
} catch (error) {
body += error;
}

response.resume(); // consume response data to free up memory

throw new Error(`server returned code ${response.statusCode} body '${body}'`);
}

async function downloadAndExtractBrotli(options: DownloadParams) {
const response = await new Promise<http.IncomingMessage>((resolve, reject) => httpRequest({
url: options.url,
headers: {
'User-Agent': options.userAgent,
},
timeout: options.connectionTimeout,
}, resolve, reject));

log(`-- response status code: ${response.statusCode}`);
if (response.statusCode !== 200)
await throwUnexpectedResponseError(response);

const totalBytes = parseInt(response.headers['content-length'] || '0', 10);
log(`-- total bytes: ${totalBytes}`);

let downloadedBytes = 0;
response.on('data', chunk => {
downloadedBytes += chunk.length;
progress(downloadedBytes, totalBytes);
});

await pipeline(
response,
createBrotliDecompress(),
tar.extract(options.browserDirectory)
);

if (downloadedBytes !== totalBytes)
throw new Error(`size mismatch, file size: ${downloadedBytes}, expected size: ${totalBytes}`);

log(`-- download complete, size: ${downloadedBytes}`);
}

async function main(options: DownloadParams) {
await downloadFile(options);
log(`SUCCESS downloading ${options.title}`);
log(`extracting archive`);
await extract(options.zipPath, { dir: options.browserDirectory });
if (options.url.endsWith('.tar.br')) {
try {
await downloadAndExtractBrotli(options);
} catch (error) {
throw new Error(`Download failed. URL: ${options.url}`, { cause: error });
}
log(`SUCCESS downloading and extracting ${options.title}`);
} else {
await downloadFile(options);
Copy link
Member

Choose a reason for hiding this comment

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

I'm seeing different error handling code in this branch, including explicit checks for ECONNRESET. Is walking away from them intended? Should we do both changes at a time? I'd be more comfortable with leaving the download code as is and swapping piping into file with piping into broti.

Copy link
Member Author

Choose a reason for hiding this comment

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

The new branch is intended to be as similar as possible, while also making the code a little more linear. The ECONNRESET check only changed the error message, so I didn't include that.

Let me see if I can refactor it to make the change less spooky.

Copy link
Member Author

@Skn0tt Skn0tt Jan 13, 2025

Choose a reason for hiding this comment

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

I've refactored it so we can reuse the existing download function. Good pointer, thanks!

log(`SUCCESS downloading ${options.title}`);
log(`extracting archive`);
await extract(options.zipPath, { dir: options.browserDirectory });
}
if (options.executablePath) {
log(`fixing permissions at ${options.executablePath}`);
await fs.promises.chmod(options.executablePath, 0o755);
Expand Down
1 change: 1 addition & 0 deletions packages/playwright-core/src/utils/tar/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This directory contains a modified copy of the `tar-stream` library that's used exclusively to extract TAR files.
Copy link
Member

Choose a reason for hiding this comment

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

Make sure all the third party files are under the third_party folder and corresponding license files are provided beside the files. Make sure they end up in third party list or in a distributed bundle

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! See diff.patch for all my changes.

Loading
Loading