[ISSUE #4700] Remove logging backends from runtime deps #4719
Conversation
This PR removes all the logging backends from the Maven artifacts. As explained in apache#4700 libraries should only depend on logging APIs, not logging implementations the same way web applications depend on the Servlet API, not its implementations. The logging backends are added to the `dist` folder to be included in the TAR binary distribution.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4719 +/- ##
=========================================
Coverage 17.44% 17.44%
Complexity 1768 1768
=========================================
Files 797 797
Lines 29845 29845
Branches 2563 2563
=========================================
Hits 5206 5206
Misses 24159 24159
Partials 480 480 ☔ View full report in Codecov by Sentry. |
ppkarwasz
force-pushed
the
logging-deps
branch
from
ea87ab8
to
9a7ca67
Compare
ppkarwasz
force-pushed
the
logging-deps
branch
from
9a7ca67
to
6b0c122
Compare
|
Something is wrong with Java 11 tests. I am switching this to a "draft" to work out the issues. |
It seems to be a problem with the third party platform. Not your code. |
|
It has been 60 days since the last activity on this pull request. I am reaching out here to gently remind you that the Apache EventMesh community values every pull request, and please feel free to get in touch with the reviewers at any time. They are available to assist you in advancing the progress of your pull request and offering the latest feedback. If you encounter any challenges during development, seeking support within the community is encouraged. We sincerely appreciate your contributions to Apache EventMesh. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4719 +/- ##
============================================
- Coverage 16.20% 16.20% -0.01%
Complexity 1710 1710
============================================
Files 857 858 +1
Lines 30883 30885 +2
Branches 2685 2694 +9
============================================
Hits 5005 5005
- Misses 25410 25412 +2
Partials 468 468 ☔ View full report in Codecov by Sentry. |
|
Given your explanations above, I'll refactor this PR so that:
Does this sound like a plan? |
There was a problem hiding this comment.
LGTM. Thanks to @ppkarwasz for his contribution over time.
In summary: This PR mainly modernizes the project's dependency on the logging backends and some obsolete gradle syntax.
- The vast majority of the changes to the Gradle tasks are equivalent to their effects before the modifications, the biggest change being the change of the
disttask from a submodule task to a root module task. - For users, the problems mentioned in [Bug] Maven artifacts should not depend on logging backend #4700 (comment) have been solved.
Pil0tXia
added
the
ready for review
There was a problem hiding this comment.
Hi~ I just found that two submodules are missing the dependency on eventmesh-common. It might have been overlooked while resolving #4719 (comment). It would be great if you could kindly consider committing it~
eventmesh-openconnect/eventmesh-openconnect-offsetmgmt-plugin/build.gradle
Outdated
Show resolved
Hide resolved
...ct/eventmesh-openconnect-offsetmgmt-plugin/eventmesh-openconnect-offsetmgmt-api/build.gradle
Outdated
Show resolved
Hide resolved
Pil0tXia
removed
the
ready for review
Pil0tXia
added
the
ready for review
| implementation('org.springframework.boot:spring-boot-starter') { | ||
| exclude module: 'spring-boot-starter-logging' | ||
| } | ||
| implementation 'org.springframework.boot:spring-boot-starter-web' |
* Sync changes in #4719 * minor change * Only keep the artifact name * Run `sed -i 's/-[0-9].*\.jar//g'` * Run `sort known-dependencies.txt | uniq > known-dependencies-unique.txt` * Allow CI to run on branches with namespace in the branch name in forked repos * Correct typo and remove useless command * Use `sort -u -o` instead of `uniq` to remove duplicate artifacts with different version * Enlarge open-pull-requests-limit * minor: polish tips * Test apache/skywalking-eyes/dependency CI result * Fix 'unable to find version `0.6.0`' * See debug log to prove it works * skywalking-eyes/dependency doesn't support gradle, test basic actions/dependency-review-action * Add all denied licenses * Remove redundant check * Remove not included SPDX: ASL, RSAL * Add a useful printAllDependencyTrees task * Exampt safe artifact under multiple licenses * Exempt more safe artifacts (Looks like the last of them) * 'allow-dependencies-licenses' attribute only supports single-line text * Add a TODO comment * Add more file extensions for checkstyle * Resolve some checkstyle header violations * Add back apache/skywalking-eyes * Fix downloaded file didn't have a `.` * Disable Go deps update & Must pass CI before merge * No need to force up-to-date & Auto-approve only * Remove the slash at the end of the homepage url in Repo GitHub desc * Skip patch updates temporarily to reduce PR noise * Logback removed after be06ef7 * Accept patch update * Submit dependency graph * Follow https://github.com/gradle/actions/blob/main/docs/dependency-submission.md#usage-with-pull-requests-from-public-forked-repositories * try to sort dependency graph workflow exec seq * `workflow_run` event will only trigger a workflow run if the workflow file is on the default branch * Grant required permission of CodeQL * Attempt to fix 'No dependency graph files found to submit' * Attempt to fix 'No dependency graph files found to submit' try 2 * Attempt to fix 'No dependency graph files found to submit' try 3 * Attempt to fix 'No dependency graph files found to submit' try 4 * Try to check dependency-review * Only check bundled dependencies * Fix 'No snapshots were found for the head SHA' attempt 1 * Test runtimeClasspath dependencies * Revert "Test runtimeClasspath dependencies" This reverts commit 3de89a5. * Try to retry 1 hr wo wait for snapshot update * Test gradle/actions#196 (comment) * Add todo comments * Keep implementation and compileOnly for now * Keep runtimeOnly deps * [Breaking Change] Remove dependency-review-action and wait for its bugfix * Add checkDeniedLicense into CI * minor code optimization
Fixes #4700.
Motivation
As explained in #4700 libraries should only depend on logging APIs (
log4j-api,slf4j-api), not logging implementations (log4j-core,logback-classic).This allows application developers to add the logging backend of their choice, without having to exclude multiple transitive dependencies.
Modifications
This PR:
runtimeClasspathtotestRuntimeOnlyin EventMesh artifacts,disttask to include Log4j Core and bridges from SLF4J and JUL to the Log4j API.Documentation