-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Remove dependency on maven-reporting-impl #1121
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.
In addition to our CI, I also did some tests locally and do not observe any differences.
So I really like this change - same functionality, almost the same code, less dependencies 👍
Also I tried to upgrade ECJ and use it for compilation into bytecode version higher than 8
, what fails without this change
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.7.0:compile (default-compile) on project jacoco-maven-plugin: Compilation failure: Compilation failure:
[ERROR] /Users/godin/projects/jacoco/jacoco/jacoco-maven-plugin/target/generated-sources/plugin/HelpMojo.java:[8,215] The package org.w3c.dom is accessible from more than one module: <unnamed>, java.xml
while after this PR succeeds 👍
@marchof are you ok if I merge it without change of org.jacoco.doc/docroot/doc/changes.html
?
<li>Maven plug-in has no dependency on <code>maven-reporting-impl</code> any more | ||
(GitHub <a href="https://github.com/jacoco/jacoco/issues/1121">#1121</a>).</li> |
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 me this is pure internal non-functional change, so I do not think that we need this changelog entry and Git commit message is enough. Similar #645 wasn't mentioned either.
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 it as it solves two tickets. But I also agree with your point of view.
This dependency is not really useful for JaCoCo reports and has several
transitive dependencies where security vulnerabilities have been
reported.
Fixes #641, #920