[build] Support os architecture in pinned chrome browser#17268
Open
nvborisenko wants to merge 1 commit intoSeleniumHQ:trunkfrom
Open
[build] Support os architecture in pinned chrome browser#17268nvborisenko wants to merge 1 commit intoSeleniumHQ:trunkfrom
nvborisenko wants to merge 1 commit intoSeleniumHQ:trunkfrom
Conversation
Contributor
Review Summary by Qodo
WalkthroughsDescription• Add architecture-aware Chrome/Chromedriver binary pinning for macOS • Support both x64 and arm64 architectures with conditional fetching • Update pin_browsers function to accept architecture parameter • Pass detected OS architecture from build context to browser pinning File Changes1. common/repositories.bzl
|
Contributor
Code Review by Qodo
1. pin_browsers() signature changed
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| load("//common/private:pkg_archive.bzl", "pkg_archive") | ||
|
|
||
| def pin_browsers(): | ||
| def pin_browsers(arch): |
Contributor
There was a problem hiding this comment.
1. pin_browsers() signature changed 📘 Rule violation ✓ Correctness
The public Starlark symbol pin_browsers was changed from no-args to requiring arch, which can break any external loads/callers still invoking pin_browsers() with the previous signature.
Agent Prompt
## Issue description
`pin_browsers` changed from no-arg to requiring `arch`, which can break existing Bazel/Starlark callers that still invoke `pin_browsers()`.
## Issue Context
This is a build/module interface surface (`common/repositories.bzl`). Even if this repo currently calls it only from the module extension, external consumers may load and call it.
## Fix Focus Areas
- common/repositories.bzl[9-16]
- common/repositories.bzl[445-446]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+9
to
+16
| def pin_browsers(arch): | ||
| local_drivers(name = "local_drivers") | ||
|
|
||
| if arch == "x86_64": | ||
| chrome_arch = "x64" | ||
| else: | ||
| chrome_arch = "arm64" | ||
|
|
Contributor
There was a problem hiding this comment.
2. Generator not updated 🐞 Bug ⚙ Maintainability
common/repositories.bzl is generated by scripts/pinned_browsers.py, but this PR changes the generated output shape (pin_browsers now takes arch and extension passes _ctx.os.arch) without updating the generator, so regenerating will overwrite and remove the arch-aware pinning.
Agent Prompt
## Issue description
`common/repositories.bzl` is generated, but the generator (`scripts/pinned_browsers.py`) still generates the old `pin_browsers()` signature and extension call. Re-running `bazel run scripts:pinned_browsers` will overwrite the PR’s new arch-aware Mac Chrome/Chromedriver pinning.
## Issue Context
The repo relies on `scripts/pinned_browsers.py` to regenerate `common/repositories.bzl` when pinned versions change.
## Fix Focus Areas
- scripts/pinned_browsers.py[537-570]
- common/repositories.bzl[1-16]
- common/repositories.bzl[445-446]
## What to change
- Teach `scripts/pinned_browsers.py` to generate `def pin_browsers(arch):` and emit the `chrome_arch` mapping logic.
- Update the generated extension impl to call `pin_browsers(arch = _ctx.os.arch)`.
- Ensure the generated output matches the committed `common/repositories.bzl` so regeneration is idempotent.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Now I can use Mac
x64. Build system doesn't requirearm.💥 What does this PR do?
This pull request updates the way Chrome and Chromedriver binaries are pinned for Mac in the
common/repositories.bzlfile, making the process architecture-aware. The changes ensure that the correct binaries are fetched for either x86_64 or arm64 architectures, improving compatibility and reliability for Mac users on different hardware.Architecture-aware browser pinning:
pin_browsersfunction to accept anarchparameter and determine the correct Chrome architecture (x64forx86_64andarm64otherwise).pin_browsers_extensionimplementation to pass the detected architecture from the build context topin_browsers.Conditional fetching of Mac Chrome/Chromedriver binaries:
mac_chrome) forx64orarm64, including patch commands and build file setup.mac_chromedriver).mac_beta_chrome) and Chromedriver (mac_beta_chromedriver). [1] [2]💡 Additional Considerations
Only chrome driver, enough for me at this time.
🔄 Types of changes