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

Fix shading of isorelax (#200). #202

Merged
merged 1 commit into from
Mar 27, 2024
Merged

Fix shading of isorelax (#200). #202

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,42 @@ SAX2 and Stax2 APIs
<scope>provided</scope>
-->
<optional>true</optional>
<exclusions>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment to indicate why these are needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added some comments.
To summarize:

  1. Don't include transient isorelax:isorelax dependency that lacks source code on Maven Central, since this implies questionable provenance.
  2. Don't include transient xml-apis:xml-apis dependency that is repackaging XML APIs (SAX, DOM, etc.) that are built into the JDK itself.
  3. Don't include transient xerces:xercesImpl dependency since all modern JDKs already include it.
  4. Manually specify com.sun.xml.bind.jaxb:isorelax since that includes source code on Maven Central, and matches the version that was shaded in previous Woodstox releases.

Excluding the transient xml-apis:xml-apis & xerces:xercesImpl dependencies can also help ensure that unit tests don't end up falsely relying on features/quirks of these jar's instead of relying on the versions that are included in the base JDK.

Copy link
Member

Choose a reason for hiding this comment

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

Given that there are sort of 2 separate things -- exclusion of XercesImpl, JDK APIs -- and then bigger part of iso-relax, would it make sense to separate these into different PRs?

To me former sounds safer in general so could go in first, and perhaps released as 6.6.2; and second bigger part separately, ad 6.7.0.
(and both merge up for upcoming 7.0.0).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose if that's what you want.
But I sort of see them as parts of the same thing: these extraneous transitive dependencies and the isorelax not being shaded were all introduced as part of the MSV update from 2013.6.1 to 2022.7.
Compare the output of mvn dependency:tree for Woodstox 6.5.1 vs 6.6.1:

6.5.1:

$ mvn org.apache.maven.plugins:maven-dependency-plugin:3.6.1:tree
[INFO] Scanning for projects...
[INFO] Inspecting build with total of 1 modules...
[INFO] Installing Nexus Staging features:
[INFO]   ... total of 1 executions of maven-deploy-plugin replaced with nexus-staging-maven-plugin
[INFO] 
[INFO] ----------------< com.fasterxml.woodstox:woodstox-core >----------------
[INFO] Building Woodstox 6.5.1
[INFO]   from pom.xml
[INFO] -------------------------------[ bundle ]-------------------------------
[INFO] 
[INFO] --- dependency:3.6.1:tree (default-cli) @ woodstox-core ---
[INFO] com.fasterxml.woodstox:woodstox-core:bundle:6.5.1
[INFO] +- org.codehaus.woodstox:stax2-api:jar:4.2.1:compile
[INFO] +- net.java.dev.msv:msv-core:jar:2013.6.1:compile
[INFO] |  \- com.sun.xml.bind.jaxb:isorelax:jar:20090621:compile
[INFO] +- net.java.dev.msv:xsdlib:jar:2013.6.1:compile
[INFO] +- relaxngDatatype:relaxngDatatype:jar:20020414:compile
[INFO] +- org.osgi:osgi.core:jar:5.0.0:provided
[INFO] +- biz.aQute.bnd:biz.aQute.bnd.annotation:jar:6.4.0:provided
[INFO] |  +- org.osgi:org.osgi.resource:jar:1.0.0:provided
[INFO] |  \- org.osgi:org.osgi.service.serviceloader:jar:1.0.0:provided
[INFO] \- junit:junit:jar:4.13.2:test
[INFO]    \- org.hamcrest:hamcrest-core:jar:1.3:test
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  0.966 s
[INFO] Finished at: 2024-03-22T14:53:25-05:00
[INFO] -----------------------------------------------------

6.6.1:

$ mvn org.apache.maven.plugins:maven-dependency-plugin:3.6.1:tree
[INFO] Scanning for projects...
[INFO] Inspecting build with total of 1 modules...
[INFO] Installing Nexus Staging features:
[INFO]   ... total of 1 executions of maven-deploy-plugin replaced with nexus-staging-maven-plugin
[INFO] 
[INFO] ----------------< com.fasterxml.woodstox:woodstox-core >----------------
[INFO] Building Woodstox 6.6.1
[INFO]   from pom.xml
[INFO] -------------------------------[ bundle ]-------------------------------
[INFO] 
[INFO] --- dependency:3.6.1:tree (default-cli) @ woodstox-core ---
[INFO] com.fasterxml.woodstox:woodstox-core:bundle:6.6.1
[INFO] +- org.codehaus.woodstox:stax2-api:jar:4.2.2:compile
[INFO] +- net.java.dev.msv:msv-core:jar:2022.7:compile
[INFO] |  +- xml-apis:xml-apis:jar:1.4.01:compile
[INFO] |  \- isorelax:isorelax:jar:20030108:compile
[INFO] +- net.java.dev.msv:xsdlib:jar:2022.7:compile
[INFO] |  \- xerces:xercesImpl:jar:2.12.2:compile
[INFO] +- relaxngDatatype:relaxngDatatype:jar:20020414:compile
[INFO] +- org.osgi:osgi.core:jar:5.0.0:provided
[INFO] +- biz.aQute.bnd:biz.aQute.bnd.annotation:jar:6.4.0:provided
[INFO] |  +- org.osgi:org.osgi.resource:jar:1.0.0:provided
[INFO] |  \- org.osgi:org.osgi.service.serviceloader:jar:1.0.0:provided
[INFO] \- junit:junit:jar:4.13.2:test
[INFO]    \- org.hamcrest:hamcrest-core:jar:1.3:test
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  0.955 s
[INFO] Finished at: 2024-03-22T14:54:00-05:00
[INFO] ------------------------------------------------------------------------

Copy link
Member

Choose a reason for hiding this comment

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

Ahhh. Ok, yes, there's not that much point since these we knew unwanted dependencies.
Not something that was already leaked like I assumed for some reason.

So let's go with just this one.

<!-- don't use isorelax version that lacks source code on Maven Central -->
<exclusion>
<groupId>isorelax</groupId>
<artifactId>isorelax</artifactId>
</exclusion>
<!-- xml-apis is unneeded since it repackages XML APIs that are built into the JDK -->
<exclusion>
<groupId>xml-apis</groupId>
<artifactId>xml-apis</artifactId>
</exclusion>
</exclusions>
</dependency>

<dependency>
<groupId>net.java.dev.msv</groupId>
<artifactId>xsdlib</artifactId>
<version>${version.msv}</version>
<!--
<scope>provided</scope>
-->
<optional>true</optional>
<!-- xercesImpl is unneeded since all modern JDKs already include it -->
<exclusions>
<exclusion>
<groupId>xerces</groupId>
<artifactId>xercesImpl</artifactId>
</exclusion>
</exclusions>
</dependency>

<!-- use an isorelax version that has source code on Maven Central -->
<dependency>
<groupId>com.sun.xml.bind.jaxb</groupId>
<artifactId>isorelax</artifactId>
<version>20090621</version>
<!--
<scope>provided</scope>
-->
Expand Down