Skip to content

[build] clarify dependency pin and update tasks#17463

Merged
titusfortner merged 2 commits into
trunkfrom
deps
May 18, 2026
Merged

[build] clarify dependency pin and update tasks#17463
titusfortner merged 2 commits into
trunkfrom
deps

Conversation

@titusfortner
Copy link
Copy Markdown
Member

@titusfortner titusfortner commented May 15, 2026

This fixes the distinctions between the 3 functions of dependency management

  • pin: regenerate lockfiles, checksums, or Bazel dependency metadata from the current declarations. (Run after update or upgrade tasks, or when updating things manually, or when we put back renovate)
  • update: move resolved dependency versions within the current declarations, then pin.
  • upgrade: change dependency declarations to later versions. UPDATED: This is not going to be its own task, this will be managed by Renovate.

It's a little confusing because

  • Java can't distinguish between update and upgrade like the other bindings.
  • Node does have the ability to "upgrade" but none of the other ecosystems have an equivalent
  • Python's custom script we were using was doing "upgrade"

The plan is to have tasks for everything for "update" and "pin" and use Renovate as appropriate for "upgrade" notifications.

💥 What does this PR do?

  • Python updates were inadvertently left out of release process
  • Release workflow needs to "pin" version updates after bump to nightly not "update"
  • Rust was just updating selenium version, this brings it in line with everything else, and splits pin and update behavior
  • UPDATED Python now splits pin and update by removing the py update script and relying on bazel tooling.

🔧 Implementation Notes

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): claude
    • What was generated: rust commands
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

Next step is figuring out Renovate
Upcoming PR to move Python dependencies to ranges so "update" won't be a noop

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Clarify dependency management tasks and include Python in release workflow

✨ Enhancement 📦 Other

Grey Divider

Walkthroughs

Description
• Clarify dependency management by distinguishing pin, update, and upgrade tasks
• Add Python to release workflow dependency pinning process
• Separate Rust pin and update behaviors for consistency
• Refactor Node.js tasks to split update and upgrade functionality
• Update release workflow to use pin instead of update for lockfiles
Diagram
flowchart LR
  A["Dependency Tasks"] --> B["Pin Task"]
  A --> C["Update Task"]
  A --> D["Upgrade Task"]
  B --> B1["Regenerate lockfiles<br/>and checksums"]
  C --> C1["Move versions within<br/>current declarations"]
  C --> B
  D --> D1["Change declarations<br/>to later versions"]
  D --> C
  E["Release Workflow"] --> F["Uses pin instead<br/>of update"]
  F --> G["All language bindings<br/>including Python"]
Loading

Grey Divider

File Changes

1. rake_tasks/java.rake ✨ Enhancement +3/-0

Add Maven upgrade task alias

• Add new upgrade task that aliases to update for Maven dependencies
• Clarify task descriptions for pin and update operations

rake_tasks/java.rake


2. rake_tasks/node.rake ✨ Enhancement +9/-6

Split Node update and upgrade tasks

• Split update task to only update within current ranges
• Create new upgrade task with --latest flag for bumping ranges
• Both tasks now invoke pin to refresh lockfile
• Remove task parameter for latest flag

rake_tasks/node.rake


3. rake_tasks/python.rake ✨ Enhancement +5/-5

Separate Python pin and update tasks

• Move update_py_deps script execution from pin to new update task
• Remove git add operations from pin task
• Change update from alias to standalone task that invokes pin
• Clarify task descriptions

rake_tasks/python.rake


View more (3)
4. rake_tasks/rust.rake ✨ Enhancement +13/-2

Split Rust pin and update tasks

• Create new pin task for Cargo.lock and Bazel crate metadata
• Refactor update task to update all dependencies instead of just selenium-manager
• Update task now invokes pin after updating
• Clarify task descriptions

rake_tasks/rust.rake


5. .github/workflows/release.yml ⚙️ Configuration changes +1/-1

Update release workflow to use pin task

• Change release workflow to use pin instead of update for lockfile reset
• Apply change to all language bindings including Rust

.github/workflows/release.yml


6. Rakefile ✨ Enhancement +2/-0

Include Python in all dependency tasks

• Add Python pin task to all:pin namespace
• Add Python update task to all:update namespace
• Ensure Python dependencies are included in batch operations

Rakefile


Grey Divider

Qodo Logo

@selenium-ci selenium-ci added the B-build Includes scripting, bazel and CI integrations label May 15, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 15, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (2) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Missing report validation ✓ Resolved 🐞 Bug ☼ Reliability
Description
scripts/update_py_deps.py treats missing packages in pip’s JSON report as “no change”, so it can
print “All repo tooling requirements are up to date!” even when version resolution didn’t return
entries for some/all requested requirements. This can silently leave py/requirements.txt stale while
the command exits successfully.
Code

scripts/update_py_deps.py[R54-73]

+    report = json.loads(result.stdout)
+    installed = {item["metadata"]["name"].lower(): item["metadata"]["version"] for item in report.get("install", [])}
+
+    # Update versions in original lines
+    updated_lines = []
+    updates = []
+    for orig_line, name_with_extras, name_normalized in packages:
+        old_version = orig_line.split("==")[1]
+        new_version = installed.get(name_normalized)
+
+        if new_version and new_version != old_version:
+            updates.append((name_with_extras, old_version, new_version))
+            updated_lines.append(f"{name_with_extras}=={new_version}")
+            print(f"  {name_with_extras}: {old_version} -> {new_version}")
+        else:
+            updated_lines.append(orig_line)
+
+    if not updates:
+        print("\nAll repo tooling requirements are up to date!")
+        return
Evidence
The script constructs installed only from report.get("install", []), then uses .get() to look
up each requested package; missing keys become None and are treated as “no update”. If this
happens for all packages, updates remains empty and the script returns early with an “up to date”
message, even though no versions were actually obtained.

scripts/update_py_deps.py[54-73]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`scripts/update_py_deps.py` builds `installed` from `report.get("install", [])` and then uses `installed.get(name_normalized)`; missing entries are treated as unchanged, and if `updates` stays empty the script prints a success message and exits. This can mask cases where the report is empty/partial and no versions were actually resolved.

## Issue Context
This script is used to update pinned versions in `py/requirements.txt` and is invoked via Bazel (`bazel run //scripts:update_py_deps`). A silent success with no updates when resolution data is missing makes dependency maintenance unreliable.

## Fix Focus Areas
- scripts/update_py_deps.py[54-73]

## Suggested fix
- After parsing the report, validate that every `name_normalized` from `packages` exists in `installed`.
 - If any are missing, raise a `RuntimeError` that includes a helpful diagnostic (e.g., mention missing names and include `result.stderr` and a truncated `result.stdout`).
- Optionally wrap `json.loads(result.stdout)` in `try/except json.JSONDecodeError` and raise a clearer error including stderr/stdout context.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. node:update drops latest arg 📘 Rule violation ≡ Correctness
Description
The node:update rake task no longer accepts/handles the latest argument, changing a user-invoked
interface and behavior for upgrading dependency ranges. This is a breaking change without
deprecation guidance to use node:upgrade instead.
Code

rake_tasks/node.rake[R47-56]

+desc 'Update JavaScript dependencies and refresh lockfile'
+task :update do
+  Bazel.execute('run', ['--', 'update', '-r', '--dir', Dir.pwd], '@pnpm//:pnpm')
+  Rake::Task['node:pin'].invoke
+end
+
+desc 'Upgrade JavaScript dependency declarations and refresh lockfile'
+task :upgrade do
+  Bazel.execute('run', ['--', 'update', '-r', '--latest', '--dir', Dir.pwd], '@pnpm//:pnpm')
  Rake::Task['node:pin'].invoke
Evidence
PR Compliance ID 2 requires maintaining backward compatibility for user-facing interfaces;
node:update is now defined without any argument handling and does not apply --latest. PR
Compliance ID 5 requires deprecating public functionality before removal with guidance; there is no
deprecation warning or compatibility shim directing users of node:update[latest] to
node:upgrade.

AGENTS.md
AGENTS.md
rake_tasks/node.rake[47-57]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`node:update` used to support a `latest` argument to bump dependency ranges; the task now ignores/removes that interface and behavior, which breaks existing invocations and provides no deprecation guidance.

## Issue Context
This PR introduces a new `node:upgrade` task that uses `--latest`, but existing workflows/users may still call `rake "node:update[latest]"` expecting the previous behavior.

## Fix Focus Areas
- rake_tasks/node.rake[47-56]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. py:update breaks on Windows ✓ Resolved 🐞 Bug ☼ Reliability
Description
The py:update rake task now runs //scripts:update_py_deps, but update_py_deps.py hardcodes the venv
pip location as <venv>/bin/pip, which does not exist on Windows (venv uses Scripts/). This will make
./go py:update fail on Windows environments with a FileNotFoundError when trying to execute pip.
Code

rake_tasks/python.rake[R132-136]

+desc 'Update Python dependencies'
+task :update do
+  Bazel.execute('run', [], '//scripts:update_py_deps')
+  Rake::Task['py:pin'].invoke
+end
Evidence
The PR changes py:update to run //scripts:update_py_deps, and that script currently builds a pip
path under venv_dir/bin which is POSIX-specific and will not exist on Windows venvs, causing the
subprocess invocations to fail.

rake_tasks/python.rake[127-136]
scripts/update_py_deps.py[34-71]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`py:update` now invokes `//scripts:update_py_deps`, but `scripts/update_py_deps.py` assumes a POSIX virtualenv layout (`venv/bin/pip`). On Windows the venv executables live under `venv\Scripts\` (e.g., `pip.exe`), so the script will fail when it tries to run pip.

### Issue Context
This regression is introduced by changing `py:update` from an alias of `py:pin` to executing `//scripts:update_py_deps`.

### Fix Focus Areas
- scripts/update_py_deps.py[34-71]
- rake_tasks/python.rake[127-136]

### Suggested fix
Update `scripts/update_py_deps.py` to invoke pip via the venv’s python in a cross-platform way:
- Determine the venv python path with `Scripts/python.exe` on Windows and `bin/python` on POSIX, then run `python -m pip ...`.
- Avoid directly calling `venv_dir / "bin" / "pip"`.

Example sketch:
```py
import os
bin_dir = "Scripts" if os.name == "nt" else "bin"
python = venv_dir / bin_dir / ("python.exe" if os.name == "nt" else "python")

subprocess.run([str(python), "-m", "pip", "install", "-q", "--upgrade", "pip"], ...)
subprocess.run([str(python), "-m", "pip", "install", "-q", *install_names], ...)
subprocess.run([str(python), "-m", "pip", "list", "--format=json"], ...)
```

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. update_py_deps Bazel target removed 📘 Rule violation ⚙ Maintainability
Description
The PR removes the //scripts:update_py_deps Bazel target (and its backing script), which can break
existing user/tooling workflows that invoke it. No deprecation path or compatibility alias is
provided to guide users to the replacement flow.
Code

scripts/BUILD.bazel[L50-54]

-py_binary(
-    name = "update_py_deps",
-    srcs = ["update_py_deps.py"],
-)
-
Evidence
Rule 2 requires avoiding breaking changes to existing user-invokable interfaces without explicit
direction, and Rule 6 requires deprecating public functionality before removal with an alternative.
The update_py_deps target is no longer present in scripts/BUILD.bazel, while py:update now
points users to the replacement //py:requirements.update -- --upgrade behavior, but without a
deprecation/compatibility bridge for the removed target.

AGENTS.md
AGENTS.md
scripts/BUILD.bazel[1-61]
rake_tasks/python.rake[127-135]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `//scripts:update_py_deps` Bazel target was removed, which can break downstream automation/users that still invoke it. Per compliance guidance, removals of user-invokable/public functionality should be preceded by deprecation and an explicit alternative.

## Issue Context
The new intended workflow appears to be `//py:requirements.update` (and `rake py:update` now passes `--upgrade`). However, existing callers of `bazel run //scripts:update_py_deps` will now fail without a clear migration path.

## Fix Focus Areas
- scripts/BUILD.bazel[47-55]
- rake_tasks/python.rake[127-135]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Rust repin path diverges 🐞 Bug ⚙ Maintainability
Description
rust:pin repins Rust crate metadata via bazel fetch @crates//... with CARGO_BAZEL_REPIN, but
the repo’s existing Rust repin automation uses CARGO_BAZEL_REPIN=true bazel run @crates//:all.
Since fetch does not execute @crates//:all, running ./go rust:pin will not follow the same
repin mechanism as CI, making local/manual repins inconsistent with the repo-standard repin process.
Code

rake_tasks/rust.rake[R26-34]

+desc 'Pin Rust dependencies'
+task :pin do
+  puts 'pinning Cargo.lock and Bazel crate metadata'
+  manifest = File.expand_path('rust/Cargo.toml')
+  Bazel.execute('run',
+                ['--', 'generate-lockfile', '--manifest-path', manifest],
+                '@rules_rust//tools/upstream_wrapper:cargo')
+  Bazel.execute('fetch', ['--repo_env=CARGO_BAZEL_REPIN=true'], '@crates//...')
+end
Evidence
The new rust:pin task uses bazel fetch ... @crates//..., while the repo’s Rust repin workflow
explicitly runs bazel run @crates//:all with CARGO_BAZEL_REPIN=true, demonstrating the standard
repin mechanism used in CI.

rake_tasks/rust.rake[26-34]
.github/workflows/ci-renovate-rbe.yml[19-26]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`rust:pin` currently uses `bazel fetch` to repin the Rust crate universe, but the repo’s CI repin workflow uses `CARGO_BAZEL_REPIN=true bazel run @crates//:all`. This means the local `rust:pin` task won’t exercise the same repin mechanism CI uses.

## Issue Context
The repo already has an established Rust repin command in `.github/workflows/ci-renovate-rbe.yml`. Aligning `rust:pin` with that command improves consistency and reduces “works locally but not in CI” drift.

## Fix Focus Areas
- rake_tasks/rust.rake[26-34]

### Suggested change
Replace the `Bazel.execute('fetch', ..., '@crates//...')` call with a `bazel run` of the repin target (e.g. `@crates//:all`) using the same `CARGO_BAZEL_REPIN` trigger.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. pip --report JSON unvalidated ✓ Resolved 📘 Rule violation ☼ Reliability
Description
The new dependency resolution flow parses pip --report - output with json.loads(result.stdout)
and then assumes specific keys exist, which can crash with non-actionable
JSONDecodeError/KeyError if pip emits non-JSON output or the report schema differs. This
violates the requirement to validate external/protocol inputs and raise deterministic, descriptive
exceptions.
Code

scripts/update_py_deps.py[R54-55]

+    report = json.loads(result.stdout)
+    installed = {item["metadata"]["name"].lower(): item["metadata"]["version"] for item in report.get("install", [])}
Evidence
The checklist requires validating external/protocol inputs and producing deterministic, actionable
exceptions. The code directly JSON-decodes pip stdout and indexes nested fields without guarding
against invalid JSON or missing keys, which can lead to unclear runtime crashes.

scripts/update_py_deps.py[54-55]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`scripts/update_py_deps.py` parses `pip --report -` output without validating that the output is valid JSON or that expected keys exist, which can produce unclear crashes (e.g., `JSONDecodeError`, `KeyError`).

## Issue Context
This script consumes external/protocol-derived data (pip report JSON). Per compliance, failures should be deterministic and actionable (explicit exceptions indicating what was wrong and how to fix).

## Fix Focus Areas
- scripts/update_py_deps.py[54-56]

## Suggested implementation notes
- Wrap `json.loads(result.stdout)` in `try/except json.JSONDecodeError` and raise a `RuntimeError` (or custom exception) that includes a short message and relevant `stderr/stdout` context.
- Validate `report` is a dict and that `report.get("install")` is a list.
- When building `installed`, use `.get()` accessors (or explicit checks) and raise a descriptive exception if required fields like `metadata.name`/`metadata.version` are missing.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR clarifies the distinction between pin, update, and upgrade dependency-management tasks across language bindings, and fixes the release workflow to invoke pin (refresh lockfiles after a version bump) instead of update (which would also bump resolved dependency versions). Python's tasks are also restructured so update actually upgrades resolved versions and pin only regenerates lockfiles, and Python is added to the all: aggregates that previously omitted it.

Changes:

  • Split pin vs update semantics for Rust and Python; add a separate node:upgrade and a java:upgrade alias for ranged bumps.
  • Add py:pin/py:update to all:pin/all:update so Python is no longer skipped.
  • Switch the release "Reset Dependencies" step from :update to :pin (and rust:updaterust:pin).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
Rakefile Adds py:pin and py:update to the all: aggregate tasks.
rake_tasks/rust.rake Introduces rust:pin (generate-lockfile + CARGO_BAZEL_REPIN) and broadens rust:update to update all crates, then pin.
rake_tasks/python.rake Makes py:pin only refresh the lockfile and py:update actually bump deps via //scripts:update_py_deps, then pin.
rake_tasks/node.rake Splits the old update[:latest] task into node:update and a new node:upgrade (uses --latest), each followed by node:pin.
rake_tasks/java.rake Adds a java:upgrade alias to java:update for naming parity with other bindings.
.github/workflows/release.yml Changes the post-version-bump reset step to call :pin (and rust:pin) instead of :update.

Comment thread rake_tasks/node.rake Outdated
Comment thread rake_tasks/python.rake Outdated
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 15, 2026

Persistent review updated to latest commit dde3f25

Comment thread scripts/update_py_deps.py Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 15, 2026

Persistent review updated to latest commit 11c1b0f

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 15, 2026

Persistent review updated to latest commit 3092715

@titusfortner
Copy link
Copy Markdown
Member Author

Not merging until @cgoldberg verifies the Python pieces look right. :)

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 16, 2026

Persistent review updated to latest commit 1ea3b7a

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 18, 2026

Persistent review updated to latest commit 4d9383e

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 18, 2026

Persistent review updated to latest commit 49a794a

@titusfortner titusfortner merged commit e9bfc1f into trunk May 18, 2026
40 checks passed
@titusfortner titusfortner deleted the deps branch May 18, 2026 22:10
shs96c added a commit to shs96c/selenium that referenced this pull request May 19, 2026
* origin/trunk: (97 commits)
  [py] update python dependencies (SeleniumHQ#17490)
  [build] fix renovate reported issues with configuration
  [build] remove base-ref from renovate workflows it does not work for the use case I had for them
  [build] add renovate dependency workflow (SeleniumHQ#17504)
  [build] simplify commit-changes workflow (SeleniumHQ#17503)
  [build] clarify dependency pin and update tasks (SeleniumHQ#17463)
  [build] do not rerun or attempt to upload logs unless workflow failure is from the Bazel step
  [build] fix renovate ignore rules_python to v2 until upstream fixed
  [build] renovate ignore rules_python until upstream fixed
  [build] bump rules_closure version (SeleniumHQ#17500)
  [build] bump rules_jvm_external (SeleniumHQ#17501)
  [js] remove npm dependency by using bazel for everything (SeleniumHQ#17499)
  [build] bump ruby versions to latest patch releases (SeleniumHQ#17496)
  [dotnet] [build] Support deterministic build output (SeleniumHQ#17497)
  [build] remove renovate update requests pending work done in SeleniumHQ#17427 (SeleniumHQ#17498)
  [dotnet] [build] Fix remote linkage in SourceLink (SeleniumHQ#17495)
  [rust] update reqwest to 0.13 (SeleniumHQ#17488)
  [build] bump low-risk Bazel module dependencies (SeleniumHQ#17494)
  [dotnet] run format against slnx instead of looping csproj (SeleniumHQ#17483)
  [build] ignore renovate.json references in renovate recommendations
  ...

# Conflicts:
#	MODULE.bazel
#	rust/BUILD.bazel
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations Compliance violation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants