Skip to content

Conversation

Earlopain
Copy link
Contributor

@Earlopain Earlopain commented Oct 15, 2025

User description

This project does not use prism at all. At most it is being used by rubocop during local development.

It was added in #16357 but it is unclear to me why. I just bumped it for this repo, perhaps that JRuby issue has since been fixed. Either way, it should not be part of the gemspec.

🔗 Related Issues

Added in #16357

💥 What does this PR do?

Remove prism as a dependency and bump it.

🔧 Implementation Notes

selenium-webdriver does not use prism at all

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix


Description

  • Remove unused prism dependency from gemspec

  • Bump prism version in MODULE.bazel to 1.5.2

  • Clean up unnecessary runtime dependency


Diagram Walkthrough

flowchart LR
  gemspec["selenium-webdriver.gemspec"] -- "remove dependency" --> prism["prism gem"]
  bazel["MODULE.bazel"] -- "bump version" --> prism_version["prism 1.5.2"]
Loading

File Walkthrough

Relevant files
Dependencies
selenium-webdriver.gemspec
Remove prism dependency from gemspec                                         

rb/selenium-webdriver.gemspec

  • Removed prism dependency from gemspec dependencies list
  • Dependency was unused by the project at runtime
+0/-1     
MODULE.bazel
Bump prism version to 1.5.2                                                           

MODULE.bazel

  • Updated prism version from 1.4.0 to 1.5.2
  • Bumped SHA hash for new version
+1/-1     

This project does not use prism at all. At most it is being used by rubocop during local development.

It was added in SeleniumHQ#16357 but it is unclear to me why.
I just bumped it for this repo, perhaps that JRuby issue has since been fixed.
Either way, it should not be part of the gemspec.
@selenium-ci selenium-ci added C-rb Ruby Bindings B-build Includes scripting, bazel and CI integrations labels Oct 15, 2025
Copy link
Contributor

qodo-merge-pro bot commented Oct 15, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
- [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true -->

</details></td></tr>
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link
Contributor

qodo-merge-pro bot commented Oct 15, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Remove unused dependency from build configuration

Remove the prism dependency from MODULE.bazel to align with its removal from
rb/selenium-webdriver.gemspec and fully complete the dependency removal.

MODULE.bazel [321]

-"prism-1.5.2": "192741663a55af1ac1b987caa1092deb666e4ff46a30c5064ad5456acd05df1d",
 
+
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a logical inconsistency in the PR, where a dependency is removed from the gemspec but only updated in the Bazel configuration, thus failing to fully achieve the PR's goal.

High
  • Update

@Earlopain
Copy link
Contributor Author

If there is something broken with prism on jruby, please let me know. I am a contributor to prism.

@diemol
Copy link
Member

diemol commented Oct 15, 2025

I think there was a conflict with Rubocop. Let's run the CI and see if it fails again.

diemol and others added 2 commits October 15, 2025 14:09
It fails to build with the following error:
src/prism.c: In function ‘context_terminator’:
src/prism.c:8651:62: error: conversion to ‘unsigned int’ from ‘int’ may change
the sign of the result [-Werror=sign-conversion]
8651 |     return token->type < 32 && (context_terminators[context] & (1 <<
token->type));
      |                                                              ^
cc1: all warnings being treated as errors
@Earlopain
Copy link
Contributor Author

Ok, I see the error. It's a build failure that I don't think is related to JRuby at all. I have pinned back to 1.4.0 in the Gemfile to allow the build to pass. Specifying it there allows consumers of selenium-webdriver to update their own copy of prism.

I will see about getting that warning fixed in prism so that it can be unpinned here. Unsure why it fails your build here, I guess some compiler option that ruby itself doesn't test against.

Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

Thank you!

@Earlopain
Copy link
Contributor Author

The build failure may be fixed in the next prism release: ruby/prism#3682

@diemol diemol merged commit 4023c01 into SeleniumHQ:trunk Oct 16, 2025
22 checks passed
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 Review effort 1/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants