-
Notifications
You must be signed in to change notification settings - Fork 179
update min Java version, Maven dependencies and plugins #92
Conversation
Signed-off-by: Vincent Privat <[email protected]>
Signed-off-by: Vincent Privat <[email protected]>
Signed-off-by: Vincent Privat <[email protected]>
Travis build if failing for OpenJDK7 only. Do you want to keep it, or could we drop it and keep only support of OpenJDK8+? |
@@ -383,6 +384,6 @@ public void onLoad(Run<?, ?> run) { | |||
|
|||
@Override | |||
public Collection<? extends Action> getProjectActions() { | |||
return jacocoProjectActions; | |||
return jacocoProjectAction != null ? Arrays.asList(jacocoProjectAction) : Collections.emptyList(); |
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 would use Collections.singletonList() here instead of Arrays.asList()
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
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.
done
@@ -33,10 +34,10 @@ | |||
private String[] includes; | |||
private String[] excludes; | |||
|
|||
private ExecutionDataStore executionDataStore; | |||
private SessionInfoStore sessionInfoStore; | |||
private transient ExecutionDataStore executionDataStore; |
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.
Can you add a comment why "transient" is necessary/useful here?
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.
Sure. It's because of the switch to org.jenkins-ci.plugins 3.0, which defines a FindBugs analysis.
pom.xml
Outdated
@@ -97,7 +97,10 @@ THE SOFTWARE. | |||
<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding> | |||
<project.build.outputEncoding>UTF-8</project.build.outputEncoding> | |||
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | |||
<ant.version>1.9.2</ant.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.
Why do we need a dependency on Ant 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.
It was to fix this error, but it's probably not the best way:
[INFO] --- maven-enforcer-plugin:3.0.0-M1:enforce (display-info) @ jacoco ---
[INFO] Ignoring requireUpperBoundDeps in org.kohsuke:access-modifier-annotation
[WARNING] Rule 6: org.apache.maven.plugins.enforcer.RequireUpperBoundDeps failed with message:
Failed while enforcing RequireUpperBoundDeps. The error(s) are [
Require upper bound dependencies error for org.apache.ant:ant:1.8.4 paths to dependency are:
+-org.jenkins-ci.plugins:jacoco:2.3-SNAPSHOT
+-org.jenkins-ci.main:jenkins-core:2.54
+-org.apache.ant:ant:1.8.4
and
+-org.jenkins-ci.plugins:jacoco:2.3-SNAPSHOT
+-org.jenkins-ci.plugins:dashboard-view:2.9.11
+-org.jenkins-ci.main:maven-plugin:2.13
+-org.jenkins-ci.main.maven:maven-agent:1.7
+-org.apache.ant:ant:1.8.4
and
+-org.jenkins-ci.plugins:jacoco:2.3-SNAPSHOT
+-org.jenkins-ci.plugins:dashboard-view:2.9.11
+-org.jenkins-ci.main:maven-plugin:2.13
+-org.jenkins-ci.lib:lib-jenkins-maven-embedder:3.11
+-org.apache.ant:ant:1.9.2
pom.xml
Outdated
</dependency> | ||
<dependency> | ||
<groupId>xerces</groupId> | ||
<artifactId>xercesImpl</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.
Xerces dependency should not be needed since Java 7, why is it necessary here?
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.
To fix this error but I should find another way:
[INFO] --- maven-enforcer-plugin:3.0.0-M1:enforce (display-info) @ jacoco ---
[INFO] Ignoring requireUpperBoundDeps in org.kohsuke:access-modifier-annotation
[WARNING] Rule 6: org.apache.maven.plugins.enforcer.RequireUpperBoundDeps failed with message:
Failed while enforcing RequireUpperBoundDeps. The error(s) are [
Require upper bound dependencies error for xerces:xercesImpl:2.9.1 paths to dependency are:
+-org.jenkins-ci.plugins:jacoco:2.3-SNAPSHOT
+-org.apache.maven.reporting:maven-reporting-impl:2.3
+-org.apache.maven.doxia:doxia-core:1.2
+-xerces:xercesImpl:2.9.1
and
+-org.jenkins-ci.plugins:jacoco:2.3-SNAPSHOT
+-org.jenkins-ci.main:jenkins-test-harness:2.32
+-org.jenkins-ci.main:jenkins-test-harness-htmlunit:2.18-1
+-xerces:xercesImpl:2.11.0
and
+-org.jenkins-ci.plugins:jacoco:2.3-SNAPSHOT
+-org.jenkins-ci.main:jenkins-test-harness:2.32
+-org.jenkins-ci.main:jenkins-test-harness-htmlunit:2.18-1
+-net.sourceforge.nekohtml:nekohtml:1.9.22
+-xerces:xercesImpl:2.11.0
pom.xml
Outdated
<version>${ant.version}</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>commons-io</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 don't see any usages of commons-io in the changed code, why is the dependency necessary 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.
Same reason than ant, but not needed anymore after a new update
pom.xml
Outdated
@@ -195,30 +215,76 @@ THE SOFTWARE. | |||
<version>3.5</version> | |||
<scope>test</scope> | |||
</dependency> | |||
<dependency> | |||
<groupId>org.apache.maven.reporting</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.
Why do we need all these Maven dependencies now? Where do we actually use functionality from them?
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.
Because of the switch to org.jenkins-ci.plugins 3.0, which defines a very strict maven enforcer configuration. If not met, the build fails. The enforcer rule causing all of this is "Require Upper Bound Dependencies".
@@ -137,7 +131,7 @@ private static void summarize(Map<String, JacocoCoverageResultSummary> summaries | |||
|
|||
JacocoCoverageResultSummary jacocoCoverageResult = getResult(run); | |||
|
|||
String date = DATE_FORMAT.format(runDate.getTime()); | |||
String date = new SimpleDateFormat("yyyy-MM-dd").format(runDate.getTime()); |
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 you are mixing many different changes here, not only Java 7->8, these seem to be intended to improve concurrent behavior somehow. This makes review a bit harder, would in general be better to send separate PRs for these unrelated changes.
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.
This is not unrelated. Yet again caused by the switch to org.jenkins-ci.plugins 3.0 and the underlying FindBugs analysis. If not fixed, the build fails.
This new enforcer rule is annoying. For example, I am trying to update to parent pom 3.2, and this is what I get:
|
What are the prospects for moving this along? I was making a pass at updating to the newly released JaCoCo 0.8 version it appears that requiring JDK 1.7 compatibility is holding it back |
I'm still waiting for apache/maven-plugin-tools#9 to be merged, I didn't think it would take this long. |
OK it appears the other PR in maven-plugin-tools is not needed anymore, no need to wait for it. |
Looks good now, I expect we will release a major version due to the changes in dependencies. |
@centic9 any idea when that release will happen? |
I tried to release, but as usual the release-steps fail at some point with some permission denied. It seems I am not able to get this solved in a way that works longer than for one release, grrr.... anybody else who knows what to do? [ERROR] Failed to execute goal org.apache.maven.plugins:maven-deploy-plugin:2.8.2:deploy (default-deploy) on project jacoco: Failed to deploy artifacts: Could not transfer artifact org.jenkins-ci.plugins:jacoco:hpi:3.0 from/to maven.jenkins-ci.org (https://repo.jenkins-ci.org/releases): Access denied to: https://repo.jenkins-ci.org/releases/org/jenkins-ci/plugins/jacoco/3.0/jacoco-3.0.hpi, ReasonPhrase: . -> [Help 1] I re-created the settings.xml and checked other things, but to no avail ... |
No description provided.