Skip to content

[rb] deprecate curb http client support#17443

Merged
titusfortner merged 2 commits into
trunkfrom
remove_curl
May 14, 2026
Merged

[rb] deprecate curb http client support#17443
titusfortner merged 2 commits into
trunkfrom
remove_curl

Conversation

@titusfortner
Copy link
Copy Markdown
Member

I don't think anyone actually uses this, and because there aren't binaries they have to be compiled, not much benefit to keeping this I don't think.

💥 What does this PR do?

  • Deprecate curb client
  • Remove tests
  • Remove dependencies
  • Remove curl install

@titusfortner titusfortner requested review from aguspe and p0deje May 12, 2026 17:13
@selenium-ci selenium-ci added C-rb Ruby Bindings B-build Includes scripting, bazel and CI integrations labels May 12, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Deprecate Curb HTTP client support

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Deprecate Curb HTTP client with logger warning
• Remove Curb unit tests and specifications
• Remove Curb gem dependency from Gemfile
• Remove curl installation from CI workflow
Diagram
flowchart LR
  A["Curb HTTP Client"] -->|deprecate| B["Logger Warning"]
  A -->|remove| C["Unit Tests"]
  A -->|remove| D["Gem Dependency"]
  A -->|remove| E["CI Setup"]
Loading

Grey Divider

File Changes

1. rb/lib/selenium/webdriver/remote/http/curb.rb Deprecation +6/-8

Add deprecation warning to Curb class

• Added deprecation warning in initialize method using WebDriver.logger.deprecate
• Updated class documentation to reference deprecation and custom HTTP client subclassing
• Removed usage examples and installation instructions

rb/lib/selenium/webdriver/remote/http/curb.rb


2. rb/spec/unit/selenium/webdriver/remote/http/curb_spec.rb 🧪 Tests +0/-54

Remove Curb HTTP client unit tests

• Removed entire test file (54 lines)
• Eliminated all Curb client unit tests including timeout initialization and configuration tests

rb/spec/unit/selenium/webdriver/remote/http/curb_spec.rb


3. .github/workflows/bazel.yml ⚙️ Configuration changes +0/-3

Remove curl setup from CI workflow

• Removed curl installation step for Ubuntu CI environment
• Eliminated sudo apt-get install libcurl4-openssl-dev command

.github/workflows/bazel.yml


View more (2)
4. MODULE.bazel Dependencies +0/-1

Remove Curb from Bazel dependencies

• Removed curb-1.0.9 dependency hash from Ruby bundle configuration

MODULE.bazel


5. rb/Gemfile Dependencies +0/-1

Remove Curb gem from Gemfile

• Removed gem 'curb', '~> 1.0.5' dependency declaration

rb/Gemfile


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 12, 2026

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (1)

Grey Divider


Action required

1. Apt indexes not refreshed 🐞 Bug ☼ Reliability
Description
The removed Ubuntu curl setup step was the only place the job ran apt-get update, but the workflow
still executes apt-get install fluxbox for browser runs. This can intermittently fail on fresh
Ubuntu runners due to stale package indexes, breaking Bazel CI for those jobs.
Code

.github/workflows/bazel.yml[L218-220]

-      - name: Setup curl for Ubuntu
-        if: inputs.os == 'ubuntu'
-        run: sudo apt-get update && sudo apt-get install -y libcurl4-openssl-dev
Evidence
The workflow still installs fluxbox via apt, but the prior step that performed the `apt-get
update has been removed, leaving an apt-get install` without an update in the job steps.

.github/workflows/bazel.yml[218-224]

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 Ubuntu workflow removed the step that ran `apt-get update`, but it still installs `fluxbox` via `apt-get install`. This can fail on fresh runners when package indexes are stale.

### Issue Context
The curl setup step previously ran `sudo apt-get update && ...` and was located before the Fluxbox/Xvfb install step. After removal, the remaining `apt-get install fluxbox` has no preceding update.

### Fix Focus Areas
- .github/workflows/bazel.yml[218-224]

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



Remediation recommended

2. Stale curb type stubs 🐞 Bug ⚙ Maintainability ⭐ New
Description
curb is removed from the repo’s Ruby bundle, but the repo still ships RBS signatures defining
Curl/Curl::Easy and Remote::Http::Curb, leaving dead/stale type definitions that can mask the
fact the runtime dependency is no longer present. This increases future maintenance risk
(type/version skew) if curb stays optional/deprecated or is later reintroduced at a different
version.
Code

rb/Gemfile[10]

-gem 'curb', '~> 1.0.5', require: false, platforms: %i[mri mingw x64_mingw]
Evidence
The PR removes the curb gem from the managed dependency set, but curb-related RBS files remain in
the repo and still reference Curl::Easy, creating a mismatch between bundled runtime deps and
committed type signatures.

rb/Gemfile[1-11]
rb/Gemfile.lock[1-40]
MODULE.bazel[282-315]
rb/sig/gems/curb/curl.rbs[1-20]
rb/sig/lib/selenium/webdriver/remote/http/curb.rbs[1-20]

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

### Issue description
This PR removes the `curb` gem from the repo-managed Ruby dependencies, but RBS signatures for `curb` remain committed under `rb/sig/**`. This leaves stale type definitions (`Curl::Easy`, `Remote::Http::Curb`) that no longer correspond to a bundled runtime dependency and can create version skew or confusion.

### Issue Context
- `rb/Gemfile` no longer includes `curb`, and `rb/Gemfile.lock` contains no `curb` entries.
- `MODULE.bazel` no longer includes a `curb-*` checksum.
- Despite that, the repository still contains curb-related RBS files.

### Fix Focus Areas
- rb/sig/gems/curb/curl.rbs[1-999]
- rb/sig/lib/selenium/webdriver/remote/http/curb.rbs[1-999]

### Suggested fix
Choose one of the following consistent approaches:
1) **Remove stale signatures**: delete `rb/sig/gems/curb/` and (if not needed) delete `rb/sig/lib/selenium/webdriver/remote/http/curb.rbs`.
2) **Keep `Curb` signature but decouple from Curl types**: keep `curb.rbs` but change `def client: () -> Curl::Easy` to `-> untyped` (or `-> Object`) and remove `rb/sig/gems/curb/curl.rbs`.
3) **If you intend to keep curb signatures** as “optional types”, document this explicitly (e.g., comment in the RBS file) and ensure it won’t conflict with future `curb` gem versions.

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


3. Curb deprecation lacks tests 📘 Rule violation ☼ Reliability
Description
Selenium::WebDriver::Remote::Http::Curb now emits a deprecation warning on initialization, but
this new user-visible behavior is not covered by tests (and the prior unit coverage was removed in
this PR). Missing coverage increases the risk of regressions or accidental warning/behavior changes
going unnoticed.
Code

rb/lib/selenium/webdriver/remote/http/curb.rb[R35-41]

          def initialize(timeout: nil)
+            WebDriver.logger.deprecate(
+              'Selenium::WebDriver::Remote::Http::Curb',
+              'a custom subclass of Selenium::WebDriver::Remote::Http::Common',
+              id: :curb
+            )
            @timeout = timeout
Evidence
PR Compliance ID 5 requires relevant changes to be covered by tests. The updated constructor now
calls WebDriver.logger.deprecate(...), introducing new behavior that should be asserted by a unit
test.

AGENTS.md
rb/lib/selenium/webdriver/remote/http/curb.rb[35-41]

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

## Issue description
A deprecation warning was added to `Selenium::WebDriver::Remote::Http::Curb#initialize`, but there is no longer unit-test coverage asserting this behavior.

## Issue Context
The PR introduces a user-visible behavior change (deprecation warning) and removes the prior unit test file for this class. Per the compliance checklist, functional/user-visible changes should be covered by relevant tests.

## Fix Focus Areas
- rb/lib/selenium/webdriver/remote/http/curb.rb[35-41]
- rb/spec/unit/selenium/webdriver/remote/http/*[1-200]

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


4. Deprecation warning unreachable 🐞 Bug ☼ Reliability
Description
Selenium::WebDriver::Remote::Http::Curb now logs a deprecation warning in #initialize, but
remote/http/curb.rb still does a top-level require 'curb'; with curb removed from the repo
Gemfile/bundle, requiring this file raises LoadError before the deprecation warning can run. This
prevents the intended migration guidance and can break any tooling/scripts that try to require this
file under the repo’s default dependencies.
Code

rb/lib/selenium/webdriver/remote/http/curb.rb[R35-43]

          def initialize(timeout: nil)
+            WebDriver.logger.deprecate(
+              'Selenium::WebDriver::Remote::Http::Curb',
+              'a custom subclass of Selenium::WebDriver::Remote::Http::Common',
+              id: :curb
+            )
            @timeout = timeout
            super()
          end
Evidence
The Curb HTTP client file still requires the curb gem at top-level, but the repo Gemfile no longer
installs it, so loading the file can raise LoadError before the newly-added logger.deprecate
call in #initialize is reached.

rb/lib/selenium/webdriver/remote/http/curb.rb[20-43]
rb/Gemfile[8-11]
rb/lib/selenium/webdriver/common/logger.rb[192-207]

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

### Issue description
`rb/lib/selenium/webdriver/remote/http/curb.rb` requires the `curb` gem at file load time, but this PR removes `curb` from the repo’s dependency set. As a result, `require 'selenium/webdriver/remote/http/curb'` can fail with `LoadError` before the new deprecation warning in `#initialize` executes.

### Issue Context
The class is being deprecated, so callers should receive a clear deprecation/migration message even if `curb` is not installed.

### Fix Focus Areas
- rb/lib/selenium/webdriver/remote/http/curb.rb[20-43]
- rb/Gemfile[8-11]

### Implementation notes
- Move `require 'curb'` to a lazy-load point (e.g., inside `client`), or
- Wrap the top-level require in `begin/rescue LoadError` and raise a `Selenium::WebDriver::Error::WebDriverError` with a clear message (and/or emit the deprecation warning) indicating `curb` is no longer bundled and users should either install it explicitly or migrate to a `Common` subclass.

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


Grey Divider

Previous review results

Review updated until commit e23e3d1

Results up to commit a519967


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


Remediation recommended
1. Curb deprecation lacks tests 📘 Rule violation ☼ Reliability
Description
Selenium::WebDriver::Remote::Http::Curb now emits a deprecation warning on initialization, but
this new user-visible behavior is not covered by tests (and the prior unit coverage was removed in
this PR). Missing coverage increases the risk of regressions or accidental warning/behavior changes
going unnoticed.
Code

rb/lib/selenium/webdriver/remote/http/curb.rb[R35-41]

          def initialize(timeout: nil)
+            WebDriver.logger.deprecate(
+              'Selenium::WebDriver::Remote::Http::Curb',
+              'a custom subclass of Selenium::WebDriver::Remote::Http::Common',
+              id: :curb
+            )
            @timeout = timeout
Evidence
PR Compliance ID 5 requires relevant changes to be covered by tests. The updated constructor now
calls WebDriver.logger.deprecate(...), introducing new behavior that should be asserted by a unit
test.

AGENTS.md
rb/lib/selenium/webdriver/remote/http/curb.rb[35-41]

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

## Issue description
A deprecation warning was added to `Selenium::WebDriver::Remote::Http::Curb#initialize`, but there is no longer unit-test coverage asserting this behavior.

## Issue Context
The PR introduces a user-visible behavior change (deprecation warning) and removes the prior unit test file for this class. Per the compliance checklist, functional/user-visible changes should be covered by relevant tests.

## Fix Focus Areas
- rb/lib/selenium/webdriver/remote/http/curb.rb[35-41]
- rb/spec/unit/selenium/webdriver/remote/http/*[1-200]

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


2. Deprecation warning unreachable 🐞 Bug ☼ Reliability
Description
Selenium::WebDriver::Remote::Http::Curb now logs a deprecation warning in #initialize, but
remote/http/curb.rb still does a top-level require 'curb'; with curb removed from the repo
Gemfile/bundle, requiring this file raises LoadError before the deprecation warning can run. This
prevents the intended migration guidance and can break any tooling/scripts that try to require this
file under the repo’s default dependencies.
Code

rb/lib/selenium/webdriver/remote/http/curb.rb[R35-43]

          def initialize(timeout: nil)
+            WebDriver.logger.deprecate(
+              'Selenium::WebDriver::Remote::Http::Curb',
+              'a custom subclass of Selenium::WebDriver::Remote::Http::Common',
+              id: :curb
+            )
            @timeout = timeout
            super()
          end
Evidence
The Curb HTTP client file still requires the curb gem at top-level, but the repo Gemfile no longer
installs it, so loading the file can raise LoadError before the newly-added logger.deprecate
call in #initialize is reached.

rb/lib/selenium/webdriver/remote/http/curb.rb[20-43]
rb/Gemfile[8-11]
rb/lib/selenium/webdriver/common/logger.rb[192-207]

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

### Issue description
`rb/lib/selenium/webdriver/remote/http/curb.rb` requires the `curb` gem at file load time, but this PR removes `curb` from the repo’s dependency set. As a result, `require 'selenium/webdriver/remote/http/curb'` can fail with `LoadError` before the new deprecation warning in `#initialize` executes.

### Issue Context
The class is being deprecated, so callers should receive a clear deprecation/migration message even if `curb` is not installed.

### Fix Focus Areas
- rb/lib/selenium/webdriver/remote/http/curb.rb[20-43]
- rb/Gemfile[8-11]

### Implementation notes
- Move `require 'curb'` to a lazy-load point (e.g., inside `client`), or
- Wrap the top-level require in `begin/rescue LoadError` and raise a `Selenium::WebDriver::Error::WebDriverError` with a clear message (and/or emit the deprecation warning) indicating `curb` is no longer bundled and users should either install it explicitly or migrate to a `Common` subclass.

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


Results up to commit 23b49b3


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


Action required
1. Apt indexes not refreshed 🐞 Bug ☼ Reliability
Description
The removed Ubuntu curl setup step was the only place the job ran apt-get update, but the workflow
still executes apt-get install fluxbox for browser runs. This can intermittently fail on fresh
Ubuntu runners due to stale package indexes, breaking Bazel CI for those jobs.
Code

.github/workflows/bazel.yml[L218-220]

-      - name: Setup curl for Ubuntu
-        if: inputs.os == 'ubuntu'
-        run: sudo apt-get update && sudo apt-get install -y libcurl4-openssl-dev
Evidence
The workflow still installs fluxbox via apt, but the prior step that performed the `apt-get
update has been removed, leaving an apt-get install` without an update in the job steps.

.github/workflows/bazel.yml[218-224]

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 Ubuntu workflow removed the step that ran `apt-get update`, but it still installs `fluxbox` via `apt-get install`. This can fail on fresh runners when package indexes are stale.

### Issue Context
The curl setup step previously ran `sudo apt-get update && ...` and was located before the Fluxbox/Xvfb install step. After removal, the remaining `apt-get install fluxbox` has no preceding update.

### Fix Focus Areas
- .github/workflows/bazel.yml[218-224]

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


Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 14, 2026

Persistent review updated to latest commit 23b49b3

Comment thread .github/workflows/bazel.yml
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 14, 2026

Persistent review updated to latest commit e23e3d1

@titusfortner titusfortner merged commit 2a11903 into trunk May 14, 2026
51 of 53 checks passed
@titusfortner titusfortner deleted the remove_curl branch May 14, 2026 15:59
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 C-rb Ruby Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants