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.

Sign up for GitHub

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

Jump to bottom

Shadow additional packages in wiremock-standalone #2327

Merged
merged 1 commit into from
Aug 29, 2023
Merged

Shadow additional packages in wiremock-standalone #2327

merged 1 commit into from
Aug 29, 2023

Conversation

jluehe
Copy link
Contributor

@jluehe jluehe commented Aug 26, 2023 •
edited

The joptsimple, javax.annotation, and org.yaml.snakeyaml packages are currently not being shadowed in the wiremock-standalone artifact:

% jar tvf build/libs/wiremock-standalone-3.0.0.jar | grep joptsimple
     0 Fri Feb 01 00:00:00 PST 1980 joptsimple/
  5158 Fri Feb 01 00:00:00 PST 1980 joptsimple/AbstractOptionSpec.class
  1606 Fri Feb 01 00:00:00 PST 1980 joptsimple/AlternativeLongOptionSpec.class
  [...]
% jar tvf build/libs/wiremock-standalone-3.0.0.jar | grep "javax/annotation"
     0 Fri Feb 01 00:00:00 PST 1980 javax/annotation/
   518 Fri Feb 01 00:00:00 PST 1980 javax/annotation/CheckForNull.class
   526 Fri Feb 01 00:00:00 PST 1980 javax/annotation/CheckForSigned.class
  [...]
% jar tvf build/libs/wiremock-standalone-3.0.0.jar | grep "org/yaml/snakeyaml"
     0 Fri Feb 01 00:00:00 PST 1980 org/yaml/snakeyaml/
  1748 Fri Feb 01 00:00:00 PST 1980 org/yaml/snakeyaml/DumperOptions$FlowStyle.class
  2139 Fri Feb 01 00:00:00 PST 1980 org/yaml/snakeyaml/DumperOptions$LineBreak.class
  [...]

This can cause conflicts when wiremock-standalone is integrated into another project which pulls in different versions of these dependencies.

The proposed fix adds these packages to the list of packages to be shadowed.

Verification:

% jar tvf build/libs/wiremock-standalone-3.0.0.jar | grep joptsimple
     0 Fri Feb 01 00:00:00 PST 1980 wiremock/joptsimple/
  5455 Fri Feb 01 00:00:00 PST 1980 wiremock/joptsimple/AbstractOptionSpec.class
  1750 Fri Feb 01 00:00:00 PST 1980 wiremock/joptsimple/AlternativeLongOptionSpec.class
  [...]
% jar tvf build/libs/wiremock-standalone-3.0.0.jar | grep "javax/annotation"
     0 Fri Feb 01 00:00:00 PST 1980 wiremock/javax/annotation/
   554 Fri Feb 01 00:00:00 PST 1980 wiremock/javax/annotation/CheckForNull.class
   562 Fri Feb 01 00:00:00 PST 1980 wiremock/javax/annotation/CheckForSigned.class
  [...]
% jar tvf build/libs/wiremock-standalone-3.0.0.jar | grep "org/yaml/snakeyaml"
     0 Fri Feb 01 00:00:00 PST 1980 wiremock/org/yaml/snakeyaml/
  1811 Fri Feb 01 00:00:00 PST 1980 wiremock/org/yaml/snakeyaml/DumperOptions$FlowStyle.class
  2211 Fri Feb 01 00:00:00 PST 1980 wiremock/org/yaml/snakeyaml/DumperOptions$LineBreak.class
  [...]

References

  • TODO

Submitter checklist

  • Recommended: Join WireMock Slack to get any help in #help-contributing or a project-specific channel like #wiremock-java
  • The PR request is well described and justified, including the body and the references
  • The PR title represents the desired changelog entry
  • The repository's code style is followed (see the contributing guide)
  • Test coverage that demonstrates that the change works as expected
  • For new features, there's necessary documentation in this pull request or in a subsequent PR to wiremock.org

@jluehe jluehe requested a review from a team as a code owner August 26, 2023 23:03
@jluehe jluehe changed the title Shadow joptsimple and javax.annotation packages Shadow additional packages Aug 27, 2023
@jluehe jluehe changed the title Shadow additional packages Shadow additional packages in wiremock-standalone Aug 27, 2023
oleg-nenashev
oleg-nenashev approved these changes Aug 28, 2023
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

LGTM but it would benefit a second check by @tomakehurst w.r.t javax.annotation

build.gradle
@@ -306,6 +306,9 @@ shadowJar {
relocate "org.checkerframework", "wiremock.org.checkerframework"
relocate "org.hamcrest", "wiremock.org.hamcrest"
relocate "org.slf4j", "wiremock.org.slf4j"
relocate "joptsimple", "wiremock.joptsimple"
relocate "javax.annotation", "wiremock.javax.annotation"
Copy link
Member

Choose a reason for hiding this comment

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

I am not 100% sure we have enough test coverage to ensure it does not explode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the quick review and feedback, @oleg-nenashev! As an alternative, we could try excluding javax.annotation from the standalone artifact ...

tomakehurst
tomakehurst reviewed Aug 28, 2023
View reviewed changes
build.gradle Outdated
@@ -306,6 +306,9 @@ shadowJar {
relocate "org.checkerframework", "wiremock.org.checkerframework"
relocate "org.hamcrest", "wiremock.org.hamcrest"
relocate "org.slf4j", "wiremock.org.slf4j"
relocate "joptsimple", "wiremock.joptsimple"
relocate "javax.annotation", "wiremock.javax.annotation"
relocate "org.yaml.snakeyaml", "wiremock.org.yaml.snakeyaml"
Copy link
Member

Choose a reason for hiding this comment

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

Suggest we shorten this to org.yaml

Copy link
Member

Choose a reason for hiding this comment

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

Please can we also add:
com.ethlo
com.networknt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, @tomakehurst! Updated the PR

@tomakehurst tomakehurst merged commit 3af6327 into wiremock:master Aug 29, 2023
@tomakehurst
Copy link
Member

Thanks!

tomakehurst pushed a commit to oleg-nenashev/wiremock-java that referenced this pull request Aug 29, 2023
@jluehe @tomakehurst
Shadow additional packages (wiremock#2327)
e6062fa
tomakehurst added a commit that referenced this pull request Aug 29, 2023
@oleg-nenashev @tomakehurst @jluehe
Add annotations for Beta and Internal APIs (#2332)
77f9b4d 
* Add annotations for Beta and Internal APIs

* Spotless

* Excluded transitive jsr305 dependency from build to avoid javax.annotation classes ending up in the standalone JAR

* Replace Guava by JDK (Partly)

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* Replace Guava by JDK (Partly)

* Replace Guava by JDK (Partly)

* Replace Guava by JDK (Partly)

* update

* update

* Shadow additional packages (#2327)

* Removed redundant relocation of javax.annotation from the build

* Fixed Spotless errors

---------

Co-authored-by: Tom Akehurst <tom@wiremock.org>
Co-authored-by: Kirill Peshin <kirill.peshin@bscmsc.ru>
Co-authored-by: jluehe <janluehe@yahoo.com>
@jluehe
Copy link
Contributor Author

jluehe commented Aug 29, 2023

Thank you, @tomakehurst! Are there any plans to do another 2.x release? If so, could you please tell me which branch I should use for backporting these changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomakehurst tomakehurst tomakehurst left review comments

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
wiremock-3.0-final-merges
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jluehe @tomakehurst @oleg-nenashev