-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add dependency management and junit4 dependency to parent pom #1912
Conversation
<log4j.version>1.2.17</log4j.version> | ||
<commons-compress.version>1.18</commons-compress.version> | ||
|
||
<!-- for the correct jackson and guava versions see https://github.com/dropwizard/dropwizard/blob/release/1.3.x/dropwizard-bom/pom.xml --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you added this link to the bom file, but did not include the bom file here? It provides the correct version for jackson, guava and many more, see below (maybe too many and it would be more readable to specify these versions explicitly in our dependency management section instead?)
<dependency> | ||
<groupId>org.slf4j</groupId> | ||
<artifactId>slf4j-log4j12</artifactId> | ||
<version>1.7.26</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately this logging bridge is not declared in the dropwizard bom, while for example slf4j-api is declared there (not sure why)
|
||
<dependencies> | ||
<dependency> | ||
<groupId>junit</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this here with the downside that all child projects have this dependency automatically now. Then again junit is used by all children and its only test scope too. With junit 5 this will be multiple dependencies, which otherwise we would have to repeat in every child pom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I did the exact opposite a while ago. I don't oppose at the moment, but let's remember that this is only for convenience: There ist absolutely zero dependency between the test frameworks each submodule uses (each one can use its own), so in a way this is an artificial restriction to save typing. Totally okay as long as one doesn't forget. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes its code re-use via inheritance in a way, use with care... If you want we can of course go back to declaring the dependency in the submodules for the reason you mention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in a way this is an artificial restriction
I don't want to be pedantic but a submodule does not inherit dependencies from a parents dependencyManagement section and is still free to override the dependency versions in its own pom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but the junit dependency is an actual dependency (not dependencyManagement). I think we all agree that using the dependencyManagement in the parent pom makes sense (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but the junit dependency is an actual dependency
Sorry, my bad.
I think we all agree that using the dependencyManagement in the parent pom makes sense
Absolutely
Before 32cfae2, Graphhopper used version variables defined in the <properties>-section of the graphhopper-parent pom.xml. In graphhopper#1906 and PR graphhopper#1912 they decided to switch to maven <dependencyManagement>. This allows to declare versions and then use them in any child pom.xml by only stating group- and artifact Id of the dependency. Although the two dependencies are only used in graphhopper-core, the versions are defined consistently in the graphhopper-parent xml.
Before 32cfae2, Graphhopper used version variables defined in the <properties>-section of the graphhopper-parent pom.xml. In graphhopper#1906 and PR graphhopper#1912 they decided to switch to maven <dependencyManagement>. This allows to declare versions and then use them in any child pom.xml by only stating group- and artifact Id of the dependency. Although the two dependencies are only used in graphhopper-core, the versions are defined consistently in the graphhopper-parent xml.
Fixes #1906.
I moved most dependency versions into parent pom's dependency management section and move the junit 4 dependency there as well. For dropwizard I used their bom file.