-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
10a64dd
to
34667d2
Compare
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.
bae0c10
to
18c9ba8
Compare
ba3bd42
to
48251d4
Compare
48251d4
to
62bd7f5
Compare
62bd7f5
to
e8c3971
Compare
71966c1
to
ba2acea
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3bebff5
to
add6387
Compare
This comment has been minimized.
This comment has been minimized.
add6387
to
5e6d1dd
Compare
This comment has been minimized.
This comment has been minimized.
5e6d1dd
to
a572b25
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
7f35898
to
b9d96dd
Compare
This comment has been minimized.
This comment has been minimized.
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.
looks great! there's a few places where comments could help, but otherwise I only have nits.
const [executable, ...args] = process.argv.slice(2); | ||
|
||
if (!(await fs.promises.stat(executable)).isFile()) | ||
throw new Error(`Executable does not exist. Did you update Playwright recently? Make sure to run npx playwright install webkit-wsl`); |
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.
Hmm, can we throw the exact error somewhere in a parent? We try to detect the package manager, so npx
isn't necessarily accurate.
8d9d0ca
to
e0ddad3
Compare
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.
The implementation looks mostly good, but I got tired halfway through the tests. It seems like we should address those tests that do not work and minimize changes that should be done by the user.
@@ -98,3 +108,8 @@ export class WebKit extends BrowserType { | |||
return webkitArguments; | |||
} | |||
} | |||
|
|||
export async function translatePathToWSL(path: string): Promise<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.
As discussed before, perhaps do this once, save somewhere on WKBrowser
and then just path.join()
when needed?
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.
The input path
is from the user, could be from a different drive, network file share, be a symlink etc. so I don't think any custom path join logic would be bug-free looking at their impl - lets keep the async call for now and iterate later on it if needed?
|
||
child.on('close', (code, signal) => { | ||
log('Child exit', { code, signal }); | ||
shutdown(code ?? (signal ? 0 : 0)); |
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.
Shouldn't this be non-zero when signal
is present?
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.
Done, ptal.
parentOut.destroy(); | ||
|
||
// Close listener and destroy any sockets | ||
await new Promise(resolve => server.close(() => resolve(null))); |
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 think the pattern should be:
- call
server.close()
- destroy sockets
- wait for
server.close
callback
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.
Done, ptal.
}); | ||
})().catch(error => { | ||
console.error('Error occurred:', error); | ||
process.exit(1); |
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 try to preserve the child
s error code?
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.
The child's exit code is already preserved on lines 67-70. The process.exit(1)
in the catch block is only for setup errors that happen before the child process starts (like missing env vars, file not found, socket connection issues). Those aren't child exit codes since the child hasn't run yet.
e0ddad3
to
763eddd
Compare
This comment has been minimized.
This comment has been minimized.
packages/playwright-core/src/server/webkit/wsl/webkit-wsl-transport-client.ts
Show resolved
Hide resolved
packages/playwright-core/src/server/webkit/wsl/webkit-wsl-transport-client.ts
Outdated
Show resolved
Hide resolved
packages/playwright-core/src/server/webkit/wsl/webkit-wsl-transport-client.ts
Outdated
Show resolved
Hide resolved
763eddd
to
539a7cd
Compare
This comment has been minimized.
This comment has been minimized.
539a7cd
to
294493a
Compare
This comment has been minimized.
This comment has been minimized.
0f3c524
to
6887a77
Compare
This comment has been minimized.
This comment has been minimized.
packages/playwright-core/src/server/webkit/wsl/webkit-wsl-transport-server.ts
Show resolved
Hide resolved
packages/playwright-core/src/server/webkit/wsl/webkit-wsl-transport-server.ts
Show resolved
Hide resolved
packages/playwright-core/src/server/webkit/wsl/webkit-wsl-transport-server.ts
Show resolved
Hide resolved
packages/playwright-core/src/server/webkit/wsl/webkit-wsl-transport-server.ts
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"1 failed 3 flaky46774 passed, 821 skipped Merge workflow run. |
This PR adds support for running WebKit in a dedicated WSL environment (
webkit-wsl
) on Windows. It introduces a transport layer to proxy browser communication between Windows and WSL, along with a PowerShell script to install and configure a WSL distro with the necessary WebKit setup.#37036