-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add transitive compileOnlyApi (requires static) dependencies #3450
base: 2.x
Are you sure you want to change the base?
Add transitive compileOnlyApi (requires static) dependencies #3450
Conversation
@jjohannes, I've just released |
@ppkarwasz, shall we also add a test for this to |
7a01da7
to
ae8af12
Compare
What test do you have in mind? Do we test that a certain dependency is in the Gradle |
@vy I rebased the PR and re-tested. I also checked the changes in If it fits, the sample Gradle project I created could be moved to |
@jjohannes, sounds plausible, thanks! We can run |
I would prefer not to use
Since 1 and 2 only differ by the JDK used, we could use the GraalVM JDK for both and merge them. |
What you could do is rename workflows/android-reusable-test.yaml to something like Then you can add another step that runs Gradle against the new example similar to this one that runs the Android example. If that sounds like a good way forward to you, I can provide a (Draft) PR with the new example. |
Sounds good to me. |
@jjohannes, I liked this proposal. Looking forward to your PR. Note that both @ppkarwasz and I are swamped with work at the moment. Please be patient while waiting for feedback and/or reviews. |
Here is the PR for the samples repo: apache/logging-log4j-samples#279 @vy no problem. There is no urgency from my side. Let me know if I can help with anything else. |
See apache#3437 for details. Signed-off-by: Jendrik Johannes <[email protected]>
ae8af12
to
6d5f2db
Compare
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.
@jjohannes, thanks so much for the fix!
@ppkarwasz, changes LGTM. Would you mind also skimming through, please?
src/changelog/.2.x.x/3437_transtive_compile_only_dependencies.xml
Outdated
Show resolved
Hide resolved
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.
Looks good to me.
There is one missing annotation library and we are good to go!
biz.aQute.bnd.annotation, | ||
com.google.errorprone.annotations, | ||
com.github.spotbugs.annotations, | ||
org.osgi.annotation.bundle |
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.
Can you add org.osgi.annotation.versioning
too?
Co-authored-by: Volkan Yazıcı <[email protected]>
This pull requests adds
compileOnlyApi
dependencies (Gradle Module Metadata) andrequires static
(module-info) for all annotation libraries used in Log4j. See #3437 for details.The following example can be used to test the effects of the change in a Gradle build on a locally built snapshot:
https://github.com/jjohannes/gradle-demos/tree/main/log4j-metadata
Checklist
2.x
branch if you are targeting Log4j 2; usemain
otherwise./mvnw verify
succeeds (if it fails due to code formatting issues reported by Spotless, simply run./mvnw spotless:apply
and retry)src/changelog/.2.x.x
directory