[AMQ-8472] Fix reload4j scope in activemq-partition module #766
Conversation
|
@cshannon here's a proposed fix |
jbonofre
changed the title
|
@jbonofre - So @gemmellr and I have been talking and this might have already been correct. The issue is apparently the Zookeeper dependencies (and linkedin) have a hard dependency on log4j so it sounds like if someone wants to use this module they actually do need a compile time dependency on reload4j so that it gets pulled in and doesn't break. Robbie can comment as well but I think maybe we just leave it as it already was? It's gone in 5.17.0 anyways Even if we don't make this change at least we can pull in the license file changes (updated years) and re-cut the release since it got cancelled already. |
|
Looking at my build the slf4j stuff is the main culprit of the error:
|
|
Yes I was only talking about reload4j itself, in relation to log4j, that it wasnt a change in behaviour to pass reload4j on as log4j was already being passed on via the 'linkedin'/zookeeper bits before. As the new JIRA didnt mention it, I didnt realise the point actually centred more around the slf4j bit. I'd agree that as the slf4j-log4j12 equivalent wasnt being passed on before the slf4j-reload4j equivalent shouldnt be now. |
activemq-partition/pom.xml
Outdated
| <dependency> | ||
| <groupId>ch.qos.reload4j</groupId> | ||
| <artifactId>reload4j</artifactId> | ||
| <scope>test</scope> |
There was a problem hiding this comment.
Personally I would be fine with test scope if this still works when used (by dependants, not within the build tests), but just to note: this is effectively different than what it did in 5.16.3 before (and different than you said you were going to do on the JIRA) since the module wont pass on reload4j like this anymore, wheras log4j was being passed on before due to the zookeeper related bits that brought it in before (but are now excluded via dependencyManagement).
Ok so maybe we just drop the slf4j part and keep reload4j and let people exclude if they want. |
|
@cshannon yes, it makes sense to me. Let me update the PR. |
|
@jbonofre - i think this is fine so i think you can go ahead with this and re-cut the release |
|
Seems like it would have been simplest to put it at test scope the way it was before, but if the tests pass without it then I guess its no biggie, just means anyone investigating them possibly needing to add it. Though I guess theres probably little chance of anyone bothering in this case, so...meh :) |
|
(The PR title and commit message are wrong though, since the scope of reload4j wasnt changed at all) |
3 participants
No description provided.