Fix shading of isorelax (#200). #202
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Without this change: With this change: |
norrisjeremy
force-pushed
the
isorelax_shading_6.6
branch
2 times, most recently
from
4bf156f
to
92a8fb8
Compare
| @@ -108,12 +108,38 @@ SAX2 and Stax2 APIs | |||
| <scope>provided</scope> | |||
| --> | |||
| <optional>true</optional> | |||
| <exclusions> | |||
There was a problem hiding this comment.
Maybe add a comment to indicate why these are needed?
There was a problem hiding this comment.
I've added some comments.
To summarize:
- Don't include transient
isorelax:isorelaxdependency that lacks source code on Maven Central, since this implies questionable provenance. - Don't include transient
xml-apis:xml-apisdependency that is repackaging XML APIs (SAX, DOM, etc.) that are built into the JDK itself. - Don't include transient
xerces:xercesImpldependency since all modern JDKs already include it. - Manually specify
com.sun.xml.bind.jaxb:isorelaxsince 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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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] ------------------------------------------------------------------------
There was a problem hiding this comment.
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.
norrisjeremy
force-pushed
the
isorelax_shading_6.6
branch
from
92a8fb8
to
6e08d42
Compare
norrisjeremy
force-pushed
the
isorelax_shading_6.6
branch
from
6e08d42
to
b9b155c
Compare
|
Aside from question of 1 or 2 PRs, one process thing is that I'd need a CLA from: https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf (unless we already got one; only needs to be done once). The usual way is to print it, fill & sign, scan/photo, email to |
I'll review the CLA later tonite or over the weekend and get it sent over. |
|
@cowtowncoder I emailed CLA this morning, please let me know if you did not receive it. |
|
@norrisjeremy For some reason I don't think I see it... |
@cowtowncoder Sorry, I think I typo'd the email address that I sent it to yesterday, do you see it now? |
|
@norrisjeremy Yes, I see it now! |
cowtowncoder
approved these changes
Mar 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix shading of isorelax (#200).