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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add upper bound to JCL exclusion rule #946

Closed
wants to merge 1 commit into from
Closed

Add upper bound to JCL exclusion rule #946

wants to merge 1 commit into from

Conversation

bdemers
Copy link
Member

@bdemers bdemers commented May 29, 2024

JCL now directly supports slf4j and log4j

We don't use JCL, nor do any of our dependencies, so we might want to consider removing this rule from the build. But, it looks like in the past year or so JCL has been updated to work with slf4j and log4j.

Opening this issue more for discussion than anything, commons-loggin 1.3+ also requires java 1.8, but again, we are not using this dep anyway. My take is we should consider removing this rule 馃

JCL now directly supports slf4j and log4j
@lhazlewood
Copy link
Contributor

The exclusion rule IMO is so that we don't add it in the future either (accidentally). That is, if anyone issues a PR using it, the build should reject it, since if we go with one it'll probably be slf4j. So my initial reaction is to leave it in for 'enforcement'. Thoughts?

@bdemers
Copy link
Member Author

bdemers commented May 30, 2024

I agree, that we would want to reject the PR (and continue to use slf4j), but any PR that adds a dependency we should inspect everything it brings in. For example, IMHO, we would want to block anything that depends on guava.

But... there are probably exceptions: if we started maintaining a Guice module, though we would still want to prevent any other sub module from using guava.

IIRC, the big issue originally with JCL was it order to use it with SLF4J, you needed to use one of the legacy adapters (which reimplemented JCL using the SLF4J API), and we wanted to prevent passing that burden onto the users.

There are many dependencies we don't want to bring in, JCL, is just one of them. I'm not sure this rule provides a lot of value to us any more.

That said, it doesn't really hurt either 馃槃

@lhazlewood
Copy link
Contributor

Makes sense - I don't feel too strongly about it because, as you said, we'd be reviewing the PR for added dependencies anyway.

But since JCL is the only exclusion configured, do you think it'd make sense to remove the enforcer plugin entirely?

@bdemers
Copy link
Member Author

bdemers commented May 30, 2024

馃 I do, one less thing in the build (we can always add it back if we need to)
I'll create another PR for that (and ref this discussion)

@bdemers bdemers closed this May 30, 2024
@bdemers bdemers deleted the dep-ban-jcl branch May 30, 2024 21:27
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.

None yet

2 participants