[
https://issues.apache.org/jira/browse/ARTEMIS-4353?focusedWorklogId=870314&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-870314
]
ASF GitHub Bot logged work on ARTEMIS-4353:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 11/Jul/23 12:24
Start Date: 11/Jul/23 12:24
Worklog Time Spent: 10m
Work Description: gemmellr commented on code in PR #4544:
URL: https://github.com/apache/activemq-artemis/pull/4544#discussion_r1259603913
##########
artemis-junit/artemis-junit-5/pom.xml:
##########
@@ -57,12 +57,7 @@
</dependency>
<dependency>
<groupId>org.apache.activemq</groupId>
- <artifactId>artemis-jms-server</artifactId>
- <version>${project.version}</version>
- </dependency>
- <dependency>
- <groupId>org.apache.activemq</groupId>
- <artifactId>artemis-jms-client</artifactId>
Review Comment:
This one is more reasonable since I believe the junit 5 stuff doesnt
actually supply the bits that use the JMS client, right?
(Which is actually really pretty bizarre, given its how we recommend most
users use the broker. I dont think anyone should use most of this artemis-junit
stuff personally.)
##########
artemis-cdi-client/pom.xml:
##########
@@ -99,15 +94,16 @@
<groupId>org.jboss.shrinkwrap</groupId>
<artifactId>shrinkwrap-api</artifactId>
<version>1.2.6</version>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>org.jboss.arquillian.junit</groupId>
<artifactId>arquillian-junit-container</artifactId>
</dependency>
<dependency>
- <groupId>org.apache.activemq</groupId>
- <artifactId>artemis-unit-test-support</artifactId>
- <version>${project.version}</version>
Review Comment:
This needs to stay if the logging impl does. The modules are all configured
with a log4j2.properties file that refers to an appender in
artemis-unit-test-support. It spews a stack when not finding it. Again, Maven
wont see a literal dependency, but its there implicitly.
##########
artemis-cli/pom.xml:
##########
@@ -38,6 +38,10 @@
<artifactId>artemis-jms-client</artifactId>
<version>${project.version}</version>
</dependency>
+ <dependency>
+ <groupId>jakarta.jms</groupId>
+ <artifactId>jakarta.jms-api</artifactId>
+ </dependency>
Review Comment:
If defining it, would make more sense to do it before the client dep above
it that would have already brought it in if not for this dep.
##########
tests/integration-tests-isolated/pom.xml:
##########
@@ -302,93 +133,59 @@
</exclusions>
</dependency>
<dependency>
- <groupId>org.apache.directory.server</groupId>
- <artifactId>apacheds-server-annotations</artifactId>
- <version>${directory-version}</version>
+ <groupId>org.apache.directory.api</groupId>
+ <artifactId>api-ldap-model</artifactId>
+ <version>2.0.0.AM1</version>
Review Comment:
We didnt have a direct dep on this before, so presumably it was transitive?
I'd leave it that way rather than hard code a version; add a version prop or
remove?
##########
tests/integration-tests/pom.xml:
##########
@@ -50,56 +56,61 @@
<groupId>org.apache.activemq</groupId>
<artifactId>artemis-jms-client</artifactId>
<version>${project.version}</version>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.activemq</groupId>
<artifactId>artemis-jms-server</artifactId>
<version>${project.version}</version>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.activemq</groupId>
<artifactId>artemis-ra</artifactId>
<version>${project.version}</version>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.activemq</groupId>
<artifactId>artemis-cli</artifactId>
<version>${project.version}</version>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.activemq</groupId>
<artifactId>artemis-commons</artifactId>
<version>${project.version}</version>
- </dependency>
- <dependency>
- <groupId>org.apache.activemq</groupId>
- <artifactId>artemis-spring-integration</artifactId>
- <version>${project.version}</version>
Review Comment:
If it isnt even being used here, and says in the module it isnt really
needed...is time to nuke it?
##########
tests/integration-tests/pom.xml:
##########
@@ -340,6 +402,19 @@
</exclusions>
</dependency>
+ <dependency>
+ <groupId>org.codehaus.woodstox</groupId>
+ <artifactId>woodstox-core-asl</artifactId>
+ <version>4.4.0</version>
Review Comment:
While we are 'cleaning up deps' and moving the dep declaration, lets at add
a prop for the version as done for some others.
##########
artemis-cdi-client/pom.xml:
##########
@@ -46,11 +46,6 @@
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
- <dependency>
- <groupId>org.apache.logging.log4j</groupId>
- <artifactId>log4j-slf4j-impl</artifactId>
- <scope>test</scope>
- </dependency>
Review Comment:
This should probably stay, there is no SLF4J logging impl present otherwise,
prompting a needless warning and perhaps removing the possibility of actually
logging something. There is not meant to be any compile dependency on this or
its deps (besides slfj4-api), so I'd expect Maven generally wont see it as
being 'used' even when it is.
##########
artemis-quorum-ri/src/test/java/org/apache/activemq/artemis/quorum/DistributedLockTest.java:
##########
@@ -222,7 +219,7 @@ public void timedTryLockFailAfterTimeout() throws
ExecutionException, Interrupte
final long timeoutSec = 1;
Assert.assertFalse(manager.getDistributedLock("a").tryLock(timeoutSec,
TimeUnit.SECONDS));
final long elapsed = TimeUnit.NANOSECONDS.toSeconds(System.nanoTime() -
start);
- assertThat(elapsed, greaterThanOrEqualTo(timeoutSec));
+ Assert.assertTrue(elapsed >= timeoutSec);
Review Comment:
In places like this (and some of the other ones) the hamcrest version is far
better because it tells you what the expected+actual values were when it fails.
Without it, you are left to guess at the detail of why it failed. I'd suggest
preparing a failure message for the assert to compensate, here and in some of
the other cases.
##########
artemis-junit/artemis-junit-4/pom.xml:
##########
@@ -84,6 +79,12 @@
<artifactId>jakarta.jms-api</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.activemq</groupId>
+ <artifactId>artemis-jms-client</artifactId>
+ <version>${project.version}</version>
+ <scope>test</scope>
+ </dependency>
Review Comment:
I think this change is probably wrong and a breaking change. Its not
necessarily there as a compile dep, but as a means of supplying the client. A
key intent of the module is to test with the client and broker (still
supplied), and it has always provided the client. It largely makes sense that
it does. Maven cant tell this.
Its a bit like log4j-slf4j-impl. They had always supplied log4j itself along
when declaring that dep. In 2.19.0 for the new slf4j2 variant of the module,
they didnt...which meant you couldnt just swap the dep over, you had had to
declare that module AND add a log4j2 dep yourself in every module you used
log4j-slf4j2-impl. But the whole point of declaring log4j-slf4j* was always to
use log4j, so it was expected that it be supplied even though its not a compile
dep. That behaviour was restored for the slf4j2 variant for 2.20.0.
##########
artemis-server/pom.xml:
##########
@@ -288,12 +295,29 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>org.mock-server</groupId>
+ <artifactId>mockserver-core</artifactId>
+ <version>${mockserver.version}</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mock-server</groupId>
+ <artifactId>mockserver-client-java</artifactId>
+ <version>${mockserver.version}</version>
+ <scope>test</scope>
+ </dependency>
Review Comment:
This seems likely to re-introduce the json-smart dependency excluded from
the mockserver-netty declaration present above, as the exclusion wont apply to
this separate direct declaration.
Either check and fix up the excludes, or just skip adding these. I would
skip.
Issue Time Tracking
-------------------
Worklog Id: (was: 870314)
Time Spent: 50m (was: 40m)
> Clean up Maven dependencies
> ---------------------------
>
> Key: ARTEMIS-4353
> URL: https://issues.apache.org/jira/browse/ARTEMIS-4353
> Project: ActiveMQ Artemis
> Issue Type: Task
> Reporter: Justin Bertram
> Assignee: Justin Bertram
> Priority: Major
> Time Spent: 50m
> Remaining Estimate: 0h
>
> Over time module dependencies can grow stale with declared dependencies which
> are never used and used dependencies which are not declared (i.e.
> transitive). Such is the case with most of the modules in Artemis.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
