-
-
Notifications
You must be signed in to change notification settings - Fork 423
RFC: New transformers #1848
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
RFC: New transformers #1848
Conversation
| * Multiple files with the same path but different content lead to an error. | ||
| */ | ||
| @CacheableTransformer | ||
| public open class DeduplicatingResourceTransformer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of a duplicate of #1548.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of, except that it validates that the content's the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe extend the existing one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add new properties and new logic to that transformer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that PropertiesFileTransformer may not be suitable for reproducible builds, as it relies on j,u.Hashtable (the order of map entries isn't deterministic). Let me see what can be done about that.
| public open class ApacheNoticeResourceTransformer @Inject constructor( | ||
| final override val objectFactory: ObjectFactory, | ||
| ) : ResourceTransformer { | ||
| ) : WithPatternFilterable( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to revert changes in ApacheNoticeResourceTransformer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. I found some dependencies with a legit NOTICE file, with the path /NOTICE (not in META-INF/).
So the idea was to keep the existing patterns, but allow users to override the defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misread... We can extract a common pattern class for these, inspired by #1848 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ref to #1850 for the migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the idea was to keep the existing patterns, but allow users to override the defaults.
Users can use include/setIncludes to do so. Noted include/exclude magic in #1850, may note setIncludes/setExcludes too.
| */ | ||
| @Suppress("unused") | ||
| @CacheableTransformer | ||
| public open class MergeLicenseResourceTransformer @Inject constructor(objectFactory: ObjectFactory) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine to add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create a PR for this first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be addressed into a separate PR for now.
...om/github/jengelman/gradle/plugins/shadow/transformers/MergePropertiesResourceTransformer.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/internal/WithPatternFilterable.kt
Outdated
Show resolved
Hide resolved
...n/com/github/jengelman/gradle/plugins/shadow/transformers/ApacheNoticeResourceTransformer.kt
Outdated
Show resolved
Hide resolved
b70a321 to
665583e
Compare
|
(Just maintaining the branch for this draft PR as a "collection of things to split out".) |
665583e to
93c8ba8
Compare
93c8ba8 to
7475920
Compare
aka: "which dependencies contain the path XYZ"
7475920 to
6b91178
Compare
@Goooler I've created a couple new transformers. Would you mind taking a look which ones you think make sense for the project? I would then create individual PRs including tests for each of those.