-
Notifications
You must be signed in to change notification settings - Fork 81
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
Rollback PDL to stable and update fetcher to match #245
Conversation
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.
lgtm!
any additional things that should go in?
otherwise I'd do a new release with this
// Check if the chosen revision has a build available for all platforms. | ||
// That not always the case, that is why we need to make sure of it. |
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.
ty
// The chromium revision is hard to get right and the relation to the CDP revision | ||
// even more so, so here are some guidances. | ||
// | ||
// We used to use the revision of Puppeteer, but they switched to chrome-for-testing. | ||
// This means we have to check things ourself. The chromium revision should at least | ||
// as great as the CDP revision otherwise they won't be compatible. | ||
// Not all revisions of chromium have builds for all platforms. | ||
// | ||
// This is essentially a bruteforce process. You can use the test `find_revision_available` | ||
// to find a revision that is available for all platforms. We recommend setting the `min` | ||
// to the current CDP revision and the max to max revision of stable chromium. | ||
// See https://chromiumdash.appspot.com/releases for the latest stable revision. | ||
// | ||
// In general, we should also try to ship as close as a stable version of chromium if possible. | ||
// The CDP should also be a bit older than that stable version. | ||
// To map a revision to a chromium version you can use the site https://chromiumdash.appspot.com/commits. | ||
|
||
/// Currently downloaded chromium revision |
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.
ty!
I will do a rewrite tomorrow and some testing. If you can do a pass same time tomorrow everything should be ready. |
Couple of important changes: