[java] publish selenium-devtools-latest alias artifact#17492
Conversation
Review Summary by QodoAdd stable selenium-devtools-latest alias artifact for latest CDP version
WalkthroughsDescription• Introduces stable selenium-devtools-latest Maven artifact tracking newest CDP version • Computes LATEST_CDP_VERSION dynamically to handle unsorted version lists • Generates CDP code and derives shim classes via sed-rewriting from latest version • Adds latest artifact to release targets and updates CDP maintenance documentation Diagramflowchart LR
versions["versions.bzl<br/>LATEST_CDP_VERSION"]
latest_build["latest/BUILD.bazel<br/>Generate & Rewrite"]
v148_build["v148/BUILD.bazel<br/>shim-sources filegroup"]
cdp_proto["CDP Protocol Files<br/>browser_protocol.json"]
generator["CdpClientGenerator<br/>with 'latest' param"]
shim_rewrite["sed-rewrite shim classes<br/>vNNN → latest"]
maven["Maven Artifact<br/>selenium-devtools-latest"]
versions -- "max CDP_VERSIONS" --> latest_build
v148_build -- "provide shim sources" --> shim_rewrite
cdp_proto -- "input" --> generator
generator -- "generate CDP code" --> latest_build
shim_rewrite -- "generate shim code" --> latest_build
latest_build -- "produces" --> maven
File Changes1. java/src/org/openqa/selenium/devtools/versions.bzl
|
| CDP_DEPS = ["//java/src/org/openqa/selenium/devtools/%s" % v for v in CDP_VERSIONS] + [ | ||
| "//java/src/org/openqa/selenium/devtools/latest", | ||
| ] |
There was a problem hiding this comment.
1. Nondeterministic cdp provider 🐞 Bug ☼ Reliability
Adding //.../devtools/latest to CDP_DEPS puts both v148CdpInfo and the generated latestCdpInfo (same major version) on the classpath, so CdpVersionFinder may return either one for an exact match depending on Set iteration order. This can cause flaky behavior and makes debugging provider selection harder because the exact CDP implementation chosen is no longer deterministic.
Agent Prompt
## Issue description
`CDP_DEPS` now includes `//java/src/org/openqa/selenium/devtools/latest`, which introduces a second `CdpInfo` provider for the same major version as the newest `vNNN` package. `CdpVersionFinder` loads all providers into a `Set` and returns the first exact match it iterates over, so the chosen provider becomes non-deterministic.
## Issue Context
- `latestCdpInfo` is generated by string-rewriting the newest `vNNN` shims, so it keeps the same `super(<major>, ...)` major version (e.g. 148).
- `CdpInfo` does not override `equals/hashCode`, so both providers coexist in the `Set`.
## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/versions.bzl[16-18]
### Suggested fix
Remove `//java/src/org/openqa/selenium/devtools/latest` from `CDP_DEPS` so the default Selenium artifacts don’t pull in both providers. Keep publishing `selenium-devtools-latest` via its own release target so downstream users can opt in explicitly.
(Alternative, if you *must* keep it in `CDP_DEPS`: make `CdpVersionFinder` deterministically resolve ties when multiple `CdpInfo` instances report the same `majorVersion`.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
c3f65cb to
a237baf
Compare
|
Persistent review updated to latest commit a237baf |
Adds a stable Maven coordinate that always tracks the newest supported CDP version. The new //java/src/org/openqa/selenium/devtools/latest target republishes the most recent vNNN package under org.openqa.selenium.devtools.latest, so downstream users no longer have to bump a version-suffixed dependency every time Chrome ships a new major release. The generated CDP code is produced by running CdpClientGenerator with "latest" against the protocol files of LATEST_CDP_VERSION, and the hand-written shim classes are derived by sed-rewriting the latest version's shim sources at build time. LATEST_CDP_VERSION is computed as max(CDP_VERSIONS) so the alias remains correct after scripts/update_cdp.py rewrites versions.bzl in place. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
a237baf to
8d568c8
Compare
|
Persistent review updated to latest commit 8d568c8 |
|
Except this will end up being the default people use even if things get out of sync. Maybe we label it @beta or call it We probably need to delete |
Users are very different. |
diemol
left a comment
There was a problem hiding this comment.
Do we still show a warning when the browser version is higher than the ones the Selenium version the user has is offering?
Yes, just checked with Selenium 4.43.0: |
diemol
left a comment
There was a problem hiding this comment.
I actually like the idea. Would be nice to add a note in the JavaDocs about the potential side effects.
|
Implementation issues, though. What if we make selenium-devtools-latest a thin alias over the current vXXX artifact rather than a self-contained copy. With this PR code as implemented, the new artifact ships its own sed-rewritten shim classes (latestCdpInfo, latestDomains, etc.) with @autoservice(CdpInfo.class). That creates a duplicate CdpInfo registration for the same major version when both selenium-devtools-latest and selenium-devtools-v are on the classpath (which is the default for selenium-java consumers, since CDP_DEPS now contains both). ServiceLoader returns both, and CdpVersionFinder.findNearestMatch exact-matches on whichever is iterated first — functionally equivalent, but observably non-deterministic and a noticeable jar-size duplication. An alternative design that I think is cleaner:
Trade-offs this resolves:
User-facing experience is identical: imports like org.openqa.selenium.devtools.latest.page.Page still work, Selenium upgrades pick up the new CDP under the same coordinate, Chrome |
|
@titusfortner Sorry, I didn't understand what means "thin alias". For the end user, the only important thing that I don't want to change this import every time: import org.openqa.selenium.devtools.v148.page.Page;Instead, I want to have the constant import: import org.openqa.selenium.devtools.latest.page.Page;How exactly it's achieved - it doesn't matter. |
|
@asolntsev I was also making it too complicated. We can just let the generator do all of the work and avoid all the problems: #17562 I built it and everything looks correct. |
|
Seems that #17562 works and fully solves the problem. |
Instead of writing
and updating "v148" part with every new release, now users can just write forever:
Similar to PR #17133, but for Java bindings.
Summary
org.seleniumhq.selenium:selenium-devtools-latestthat republishes the most recent supported CDP version under the stableorg.openqa.selenium.devtools.latestpackage. Downstream consumers no longer have to bump a version-suffixed dependency (selenium-devtools-vNNN) each time Chrome ships a new major release.CdpClientGeneratorwith"latest"against the protocol files ofLATEST_CDP_VERSION; the hand-written shim classes are derived by sed-rewriting the corresponding files from the latestvNNNpackage at build time, so nothing is duplicated on disk.LATEST_CDP_VERSIONis computed asmax(CDP_VERSIONS, key = lambda v: int(v[1:]))so the alias keeps pointing at the highest-numbered entry even afterscripts/update_cdp.pyrewritesversions.bzlin place (which can leave the list unsorted).latest/directory — onlyBUILD.bazel. Adding a new CDP version (and removing the oldest) via./go update_cdpcontinues to "just work"; theshim-sourcesfilegroup added tov148/BUILD.bazelpropagates forward throughupdate_cdp.py's copy step.Test plan
bazelisk build //java/src/org/openqa/selenium/devtools/latest:latestlatest-project.jar: contains both shim classes (latestCdpInfo,latestDomains, …) and all generated CDP domain classes (Fetch,Network,Target, …) underorg/openqa/selenium/devtools/latest/, plus aMETA-INF/services/org.openqa.selenium.devtools.CdpInfoentry registeringlatestCdpInfo.bazelisk build //java/src/org/openqa/selenium:client-combined //java/src/org/openqa/selenium/grid:grid— existing consumers ofCDP_DEPSstill build../go format🤖 Generated with Claude Code