Fix shading of isorelax (#200). by norrisjeremy · Pull Request #202 · FasterXML/woodstox (original) (raw)

Fix shading of isorelax (#200).

Without this change:

$ jar -tvf target/woodstox-core-6.6.2-SNAPSHOT.jar com/ctc/wstx/shaded/msv/org_isorelax/ com/ctc/wstx/shaded/msv/org_jp_gr_xml/
[no output since isorelax is missing]

With this change:

$ jar -tvf target/woodstox-core-6.6.2-SNAPSHOT.jar com/ctc/wstx/shaded/msv/org_isorelax/ com/ctc/wstx/shaded/msv/org_jp_gr_xml/
     0 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/
     0 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/
  1204 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/IslandSchema.class
   428 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/AttributesDecl.class
   286 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/IslandSchemaReader.class
   785 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/Dispatcher$UnparsedEntityDecl.class
  1091 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/Dispatcher.class
   517 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/SchemaProvider.class
   422 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/ElementDecl.class
   704 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/Dispatcher$NotationDecl.class
     0 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/impl/
  2642 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/impl/IgnoreVerifier.class
  3225 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/impl/IgnoredSchema.class
  1082 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/impl/IgnoredSchema$2.class
  5822 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/impl/DispatcherImpl$Transponder.class
  5635 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/impl/DispatcherImpl.class
  1079 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/impl/IgnoredSchema$1.class
   866 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/impl/DispatcherImpl$Context.class
   296 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/impl/DispatcherImpl$1.class
  2017 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/impl/AbstractSchemaProviderImpl.class
   165 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/AttributesVerifier.class
   542 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/dispatcher/IslandVerifier.class
     0 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/verifier/
   279 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/verifier/VerifierFactoryLoader.class
   268 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/verifier/VerifierFilter.class
  7304 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/verifier/VerifierFactory.class
  1497 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/verifier/VerifierException.class
   275 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/verifier/VerifierHandler.class
     0 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/verifier/impl/
  2973 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/verifier/impl/ForkContentHandler.class
 13073 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/verifier/impl/SAXEventGenerator.class
  5105 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/verifier/impl/VerifierImpl.class
  1005 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/verifier/impl/VerifierImpl$1.class
  3473 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/verifier/impl/VerifierFilterImpl.class
   327 Tue May 14 14:43:10 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/verifier/Schema.class
  1737 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/verifier/VerifierConfigurationException.class
  1277 Tue May 14 14:43:10 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/verifier/Verifier.class
     0 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/jaxp/
  2961 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/jaxp/ValidatingDocumentBuilder.class
  2743 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/jaxp/ValidatingSAXParserFactory.class
  3902 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/jaxp/ValidatingDocumentBuilderFactory.class
  4543 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/jaxp/ValidatingSAXParser.class
     0 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/catalog/
  1334 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/catalog/RELAXCatalog$CatalogHandler.class
  2095 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_isorelax/catalog/RELAXCatalog.class
     0 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/
     0 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/dom/
  8691 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/dom/XMLMaker.class
  1183 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/dom/UDOM.class
  3297 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/dom/UDOMVisitor.class
  1477 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/dom/IDOMVisitor.class
  1733 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/dom/DOMVisitorException.class
     0 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/xml/
  3246 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/xml/UXML.class
     0 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/sax/
  1218 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/sax/LexicalHandlerBase.class
  1333 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/sax/RELAXEntityResolver.class
   921 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/sax/DTDSkipper.class
  3375 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/sax/SimpleEntityResolver.class
 12407 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/sax/DOMSAXProducerVisitor.class
  1139 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/sax/DeclHandlerBase.class
  3058 Tue May 14 14:43:12 CDT 2013 com/ctc/wstx/shaded/msv/org_jp_gr_xml/sax/DOMSAXProducer.class

provided
-->
true

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?

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:

Don't include transient isorelax:isorelax dependency that lacks source code on Maven Central, since this implies questionable provenance.
Don't include transient xml-apis:xml-apis dependency that is repackaging XML APIs (SAX, DOM, etc.) that are built into the JDK itself.
Don't include transient xerces:xercesImpl dependency since all modern JDKs already include it.
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.

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).

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] ------------------------------------------------------------------------

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.

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 cla at fasterxml dot com. With that, I can merge PR(s) and get things moving.

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?

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})