- Notifications
You must be signed in to change notification settings - Fork4.4k
feat(webkit): allow running WebKit via WSL on Windows#36358
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
10a64dd to34667d2Compare This comment has been minimized.
This comment has been minimized.
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.
This is very clean. I like it.
| socket.on('error', reject); | ||
| }); | ||
| const [executable, ...args] = process.argv.slice(2); |
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.
Even though this is run by us, this should have proper error handling.
| env: { | ||
| ...this.amendEnvironment(env, userDataDir, executable, browserArguments, options.channel), | ||
| "WSLENV": "SOCKET_ADDRESS", | ||
| 'SOCKET_ADDRESS': (transportServer?.address() as any)?.port?.toString() ?? '', |
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.
What is our goal here if we don't have a transport?
bae0c10 to18c9ba8Compareba3bd42 to48251d4Compare48251d4 to62bd7f5Compare| override amendEnvironment(env: Env, userDataDir: string, executable: string, browserArguments: string[], channel?: string): Env { | ||
| return { | ||
| ...env, | ||
| // CURL_COOKIE_JAR_PATH: path.join(channel === 'webkit-wsl' ? translatePathToWSL(userDataDir) : userDataDir, 'cookiejar.db'), |
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.
if condition.
62bd7f5 toe8c3971Compare| } | ||
| export type LaunchLifecycleHooks = { | ||
| preLaunch(): Promise<void>; |
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.
This could be an abstract method onBrowserType.
| } | ||
| export type LaunchLifecycleHooks = { | ||
| preLaunch(): Promise<void>; | ||
| onExit(): Promise<void>; |
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.
Probably an abstract method as well.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| readPipe(): NodeJS.ReadableStream; | ||
| writePipe(): NodeJS.WritableStream; | ||
| rewriteArgs(args: string[]): string[]; | ||
| rewriteExecutable(): string; |
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.
An abstract method.
| readPipe(): NodeJS.ReadableStream; | ||
| writePipe(): NodeJS.WritableStream; |
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.
For these, perhaps we can docreateTransport() that will combine these pipes andwaitForReadyState() together.
| rewriteExecutable: () => 'wsl', | ||
| rewriteArgs: (args: string[]) => { | ||
| const executablePath = registry.findExecutable('webkit-wsl')!.executablePathOrDie('node'); | ||
| return [ | ||
| '-d', | ||
| 'playwright', | ||
| '--cd', | ||
| '/home/pwuser', | ||
| '/home/pwuser/node/bin/node', | ||
| '/home/pwuser/webkit-wsl-pipe-wrapper.mjs', | ||
| executablePath, | ||
| ...args, | ||
| ]; | ||
| }, | ||
| }; |
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.
Can we replace this with something similar topw_run.sh?
71966c1 toba2aceaCompare This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"8 failed 18 flaky3753 passed, 430 skipped Mergeworkflow run. |
No description provided.