[build] Reconcile Gemfile.lock in rb:pin with bundle lock, not bundle update#17772
Merged
Conversation
selenium-ci
added
C-rb
Member
|
Thank you, @titusfortner for this code suggestion. The support packages contain example code that many users find helpful, but they do not necessarily represent After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks. |
Contributor
PR Summary by QodoMake rb:pin regenerate Gemfile.lock via bundle lock before syncing checksums
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
19 rules 1. No tests for rb:pin change
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
titusfortner
force-pushed
the
c/bundle-update-versions-dcf812
branch
from
376e106 to
8f97969
Compare
Comment on lines
+190
to
+193
| desc 'Reconcile Gemfile.lock and sync gem checksums to MODULE.bazel (use force to re-download all)' | ||
| task :pin, [:force] do |_task, arguments| | ||
| Bazel.execute('run', [], '//rb:bundle-lock') | ||
|
|
Contributor
There was a problem hiding this comment.
1. Bundler runs twice in update 🐞 Bug ➹ Performance
rb:update runs //rb:bundle-update and then invokes rb:pin, which now always runs //rb:bundle-lock, causing an extra Bundler invocation on every update and potentially rewriting rb/Gemfile.lock a second time. This increases runtime and can introduce non-essential lockfile churn in the upgrade workflow.
Agent Prompt
### Issue description
`rb:update` already runs Bundler (`//rb:bundle-update`) and then calls `rb:pin`, which now runs Bundler again (`//rb:bundle-lock`). This adds unnecessary work and may produce extra lockfile churn.
### Issue Context
- `rb:pin` now executes `//rb:bundle-lock` unconditionally.
- `rb:update` executes `//rb:bundle-update` and then invokes `rb:pin`.
### Fix Focus Areas
- rake_tasks/ruby.rake[190-193]
- rake_tasks/ruby.rake[248-254]
### Suggested fix
Introduce a way for `rb:update` to reuse the already-updated lockfile without re-running `bundle lock` (e.g., split checksum syncing into its own task, or add an optional `skip_lock` argument to `rb:pin` and use it from `rb:update`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+38
to
+41
| # The bundle subcommand (e.g. "lock" or "update") and any flags are supplied by | ||
| # the caller so a single script backs both //rb:bundle-lock and //rb:bundle-update. | ||
| ruby = RbConfig.ruby | ||
| exec ruby, "-S", "bundle", "update", *ARGV | ||
| exec ruby, "-S", "bundle", *ARGV |
Contributor
There was a problem hiding this comment.
2. No bundle subcommand check 🐞 Bug ☼ Reliability
rb/support/bundle.rb now forwards caller-supplied ARGV directly to bundle without validating a subcommand is present, so a misconfigured Bazel target or direct invocation can trigger Bundler’s default/help behavior with unclear diagnostics. Adding a small guard makes failures deterministic and easier to debug.
Agent Prompt
### Issue description
`rb/support/bundle.rb` now executes `bundle` with `*ARGV` and does not enforce that `ARGV[0]` is a supported subcommand (e.g., `lock` or `update`). If invoked without args (or with a typo), behavior/exit codes are delegated to Bundler and can be confusing.
### Issue Context
This wrapper is intended to be shared by multiple Bazel entrypoints (`//rb:bundle-lock`, `//rb:bundle-update`). Those currently supply arguments correctly, but adding validation prevents future misconfiguration.
### Fix Focus Areas
- rb/support/bundle.rb[38-41]
### Suggested fix
Add a guard like:
- if `ARGV.empty?`, print usage and exit non-zero
- optionally validate `ARGV.first` is in an allowlist (`lock`, `update`) before calling `exec`
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 8f97969 |
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.
🔗 Related Issues
Supersedes #17768
💥 What does this PR do?
Makes
rb:pinregeneraterb/Gemfile.lockbefore syncing checksums, so it is self-sufficient like every other language's:pintask.rb:pinpreviously only copied gem checksums out of the existing lockfile; it never re-ran bundler. After a release version bump,Gemfile.lock's self-referencingselenium-webdriverpath-gem version drifted from the gemspec, and bundler's frozen mode then rejected the mismatch, breakingbazel run //rb:rubocopon trunk.It reconciles with
bundle lock(notbundle update), so:pinstays deterministic — it only updates what actually changed and does not opportunistically upgrade other gems. This preserves the:pin(reconcile) vs:update(intentional upgrade) split, which matters because:pinruns automatically in the release reset and in renovate.🔧 Implementation Notes
The bundle subcommand is now a caller-supplied argument, so one
rb/support/bundle.rbbacks both//rb:bundle-lockand//rb:bundle-updateinstead of duplicating the bundle-env setup across two scripts.🤖 AI assistance
💡 Additional Considerations
Supersedes #17768, which fixed the same drift but reconciled with
bundle updateinside:pin— that turns every automated repin into a full re-resolution. Close that PR in favor of this one.🔄 Types of changes