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

ARTEMIS-4353 clean up Maven dependencies #4544

Merged
merged 1 commit into from Jul 14, 2023
Merged

Conversation

jbertram
Copy link
Contributor

@jbertram jbertram commented Jul 7, 2023

This commit contains the following changes:

  • eliminate used, undeclared dependencies
  • eliminate unused, declared dependencies
  • fix scope for test dependencies
  • eliminate org.hamcrest completely as its use involved deprecated code as well as dependencies from multiple versions

@jbertram jbertram requested a review from gemmellr July 7, 2023 20:21
@jbertram
Copy link
Contributor Author

jbertram commented Jul 7, 2023

@gemmellr, I know you've worked on the artemis-server test-jar in the past. I added that dependency to a few modules since mvn dependency:analyze was complaining about it. If you think those should be left out let me know.

More generally, I wonder if we shouldn't move all the test support classes (e.g. org.apache.activemq.artemis.tests.util.ActiveMQTestBase) into artemis-test-support and then move all the necessary tests out of artemis-server and into integration-tests so we don't have a circular dependency. As it stands at the moment it's kind of a mess.

@gemmellr
Copy link
Member

I havent actually yet looked at this, but a few comments before I get to it:

  • The commons-configuration2 upgrade mentioned should be probably done separately.
  • Yep, I elected not to have the artemis-server test-jar dep declared everywhere, and isolated it to artemis-test-support to give one place to break it later. It sticks out transitively anyway, and its mainly used within the tests tree so it didnt seem like it was really any nicer listing it.
  • Moving what are really more like integration test base classes such as ActiveMQTestBase into artemis-test-support (or some in artemis-unit-test-support as well) was partof my long term plan yes. Ideally 'actual unit tests' should be in the modules, but these current tests are often wiring so much together I'd question if they they really are even though thats where they live now (not sure 'kind of a mess' really does it justice hehe).

One thing to consider is, moving any tests from e.g artemis-server into e.g integration-tests will reduce the set of tests run in the PR check. They could be retained in the PR run tests (if still seeming suitable) by including them in the integration-tests subset run by surefure when enabling the fast-tests profile (see the pom).

@jbertram jbertram force-pushed the ARTEMIS-4353 branch 2 times, most recently from caf5cde to b33449a Compare July 10, 2023 18:46
@jbertram
Copy link
Contributor Author

I backed out the commons-configuration2 change and sent #4548 instead. Also, I removed any additions of the artemis-server test-jar.

Copy link
Member

@gemmellr gemmellr left a comment

Choose a reason for hiding this comment

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

There are a number of warnings from duplicate definitions that need fixed:

https://github.com/apache/activemq-artemis/actions/runs/5511789477/jobs/10047790267?pr=4544#step:5:9

Warning:
Warning: Some problems were encountered while building the effective model for org.apache.activemq:artemis-log-annotation-processor:jar:2.30.0-SNAPSHOT
Warning: 'dependencyManagement.dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: org.seleniumhq.selenium:selenium-api:jar -> duplicate declaration of version ${selenium.version} @ org.apache.activemq:artemis-pom:2.30.0-SNAPSHOT, /home/runner/work/activemq-artemis/activemq-artemis/pom.xml, line 1057, column 22
Warning:
Warning: Some problems were encountered while building the effective model for org.apache.activemq.tests:soak-tests:jar:2.30.0-SNAPSHOT
Warning: 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: org.apache.activemq.tests:artemis-test-support:jar -> duplicate declaration of version ${project.version} @ line 140, column 19
Warning:
Warning: Some problems were encountered while building the effective model for org.apache.activemq.tests:stress-tests:jar:2.30.0-SNAPSHOT
Warning: 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: org.apache.activemq.tests:artemis-test-support:jar -> duplicate declaration of version ${project.version} @ line 126, column 19
Warning:
Warning: Some problems were encountered while building the effective model for org.apache.activemq.tests:smoke-tests:jar:2.30.0-SNAPSHOT
Warning: 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: org.seleniumhq.selenium:selenium-api:jar -> duplicate declaration of version (?) @ line 171, column 19
Warning: 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: org.apache.activemq.tests:artemis-test-support:jar -> duplicate declaration of version ${project.version} @ line 261, column 19
Warning:
Warning: Some problems were encountered while building the effective model for org.apache.activemq:artemis-pom:pom:2.30.0-SNAPSHOT
Warning: 'dependencyManagement.dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: org.seleniumhq.selenium:selenium-api:jar -> duplicate declaration of version ${selenium.version} @ line 1057, column 22
Warning:
Warning: It is highly recommended to fix these problems because they threaten the stability of your build.
Warning:
Warning: For this reason, future Maven versions might no longer support building such malformed projects.

artemis-cdi-client/pom.xml Outdated Show resolved Hide resolved
artemis-cdi-client/pom.xml Outdated Show resolved Hide resolved
artemis-cli/pom.xml Outdated Show resolved Hide resolved
artemis-junit/artemis-junit-4/pom.xml Outdated Show resolved Hide resolved
artemis-junit/artemis-junit-5/pom.xml Outdated Show resolved Hide resolved
artemis-server/pom.xml Outdated Show resolved Hide resolved
tests/integration-tests-isolated/pom.xml Outdated Show resolved Hide resolved
tests/integration-tests/pom.xml Show resolved Hide resolved
tests/integration-tests/pom.xml Outdated Show resolved Hide resolved
artemis-quorum-ri/pom.xml Outdated Show resolved Hide resolved
tests/soak-tests/pom.xml Outdated Show resolved Hide resolved
tests/stress-tests/pom.xml Show resolved Hide resolved
tests/smoke-tests/pom.xml Show resolved Hide resolved
tests/smoke-tests/pom.xml Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
tests/smoke-tests/pom.xml Outdated Show resolved Hide resolved
tests/soak-tests/pom.xml Outdated Show resolved Hide resolved
@jbertram jbertram force-pushed the ARTEMIS-4353 branch 2 times, most recently from 844da76 to cb7307f Compare July 12, 2023 19:11
pom.xml Outdated Show resolved Hide resolved
This commit contains the following changes:
 - eliminate used, undeclared dependencies
 - eliminate unused, declared dependencies
 - fix scope for test dependencies
 - eliminate org.hamcrest completely as its use involved deprecated code
   as well as dependencies from multiple versions
@gemmellr gemmellr merged commit cce565e into apache:main Jul 14, 2023
6 checks passed
@jbertram jbertram deleted the ARTEMIS-4353 branch September 12, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants