Repository navigation
Remove the Maven and commons-lang dependencies from the compiler TCK - #499
Merged
Merged
Conversation
The TCK builds a classpath for the compiler under test out of exactly two jars: commons-lang for every module, plus aspectjrt for the AspectJ one. Both are declared dependencies of the modules that need them, so both are already on the test JVM's own classpath by the time the test runs. Looking them up by path inside the local repository was resolution reinvented, and it needed maven-resolver, an injected LocalRepositoryProvider, a DefaultRepositorySystemSession, and a maven.repo.local system property handed down from surefire to do it. Asking the class loader where a class came from replaces all of that. The pom comment asking for exactly this - "it would be preferable for the tests to resolve it directly" - has been there since the dependency was added. plexus-compiler-test now depends on no part of Maven at all. API: getLocalArtifactPath is gone, both overloads, replaced by getJarPath. A subclass wanting a jar on the compiler classpath names a class in it rather than its coordinates. Verified: mvn clean install on JDK 25, whole reactor green. AspectJCompilerTest compiles ExternalDeps.java, which imports commons-lang, and asserts ExternalDeps.class among its outputs - so both classpath entries are load bearing, not merely appended.
The TCK hands the compiler under test a classpath holding one library, so that ExternalDeps.java has something external to resolve. That library was commons-lang 2.6, whose last release was 2011 and which is the last Dependabot alert on this repository that comes from a real dependency. commons-lang3 3.20.0 serves the same purpose: StringUtils.upperCase, the only method the fixture calls, is present in both. Nothing here tests commons-lang itself. plexus-compilers/pom.xml declared it a second time, for a reason its own comment gives - "is required in the local repository" - that stopped being true when the TCK started resolving its classpath through the class loader. The remaining declaration in plexus-compiler-test reaches the compiler modules on its own. Verified: mvn clean install on JDK 25, whole reactor green; dependency:tree across the reactor mentions no commons-lang. AspectJCompilerTest, and the eclipse and javac tests, still assert ExternalDeps.class among their outputs, so the swapped import is genuinely being resolved.
slachiewicz
changed the title
slachiewicz
changed the title
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
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.
This change follows #498, which replaced
maven-corewith the Maven resolver. The resolver isn't needed either.The TCK builds a classpath for the compiler under test from two JAR files: one library for the
ExternalDeps.javatest input to resolve, and theaspectjrtlibrary for the AspectJ module. Each module that needs one of them declares it as a dependency, so both are on the test JVM classpath before the test runs. Locating them by path inside the local repository duplicates that resolution, and it requires an injectedLocalRepositoryProviderobject, aDefaultRepositorySystemSessionobject, and amaven.repo.localsystem property set by the Maven Surefire plugin. Asking the class loader where a class came from replaces all three.A comment beside that dependency asks for this: "it would be preferable for the tests to resolve it directly".
After this change, the
plexus-compiler-testartifact depends on no part of Maven. Its dependencies arejavax.inject,plexus-compiler-api,junit-jupiter-api,plexus-testing,plexus-utils,plexus-xml, and the fixture library.The second commit changes that fixture library from
commons-lang2.6, whose last release was in 2011, tocommons-lang33.20.0. TheStringUtils.upperCasemethod is the only method that theExternalDeps.javafile calls, and both libraries provide it. Theplexus-compilers/pom.xmlfile declares the same library a second time, for a reason its own comment gives: "is required in the local repository". The first commit makes that untrue.Together with #498, this leaves one Dependabot alert on the repository, for the Jenkins
remotinglibrary in an integration-test reproducer POM that the reactor doesn't build.API change: both overloads of
getLocalArtifactPathare replaced by thegetJarPathmethod, which takes a class name rather than artifact coordinates. Both areprotectedmethods on a base class in a published artifact, so a TCK subclass outside this repository needs a one-line change. That's the same consideration as #498, and an argument for releasing this in 2.17.0 rather than in a patch version.Verified:
dependency:treeacross the reactor reports no Maven artifact and nocommons-lang. TheAspectJCompilerTestclass compiles theExternalDeps.javafile and assertsExternalDeps.classamong its outputs, so both classpath entries are in use.This change was created with AI assistance.