Repository navigation
SCANJLIB-324 Add sonar-scanner-java-library-shaded fat jar for Gradle - #307
Conversation
Gradle doesn't isolate plugins in separate classloaders, so any unshaded dependency of sonar-scanner-java-library (gson, commons-compress, commons-lang3, fastcsv, ayza, BouncyCastle) can collide with a different version already on the build classpath. Maven and the CLI don't have this problem, so instead of shading everything into the one artifact they all share, publish a second, Gradle-only artifact with every dependency relocated under org.sonarsource.scanner.lib.internal. The new lib-shaded module has no sources of its own: it just depends on lib and shades it plus its dependencies with maven-shade-plugin, reusing Alban Auzeill's BouncyCastle relocation findings (the pkcs12 property-name exclude and the Provider services transformer) folded into this single shading module instead of a standalone one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ocation naming The artifactSet's include list only enumerated lib's direct dependencies, silently dropping non-optional transitive compile deps of commons-compress (commons-io, commons-codec) and ayza (sude) from the fat jar: since they matched neither the includes nor the reduced pom's single top-level dependency, they were packaged nowhere, which would fail at runtime with NoClassDefFoundError as soon as commons-compress touches its own org.apache.commons.io usage. Switch to an excludes-based artifactSet (only the provided, non-bundled slf4j-api/jsr305 stay out) so any current or future transitive dependency gets shaded automatically, and add the relocations these newly-included artifacts need, including error_prone_annotations (a compile dependency of gson also missing until now). Also merge the bundled libraries' LICENSE/NOTICE files instead of letting them collide arbitrarily, and align BouncyCastle's relocation target with every other library's org.sonarsource.scanner.lib.internal.shaded prefix now that it's relocated in this unified module rather than a standalone one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
✅ All code review findings resolved.
com.google, org.apache.commons and nl.altindag each currently map to a single library plus its own transitive dependencies (gson + error_prone_annotations, commons-compress/commons-lang3 + their own commons-io/commons-codec, ayza + sude). None of these namespaces overlap with anything else bundled in the jar, so one relocation per namespace covers the same ground as five separate entries while also covering any future transitive dependency added under the same namespace automatically, instead of needing a new relocation entry added by hand every time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…dividually org.bouncycastle.util.Properties defines ~30 system-property names as String constants. The shade plugin rewrites string constants that look like class names, which would rename these properties in the shaded jar but not in the forked scanner-engine JVM they're forwarded to, which runs its own unrelocated BouncyCastle. Some of these properties (x509.*, asn1.*) share their prefix with real BouncyCastle packages containing real classes that do need relocation, so a wildcard exclude would also block those classes. Listing each property as an exact literal avoids that collision.
…cation regression test Many BouncyCastle system-property names are read as literals outside org.bouncycastle.util.Properties too (ASN1Integer, RSAKeyParameters, ECCurve, PKCS1Encoding, the PKCS12/BC/BCFKS keystores, Argon2Parameters, ...). Add the ~28 that were missing from the relocation excludes. Add BouncyCastleRelocationIT, which scans the real bcprov-jdk18on jar for every org.bouncycastle.* string constant that isn't a real class/resource reference and fails the build if it's not excluded, so a future BouncyCastle upgrade adding new properties is caught automatically instead of silently breaking them in the shaded jar.
Despite its name, ApacheLicenseResourceTransformer doesn't merge or dedupe META-INF/LICENSE(.txt|.md) entries: processResource() is a no-op and hasTransformedResource() always returns false, so it unconditionally drops every one it matches - including BouncyCastle's own bundled MIT META-INF/LICENSE.md (verified against the plugin's source). Drop it and let the shade plugin's default last-jar-wins handling take over for the one remaining collision (the identical Apache-2.0 META-INF/LICENSE.txt bundled by commons-compress/io/codec/lang3), the same way the pre-existing META-INF/MANIFEST.MF overlap is already tolerated. ApacheNoticeResourceTransformer genuinely merges NOTICE content, so it's kept.
…BouncyCastleRelocationIT Replaces the manual CONSTANT_Utf8 tag-switch parser with an ASM ClassVisitor that collects LDC-loaded String constants and field ConstantValue defaults. This also makes the test a more faithful mirror of the actual mechanism it's guarding: maven-shade-plugin's own DefaultShader.LazyInitRemapper overrides ASM's Remapper#mapValue to rewrite exactly those two kinds of String constants.
- The META-INF/versions/** filter comment claimed those entries hold
"no functional code". Verified against bcprov-jdk18on-1.85.2: its
META-INF/versions/{17,25} carry classes with no base-tree counterpart,
registered by always-loaded Mappings classes (KEM.ML-KEM, KDF.PBKDF2*/
HKDF-*). Document exactly what's lost and why it's accepted (this
module only uses BouncyCastle for PKCS12 keystore loading).
- The license-transformer comment said the shade plugin's default
resource-merge behaviour is last-jar-wins; DefaultShader actually
keeps the first jar processed and skips later duplicates. Correct the
wording.
Code Review ✅ Approved 7 resolved / 7 findingsAdds a ✅ 7 resolved✅ Bug: artifactSet omits transitive deps: commons-io, commons-codec, sude
✅ Quality: Bundled third-party licenses are not preserved or reported
✅ Quality: BouncyCastle relocation prefix differs from all other relocations
✅ Bug: BC exclude list misses ~28 property names read outside Properties
✅ Quality: ApacheLicenseResourceTransformer drops LICENSE files, not merges them
...and 2 more resolved from earlier reviews OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Summary
sonar-scanner-java-library(gson, commons-compress, commons-lang3, fastcsv, ayza, BouncyCastle) can collide with a different version already on the build classpath. Maven and the CLI don't have this problem.sonar-scanner-java-library-shaded) with every third-party dependency relocated underorg.sonarsource.scanner.lib.internal.libitself is untouched and keeps its plain, unrelocated dependencies for Maven/CLI.lib-shadedmodule has no sources of its own — it depends onliband shades it plus its dependencies withmaven-shade-plugin. BouncyCastle's relocation reuses Alban Auzeill's findings (theorg.bouncycastle.pkcs12.*property-name exclude and theServicesResourceTransformerfor theProviderSPI file), folded into this single shading module rather than a standalone one.Test plan
mvn install -pl lib-shaded --also-makebuilds the new moduleMETA-INF/services/java.security.Provideris correctly rewritten to the relocatedBouncyCastleProviderclass namemodule-info.classandMETA-INF/versions/**are stripped from all bundled libraries (contained only duplicate module descriptors, no functional code)mvn install -pl lib --also-make— confirmedlib's full test suite is unaffected (no source changes)mvn installfrom repo root — full reactor builds🤖 Generated with Claude Code