-
-
Notifications
You must be signed in to change notification settings - Fork 427
Add ShadingModule support for dependency relocation (Issue #3815) #6520
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.
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
base: main
Are you sure you want to change the base?
Conversation
This implements comprehensive shading support for Mill: - ShadingModule trait with shadedMvnDeps, shadeRelocations, shadedJar tasks - ShadingPublishModule for publishing with shaded deps filtered from POM - ScalaShadingModule and ScalaShadingPublishModule for Scala projects - Example builds for both Java and Scala - Unit tests for shading functionality - Documentation updates 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add compileClasspath override to include shadedJar, allowing source code to import shaded package names (e.g., `import shaded.gson.Gson`) - Fix jar task to use Assembly.create instead of Jvm.createJar, which properly extracts and merges shaded JAR contents instead of adding the JAR as an opaque file - Update example build.mill files with proper SNIPPET markers so Java example correctly overrides Scala upstream's test commands Fixes the shading implementation for issue com-lihaoyi#3815. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
CI Failure AnalysisThe 3 failing jobs appear to be infrastructure issues unrelated to this PR:
The shading-specific tests all passed:
Happy to address any actual issues if they come up after a re-run. |
|
Please write a proper PR description explaining what the new feature is and how to use it |
I have updated the PR accordingly. Let me know if you need anything else. |
|
Please cut all the AI slop from the PR description, it is unhelpful |
|
One thing still missing is an explanation of the design and limitations of your PR and what alternatives were considered and why this one was chosen |
|
What happens if you publish a ShadingModule? Do its mvnDeps get vendored? what about its moduleDeps? and what about the transitive versions of each? |
|
Could you comment on how Maven/Gradle/SBT manage shading, and how your approach implemented here compares to what they do |
|
Let me know if you need any further changes :) |
|
Why are the |
…raits Scala users can just import from mill.javalib directly, consistent with how PublishModule works (no ScalaPublishModule exists). Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
They're not needed. Removed them - Scala users can just |
- Add validation warning when shadedMvnDeps is non-empty but shadeRelocations is empty - Add tests for compileClasspath override - Add tests for ShadingPublishModule.publishXmlDeps filtering - Add tests using ShadingNoRelocationsModule - Add foo.run to example tests for runtime verification - Fix scaladoc accuracy in ShadingPublishModule (jar override, not localClasspath) - Improve documentation for shadeRelocations (wildcard syntax, edge case behavior) - Improve documentation for compileClasspath (clarify source code MUST use relocated names) - Revert spurious trailing newline in ScalaModule.scala Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Pushed additional improvements:
|
|
I'm thinking whether or not it's better for In that world, What do you think? |
|
Interesting idea. The standalone approach would give cleaner separation and follows the BomModule pattern. A few questions/thoughts: Pros I see:
Questions:
I can see the standalone approach being cleaner architecturally. Happy to refactor if you think that's the better direction. |
|
How do Maven/Gradle/SBT's shading work with upstream dependencies and publishing? |
|
Also please link to your sources in the maven/gradle/sbt docs |
How Maven/Gradle/SBT Handle Shading & PublishingMaven Shade PluginApproach: Shading is configured directly in the module being published (same as current PR). POM Handling: Automatically generates a
Docs: Gradle Shadow PluginApproach: Shading configured in the module being published via POM Handling: Uses separate dependency configurations:
The Docs: SBT AssemblyApproach: Recommends a separate subproject for shading (similar to your suggestion). From the sbt-assembly README:
POM Handling: No automatic POM generation - requires manual Docs: Summary
The current PR follows the Maven/Gradle pattern. The standalone Trade-off: The mixed approach (current PR) is simpler for the common case. The standalone approach offers better reusability and separation but requires more boilerplate for simple use cases. Happy to refactor to the standalone pattern if you think that's the better direction for Mill. |
|
IIUC, the current implementation can already be used for both scenarious. The user either mixes |
Adds
ShadingModuleandShadingPublishModuletraits for dependency shading (relocation).Usage
For publishing (filters shaded deps from POM):
For Scala modules, import from
mill.javalib:What Gets Shaded
mvnDepsshadedMvnDepsmvnDepsshadedMvnDepsmoduleDepsOnly
shadedMvnDepsand their transitives are vendored.moduleDepsare unaffected.Comparison with Maven/Gradle/SBT
Gradle Shadow (closest to this approach):
implementation(bundled, excluded from POM) vsshadow(in POM, not bundled)shadedMvnDeps≈ Gradle'simplementationin shadow contextMaven shade-plugin:
<includes>/<excludes>filtersSBT sbt-assembly:
assembly, not regularpackagejarto support library publishingDesign
Reuses
Assembly.Rule.Relocateand jarjar-abrams for bytecode transformation:shadedMvnDepsresolved with transitives →resolvedShadedDepsshadedJarapplies relocation via jarjar-abramscompileClasspath/runClasspath/localClasspathto swap original JARs for shaded JARShadingPublishModulefilters shaded artifacts frompublishXmlDepsWhy two traits?
ShadingModulehandles the shading mechanics.ShadingPublishModuleadds POM filtering. Separating them allows shading for internal use (assembly, run) without requiring publish setup.Limitations
shadedMvnDepsare shaded togethermvnDepsandshadedMvnDeps, behavior is undefined (user should avoid this)moduleDepscontains aShadingModule, each module shades independently (no composition)Alternatives Considered
Closes #3815