-
Notifications
You must be signed in to change notification settings - Fork 371
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
Update to use BOM #273
Update to use BOM #273
Conversation
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.
OK but could be further simplified by removing mention of transitive dependencies. (Generally good style to keep them when used during compilation, as for example structs
for @Symbol
even though it would be pulled in anyway.)
pom.xml
Outdated
<version>5</version> | ||
<scope>import</scope> | ||
<type>pom</type> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.jenkins-ci</groupId> | ||
<artifactId>annotation-indexer</artifactId> |
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.
Note: probably superseded by use of core BOM in 4.x parent. @jtnord could confirm
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 don't know what this means.
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.
That you would pick up https://github.com/jenkinsci/jenkins/blob/c376ffe0a41ff4a2100d41ca679a2c7995557dfd/bom/pom.xml#L168-L172 if you updated to the beta parent and got jenkinsci/plugin-pom#269. Can ignore for now.
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.
should not be required in latest parent pom
@@ -26,7 +26,6 @@ | |||
<hpi.compatibleSinceVersion>2.2.0</hpi.compatibleSinceVersion> | |||
<java.level>8</java.level> | |||
<jenkins.version>2.150.3</jenkins.version> | |||
<scm-api.version>2.6.3</scm-api.version> | |||
<hamcrest.version>2.2</hamcrest.version> | |||
<useBeta>true</useBeta> | |||
<jcasc.version>1.35</jcasc.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.
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.
So, should we wait on this change until that pr is resolved?
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.
No, just FYI.
Is there way to determine this without careful consideration of each dependency? I removed structs and everything still built fine, but it sound like I should put it back? |
Yes but I would not worry about it. If and when we strengthen the parent POM to require explicit dependencies for types used during compilation (as for example the Maven harness for NetBeans modules does), you would be informed via build errors of issues. For now this is just stylistic. |
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.
+7 -111, nice. :-)
pom.xml
Outdated
@@ -26,7 +26,6 @@ | |||
<hpi.compatibleSinceVersion>2.2.0</hpi.compatibleSinceVersion> | |||
<java.level>8</java.level> | |||
<jenkins.version>2.150.3</jenkins.version> | |||
<scm-api.version>2.6.3</scm-api.version> | |||
<hamcrest.version>2.2</hamcrest.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.
It is possible this is obsolete, too. Check whether you can delete all mention of Hamcrest.
Leave it to the bom to decide.
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.
Would be nice to merge this, 👍
I was too slow and need to go back and fix the merge. |
No description provided.