Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[java] make external modules static #12294

Merged
merged 2 commits into from
Jul 31, 2023
Merged

Conversation

joerg1985
Copy link
Member

Motivation and Context

In the past without the java module system you could exclude jars from the dependencies in your pom.xml.
This helped to reduce the overhead of dependencies which where not used by your code.
e.g. if you do not use the new JDK HttpClient you could exclude the netty async client.

With the module system enabled this was not possible, the netty async client was a required module.
When compiling the code a module not found error is raised.

Description

This change will use the static modifier on external² modules, making them optional at runtime. The user can now remove them and might risk to get an exception at runtime.

² not selenium and not java modules

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@joerg1985
Copy link
Member Author

@titusfortner this is what we wrote about in slack.

@joerg1985 joerg1985 marked this pull request as draft July 1, 2023 18:10
@joerg1985
Copy link
Member Author

The browser driver jars should also be static, so i need to rework this.

@joerg1985 joerg1985 marked this pull request as ready for review July 2, 2023 17:15
@joerg1985
Copy link
Member Author

joerg1985 commented Jul 2, 2023

Looks like the browser jars are not referenced in the other jars so there is no need to make them static.

@shs96c I am not sure if the flags are really set in the final jars, e.g. the transitive flag is set for java.logging in org.seleniumhq.selenium.api, but the module-info.class has this flag not set as far as i can tell. I see a change in the outJar when changing the flags. So i assume the flags are lost in the bazel-jar-magic but did not find the cause for this.

@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (25a5189) 57.38% compared to head (bb8bd71) 57.38%.
Report is 4 commits behind head on trunk.

❗ Current head bb8bd71 differs from pull request most recent head fee0897. Consider uploading reports for the commit fee0897 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff           @@
##            trunk   #12294   +/-   ##
=======================================
  Coverage   57.38%   57.38%           
=======================================
  Files          86       86           
  Lines        5369     5369           
  Branches      206      206           
=======================================
  Hits         3081     3081           
  Misses       2082     2082           
  Partials      206      206           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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, @joerg1985!

@diemol diemol merged commit f2d8427 into SeleniumHQ:trunk Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants