-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
chore: move winldd to CDN #34078
chore: move winldd to CDN #34078
Conversation
667732e
to
75aaf0e
Compare
This comment has been minimized.
This comment has been minimized.
@@ -1315,6 +1315,8 @@ export async function installBrowsersForNpmInstall(browsers: string[]) { | |||
return false; | |||
} | |||
const executables: Executable[] = []; | |||
if (process.platform === 'win32') | |||
executables.push(registry.findExecutable('winldd')!); |
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.
why do we have this for winldd but not for ffmpeg?
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 function is used for the browser-* packages which will install the browsers during the NPM install. We specify ffmpeg
there every time on the caller side - we don't want to do that with winldd
since its based on the process.platform
so I moved it one layer deeper.
This comment has been minimized.
This comment has been minimized.
8da07c7
to
962e2f2
Compare
This comment has been minimized.
This comment has been minimized.
962e2f2
to
1c6a9a1
Compare
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"6 flaky37391 passed, 650 skipped Merge workflow run. |
Fixes #34071