Initial migration of maven-shaded-log4j-transformer #2
Conversation
|
Fantastic work @edwgiz, thanks so much! One more question: I am a |
log4j-maven-plugins/log4j-maven-plugins-shade-transformer/src/site/markdown/index.md
Show resolved
Hide resolved
|
|
||
| ## Overview | ||
|
|
||
| This module includes the transformer for [Apache Maven Shade Plugin](https://maven.apache.org/plugins/maven-shade-plugin/), that concatenates `Log4j2Plugins.dat` files, |
There was a problem hiding this comment.
Before technically explaining what the transformer does, maybe it is good to have some introduction to the problem: "Log4j 2 is composed of plugins and they are loaded from Log4j2Plugins.dat file found in the classpath at runtime. Shading overrides the cache files provided by each individual module. For instance, -web and -core, etc. ... So if you happen to shade libraries providing Log4j 2 plugins, you need this thing." Something along these lines.
It is also important to keep the Log4j 2 emphasis here. Since, 1) Log4j 1 doesn't have plugins and 2) Log4j 3 plugins doesn't suffer from this problem.
There was a problem hiding this comment.
a) the comments can be improved after the PR is pushed. b) There is no Log4j 3. It will be Log4j 2 3.0
log4j-maven-plugins/log4j-maven-plugins-shade-transformer/src/site/site.xml
Show resolved
Hide resolved
| <version>2.14.1-SNAPSHOT</version> | ||
| <relativePath>../</relativePath> | ||
| </parent> | ||
| <artifactId>log4j-maven-plugins-shade-transformer</artifactId> |
There was a problem hiding this comment.
I think this is a good spot to talk about the module naming and hiearchy. @edwgiz, I see that you have created a nested module structure: log4j-maven-plugins-shade-transformer under log4j-maven-plugins. I share my remarks below.
- I see some things are named
shadeand someshaded. I think we need to stick to one. - I don't think there is a need for a nested structure;
log4j-maven-plugins-shade-transformerat the top is good enough. - I propose naming the module to
log4j-maven-shade-plugin-extensions. Because,log4j-prefix in the artifact IDs is a convention we stick to everywhere, I don't see why this needs to be an exception.- I see
-extensionssuffix is used by others extending Maven Shade plugin too.
- Whatever the consensus on the module name is, JPMS module name and Java package path need to reflect that too.
@jvz, @rgoers, @garydgregory, comments?
There was a problem hiding this comment.
Typically, the groupId is org.apache.logging.log4j for everything related to log4j. I'm not sure we need the maven distinction on the groupId.
I am fine with the package name being org.apache.logging.log4j.maven.
I am fine with the artifactId. The module should be the same (which it is).
I am fine with nesting this plugin under log4j-maven-plugins. Although we may never have another Maven plugin I see no reason to preclude it.
This jar doesn't need JPMS support. It is a jar to be added to the maven shade plugin, which doesn't rely on JPMS (thank goodness). That said, the package name should not overlap any other potential maven plugins, so the full package name of org.apache.logging.log4j.maven.shaded.transformer is correct, unless we decide that "shade" should be used everywhere.
|
Thanks for positive feedback
Without MANIFEST.MF the build fails
[ERROR] Failed to execute goal
org.apache.maven.plugins:maven-jar-plugin:3.2.0:jar (default-jar) on
project log4j-maven-plugins-shade-transformer: Error assembling JAR:
Manifest file:
logging-log4j-tools/log4j-maven-plugins/log4j-maven-plugins-shade-transformer/target/classes/META-INF/MANIFEST.MF
does not exist
If you have any idea how to gracefully deal with the error I will
appreciate it
…--
Best regards,
Ed
On Tue, Oct 19, 2021 at 11:29 PM Volkan Yazıcı ***@***.***> wrote:
Fantastic work @edwgiz <https://github.com/edwgiz>, thanks so much!
One more question: I am a MANIFEST.MF noob, is the empty file necessary?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOPG2RJ2YXHRKTCEY6SPP4TUHXILPANCNFSM5GIL5BUA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Maven project structure simplification; Artifact and java package renaming; Documentation improvement
|
Sorry it took us so much time to merge your PR. In the meantime we rearranged Github repositories a bit. Would you mind resubmitting your plugin to |
|
Hello, @ppkarwasz That's good news, I will prepare the merge this week |
|
This PR was moved to logging-log4j-transform#2. |
4 participants
No description provided.