Skip to content

[rb] Remove prism dependency#16437

Merged
diemol merged 4 commits intoSeleniumHQ:trunkfrom
Earlopain:rb-remove-prism-dep
Oct 16, 2025
Merged

[rb] Remove prism dependency#16437
diemol merged 4 commits intoSeleniumHQ:trunkfrom
Earlopain:rb-remove-prism-dep

Conversation

@Earlopain
Copy link
Copy Markdown
Contributor

@Earlopain Earlopain commented Oct 15, 2025 •
edited by qodo-code-review bot
Loading

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     

@Earlopain
[rb] Remove prism dependency
c44f90e 
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
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review bot commented Oct 15, 2025 •
edited
Loading

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

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review bot commented Oct 15, 2025 •
edited
Loading

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
Copy Markdown
Contributor Author

Earlopain commented Oct 15, 2025

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

@diemol
Copy link
Copy Markdown
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
@diemol
Merge branch 'trunk' into rb-remove-prism-dep
dee4ce8
@Earlopain
Pin prism to 1.4.0 for the project
3e3b2f1 
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
Copy Markdown
Contributor Author

Earlopain commented Oct 15, 2025

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.

diemol
diemol approved these changes Oct 15, 2025
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Earlopain commented Oct 16, 2025

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
@jmaino jmaino mentioned this pull request Oct 16, 2025
Closed
@Earlopain Earlopain mentioned this pull request Oct 16, 2025
Merged
@boris-petrov
Copy link
Copy Markdown
Contributor

boris-petrov commented Oct 20, 2025

@Earlopain I'm not sure what specific JRuby issue they are talking about here but I have this specific JRuby issue with prism. There is a reproduction repo. Note, however, that this is most likely a JRuby issue, not a prism one, but just posting this here as you might have any idea...

@Earlopain Earlopain mentioned this pull request Oct 20, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@diemol diemol diemol approved these changes

Assignees

No one assigned

Labels

B-build Includes scripting, bazel and CI integrations C-rb Ruby Bindings Review effort 1/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Earlopain @diemol @boris-petrov @selenium-ci