Skip to content
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

SO-6223: Replace log4j with reload4j #1314

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

adamfilep
Copy link
Contributor

No description provided.

@adamfilep adamfilep self-assigned this Aug 14, 2024
Copy link
Member

@apeteri apeteri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@cmark cmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥓

@cmark
Copy link
Member

cmark commented Aug 14, 2024

While this adds reload4j as an additional jar, if there is no use for log4j 1.x jars anywhere then it would be great to completely remove it from the target platform during a Tycho build.
@apeteri @adamfilep could you guys take a look at this as well as part of this PR? Or let me know if this cannot be added.

@apeteri
Copy link
Member

apeteri commented Aug 14, 2024

xtext.common.types has a non-negotiable Import-Package on log4j 1.x:

image

Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.46%. Comparing base (ddf3fb4) to head (9de4670).

Additional details and impacted files
@@            Coverage Diff            @@
##                9.x    #1314   +/-   ##
=========================================
  Coverage     48.46%   48.46%           
- Complexity    14034    14035    +1     
=========================================
  Files          1949     1949           
  Lines         95377    95377           
  Branches      11024    11024           
=========================================
  Hits          46226    46226           
- Misses        46109    46110    +1     
+ Partials       3042     3041    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cmark
Copy link
Member

cmark commented Aug 14, 2024

xtext.common.types has a non-negotiable Import-Package on log4j 1.x:

image

But that is an Import-Package, not a Require-Bundle right? So in theory the reload4j bundle could server the necessary packages to it. Am I missing something?

@apeteri
Copy link
Member

apeteri commented Aug 14, 2024

The only thing you might be missing is that reload4j is referenced in the "dependencies" feature by OSGi bundle ID, which is still org.apache.log4j for maximum compatibility. Excerpt from ~/.m2/repository/ch/qos/reload4j/reload4j/1.2.25/reload4j-1.2.25.jar!/META-INF/MANIFEST.MF:

Manifest-Version: 1.0
Implementation-Title: reload4j
Bundle-Description: Reload4j revives EOLed log4j 1.x
Automatic-Module-Name: ch.qos.reload4j
Bundle-License: http://www.apache.org/licenses/LICENSE-2.0.txt
Bundle-SymbolicName: org.apache.log4j
...

@cmark
Copy link
Member

cmark commented Aug 14, 2024

Ahhh, I see, that was unclear to me, I thought the name of the file is identical with the Bundle-SymbolicName, but this is way better. Now everyone who'd like to depend on log4j can do it, and security scans can also be happy. Yay! Thanks!

@cmark cmark merged commit 7f7656b into 9.x Aug 14, 2024
5 checks passed
@cmark cmark deleted the issue/SO-6223-replace-log4j-with-reload4j branch August 14, 2024 14:41
@apeteri
Copy link
Member

apeteri commented Aug 14, 2024

It is highly likely that Xtext pulled in the very same library with matching content, but for P2 (or other, unknown) reasons the file keeps using org.apache.log4j_<version>.jar as its name. This is the most aggravating part 😄

@cmark
Copy link
Member

cmark commented Aug 14, 2024

Yeah, that's what we need to change, either by renaming it or doing some other magic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants