Skip to content

Downgrade slf4j to 1.7.36#4023

Merged
adangel merged 3 commits into
pmd:pmd/7.0.xfrom
adangel:pmd7-downgrade-slf4j
Jul 14, 2022
Merged

Downgrade slf4j to 1.7.36#4023
adangel merged 3 commits into
pmd:pmd/7.0.xfrom
adangel:pmd7-downgrade-slf4j

Conversation

@adangel

@adangel adangel commented Jun 24, 2022

Copy link
Copy Markdown
Member

Describe the PR

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by github actions)
  • Added (in-code) documentation (if needed)

@adangel adangel added this to the 7.0.0 milestone Jun 24, 2022
Comment thread pmd-core/src/test/java/net/sourceforge/pmd/cli/CoreCliTest.java Outdated
@adangel adangel mentioned this pull request Jun 24, 2022
8 tasks
@ghost

ghost commented Jun 24, 2022

Copy link
Copy Markdown
2 Messages
📖 Compared to pmd/7.0.x:
This changeset changes 29 violations,
introduces 19 new violations, 0 new errors and 0 new configuration errors,
removes 24 violations, 0 errors and 0 configuration errors.
Full report
📖 Compared to master:
This changeset changes 57095 violations,
introduces 34416 new violations, 2 new errors and 0 new configuration errors,
removes 138133 violations, 25 errors and 6 configuration errors.
Full report
Compared to pmd/7.0.x:
This changeset changes 28 violations,
introduces 17 new violations, 0 new errors and 0 new configuration errors,
removes 21 violations, 0 errors and 0 configuration errors.
Full report
Compared to master:
This changeset changes 57097 violations,
introduces 34421 new violations, 2 new errors and 0 new configuration errors,
removes 138131 violations, 25 errors and 6 configuration errors.
Full report

Generated by 🚫 Danger

@jsotuyod

Copy link
Copy Markdown
Member

I support the idea of not using non-GA dependencies.

Having said that, I don't really like the idea of downstream projects (ie: Sonar) conditioning our usage of libraries. We are otherwise conditioned (and forced) by them on when to upgrade to another major (and due to backwards compatibility issues, there is a good chance the 'when' is actually 'never' for them). Moreover, we may find other downstream projects following a separate schedule.

I see two possible ways forward:

  1. We make a hard stance. We support whichever version we feel is right at any given moment in time and push for downstream project to do proper classloader isolation.
  2. We acknowledge that logging may be an edge case and treat it separately. Doing so would probably require we setup our own PmdLogger abstraction within pmd-core and rescope that module to the pmd engine itself; moving the CLI support to either pmd-dist or a new module, and having that module provide the bridge between that and SLF4J. Downstream projects can therefore depend on pmd-core and provide their own bridge between PmdLogger and whatever backend they are keen on using.

@adangel

adangel commented Jun 30, 2022

Copy link
Copy Markdown
Member Author

I'd rather be pragmatic here - especially as it is logging, which is a cross-cutting concern. I'm not sure if there is even a proper way to do classloader isolation, if every library's log should end up in the same log channel (and sonar already seems to isolate the dependencies of the plugins with the only exception being slf4j).

Creating an own logging abstraction/wrapper doesn't seem to be a good idea either, see https://www.slf4j.org/faq.html#optional_dependency

So I'd acknowledge that logging is an edge case and simply downgrade. It's more important for me, that PMD is working in different environments than insisting on using slf4j2.

@adangel adangel merged commit 0b4718a into pmd:pmd/7.0.x Jul 14, 2022
@adangel adangel deleted the pmd7-downgrade-slf4j branch July 14, 2022 11:37
@adangel adangel mentioned this pull request Jan 23, 2023
55 tasks
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