Added support for scalac-scoverage-plugin > 2.0.0 #107

samantmaharaj · 2022-12-14T22:47:12Z

Since the 2.0.0 release, scalac-scoverage-plugin has been split into several modules. This has resulted in breaking changes that meant that the scoverage-maven-plugin could not use any version from the v2 series.

This PR implements the changes needed for compatibility with v2 onwards.

The changes are primarily repackaging changes and modifications needed to cope with scoverage no longer using absolute paths.

Reference to the scoverage changes:
scoverage/scalac-scoverage-plugin#368

samantmaharaj · 2022-12-15T01:12:23Z

This should partially resolve #106, and should resolve #100

RPCMoritz

I found two small items which you might want to look at, before merging.

RPCMoritz · 2023-02-13T14:33:46Z

src/main/java/org/scoverage/plugin/SCoveragePreCompileMojo.java

@@ -457,9 +461,9 @@ private Artifact getScalaScoveragePluginArtifact( String resolvedScalaVersion, S
                    resolvedScalaVersion,  resolvedScalacPluginVersion, scalaMainVersion, resolvedScalacPluginVersion ) );

            // for scalac-scoverage-plugin versions up to 1.4.1


this comment should be amended?

I've modified the code to have specific cases for the versions of scalac-scoverage-plugin that were published.

RPCMoritz · 2023-02-13T14:35:38Z

src/main/java/org/scoverage/plugin/SCoveragePreCompileMojo.java

            Artifact runtimeArtifact = getScalaScoverageRuntimeArtifact( scalaBinaryVersion );

-            if ( pluginArtifact == null )
+            if ( pluginArtifacts == null )


This null check looks redundant - I see no code path which dumps a null into this variable. You may want to check for an empty List though.

I've removed this check now.

ckipp01

Thanks for the pr @samantmaharaj. I just let CI run, but it looks like you have some issues. Could you rebase (I just merged a handful of updates) and then address the failing CI. Feel free to ping me when ready for another review.

…verage-plugin Improved comment explaining the artifact resolution process Removed unnecessary null check

…n or equal.

samantmaharaj · 2023-02-14T20:41:36Z

Thanks for the pr @samantmaharaj. I just let CI run, but it looks like you have some issues. Could you rebase (I just merged a handful of updates) and then address the failing CI. Feel free to ping me when ready for another review.

Hi Chris,

The failing part of the CI is the animal sniffer Java API check. Are we targeting a 1.6 API? If so, I'll modify the code to not use Streams.

ckipp01 · 2023-02-14T20:46:00Z

Are we targeting a 1.6 API? If so, I'll modify the code to not use Streams.

No, we can target >= 8.

samantmaharaj · 2023-02-14T21:43:43Z

pom.xml

@@ -380,7 +380,7 @@ under the License.
                    <configuration>
                        <signature>
                            <groupId>org.codehaus.mojo.signature</groupId>
-                            <artifactId>java16</artifactId>
+                            <artifactId>java18</artifactId>


Switched to targeting Java 1.8

RPCMoritz · 2023-02-15T11:29:15Z

pom.xml

-        <scalac-scoverage-plugin.version>1.4.11</scalac-scoverage-plugin.version>
-        <scalac-scoverage-plugin.scala.version>2.12.15</scalac-scoverage-plugin.scala.version>
+        <scalac-scoverage-plugin.version>2.0.7</scalac-scoverage-plugin.version>
+        <scalac-scoverage-plugin.scala.version>2.13</scalac-scoverage-plugin.scala.version>


I think this may have to be pinned to e.g. 2.13.10, to actually work? (going by available maven coordinates)
You may also want to bump the plugin version to 2.0.8 already

It looks like this is only being used by the reporter down below, which doesn't need to be the exact version of Scala, only the binary 2.12, 2.13, or 3. The actual compiler plugin is what needs to be the full version.

referencing that in the variable name would make it unmistakable.

ckipp01

So the logic that currently exists in resolveScalaVersion() will fall apart with Scala 3 builds since the Scala library version you're searching for will never be 3. You'll need to detect Scala 3 in order to not break Maven builds that are using Scala 3. Furthermore, in that case where a user has Scala 3 you also don't need to include the plugin and instead rely on the actual compiler to generate the coverage data.

I know there might be the desire to split that and the update to 2.0.0 apart, but I think it might be a bit dangerous to do that since the logic currently will break for Scala 3 builds. Unless you think that risk is worth it, and then we can just merge in general support for 2.0.0 only for Scala 2, but we should then issue a warning that Scala 3 isn't yet supported.

ckipp01 · 2023-02-15T13:05:30Z

pom.xml

-        <scalac-scoverage-plugin.version>1.4.11</scalac-scoverage-plugin.version>
-        <scalac-scoverage-plugin.scala.version>2.12.15</scalac-scoverage-plugin.scala.version>
+        <scalac-scoverage-plugin.version>2.0.7</scalac-scoverage-plugin.version>
+        <scalac-scoverage-plugin.scala.version>2.13</scalac-scoverage-plugin.scala.version>


It looks like this is only being used by the reporter down below, which doesn't need to be the exact version of Scala, only the binary 2.12, 2.13, or 3. The actual compiler plugin is what needs to be the full version.

samantmaharaj · 2023-02-16T22:44:56Z

Given there's no support for Scala 3 at the moment, it would probably be better to implement that explicitly in another PR.

ckipp01 · 2023-02-17T06:30:27Z

Given there's no support for Scala 3 at the moment, it would probably be better to implement that explicitly in another PR.

Correct, but we should still warn right now in the case that someone tries this with Scala 3. Or what is the behavior currently? The worse thing would be silent failure because it will just lead to more issues being created and confusion about why it's not working.

RPCMoritz · 2023-02-20T18:32:46Z

src/main/java/org/scoverage/plugin/SCoveragePreCompileMojo.java

+        final ArtifactVersion pluginArtifactVersion = new DefaultArtifactVersion(resolvedScalacPluginVersion);
+        if(pluginArtifactVersion.getMajorVersion() >= 2) {


Suggested change

final ArtifactVersion pluginArtifactVersion = new DefaultArtifactVersion(resolvedScalacPluginVersion);

if(pluginArtifactVersion.getMajorVersion() >= 2) {

final ArtifactVersion pluginArtifactVersion = new DefaultArtifactVersion(resolvedScalacPluginVersion);

if(pluginArtifactVersion.getMajorVersion() > 2) {

throw new IllegalArgumentException("Currently Scala version 3 or above are not supported by this Maven plugin. Detected Scala version from scoverage-plugin-version: " + pluginArtifactVersion.getMajorVersion());

}

if(pluginArtifactVersion.getMajorVersion() == 2) {

I guess something like this would make the failure more evident?

Given there's no support for Scala 3 at the moment, it would probably be better to implement that explicitly in another PR.

Correct, but we should still warn right now in the case that someone tries this with Scala 3. Or what is the behavior currently? The worse thing would be silent failure because it will just lead to more issues being created and confusion about why it's not working.

The current behaviour will be to fail to resolve the scoverage compiler plugin for Scala 3 as no plugins are currently published newer than 2.13.10

(Link to MVNRepository here)

This will cause a build failure. It's not as nice as explicitly notifying the user.

I guess something like this would make the failure more evident?

The artifact version in the test you referenced is the version of the scoverage plugin. The scala version is calculated through resolveScalaVersion().

That function will default to an explicitly configured scala version or a search for the scala standard library. If I understand correctly, the Scala 3 standard library uses different coordinates from the earlier versions org.scala-lang:scala3-library.

If the user is depending on the scala 3 standard library resolveScalaVersion() will fail to detect the scala version resulting in a warning being emitted and the execution of the mojo being skipped.

To me that sounds like acceptable behavior - @ckipp01 since an explicit scala-3-warning would need explicit scala-3 detection, I would leave that to a separate fix?

To me that sounds like acceptable behavior - @ckipp01 since an explicit scala-3-warning would need explicit scala-3 detection, I would leave that to a separate fix?

Sure that would be fine, but I'd for sure want to try to get that in before we attempt to do another release.

ckipp01

Alright, so I'm alright with this with the caveat that I haven't tested it. I don't use Maven at all, so if possible give this a couple 👍🏼 if users out there have actually given this a try.

RPCMoritz · 2023-03-29T10:42:04Z

I built the branch, and tried to run coverage, but the compiler is failing:

[DEBUG] [zinc] Running cached compiler 22a8e87b for Scala compiler version 2.13.10
[DEBUG] [zinc] The Scala compiler is invoked with:
	-Wunused
	-deprecation
	-feature
	-P:scoverage:dataDir:<redacted project dir>\target\scoverage-data
	-P:scoverage:sourceRoot:<redacted project dir>
	-Yrangepos
	-Xplugin:<redacted>\.m2\repository\org\scoverage\scalac-scoverage-plugin_2.13.10\2.0.8\scalac-scoverage-plugin_2.13.10-2.0.8.jar:<redacted>\.m2\repository\org\scoverage\scalac-scoverage-domain_2.13\2.0.8\scalac-scoverage-domain_2.13-2.0.8.jar:<redacted>\.m2\repository\org\scoverage\scalac-scoverage-serializer_2.13\2.0.8\scalac-scoverage-serializer_2.13-2.0.8.jar
	-Xplugin:<redacted>\.m2\repository\org\scalameta\semanticdb-scalac_2.13.10\4.7.1\semanticdb-scalac_2.13.10-4.7.1.jar
	-bootclasspath
	<redacted>\.m2\repository\org\scala-lang\scala-library\2.13.10\scala-library-2.13.10.jar
	-classpath
	<redacted>
[ERROR] : bad option: -P:scoverage:dataDir:<redacted project dir>\target\scoverage-data
[ERROR] : bad option: -P:scoverage:sourceRoot:<redacted project dir>
[ERROR] two errors found
[DEBUG] Compilation failed (CompilerInterface)

because -Xplugin is not platform independent. I proposed a fix.

Once I got that in, made sure to get all the source roots populated, it worked!

RPCMoritz · 2023-03-29T11:31:40Z

src/main/java/org/scoverage/plugin/SCoveragePreCompileMojo.java


-            arg = PLUGIN_OPTION + pluginArtifact.getFile().getAbsolutePath();
+            arg = PLUGIN_OPTION + pluginArtifacts.stream().map(x -> x.getFile().getAbsolutePath()).collect(Collectors.joining(":"));


Make this platform independent:

Suggested change

arg = PLUGIN_OPTION + pluginArtifacts.stream().map(x -> x.getFile().getAbsolutePath()).collect(Collectors.joining(":"));

arg = PLUGIN_OPTION + pluginArtifacts.stream().map(x -> x.getFile().getAbsolutePath()).collect(Collectors.joining(String.valueOf(java.io.File.pathSeparatorChar)));

@samantmaharaj could you amend this change to the PR?

I've pushed that change now.

samantmaharaj · 2023-04-12T21:22:35Z

Hi Rick, I'm travelling this week. I'll update the PR when I get back next week.

…

On Thu, 6 Apr 2023 at 18:18, Rick Moritz ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/org/scoverage/plugin/SCoveragePreCompileMojo.java <#107 (comment)> : > - arg = PLUGIN_OPTION + pluginArtifact.getFile().getAbsolutePath(); + arg = PLUGIN_OPTION + pluginArtifacts.stream().map(x -> x.getFile().getAbsolutePath()).collect(Collectors.joining(":")); @samantmaharaj <https://github.com/samantmaharaj> could you amend this change to the PR? — Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGEVFDIZR5KHEABUTKMTREDW7Z34PANCNFSM6AAAAAAS7BHJOI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

gna-phetsarath · 2023-05-12T13:46:47Z

@samantmaharaj Any updates on this PR? Thanks.

LvffY · 2023-06-07T14:07:54Z

Is there any update on this PR ? I think we're stuck because of this issue :(

samantmaharaj · 2023-06-07T19:35:03Z

I committed the changes related to platform independent paths on the 23rd of May. Probably should have added a comment in the main thread.

To my knowledge, that should resolve the last outstanding issue.

jozic

I've tried this locally on a big project with Scala 2.13.10 and it works as expected

pom.xml

jozic · 2023-07-17T21:54:01Z

@samantmaharaj I'm assuming you also tested that on you project(s)
@ckipp01 could you please re-review and merge if this looks okay?

Thank you both!

ckipp01

Thanks for taking care of this @samantmaharaj!

dalbani · 2023-07-21T13:14:55Z

I'm super excited that this PR has been merged.
What do you think of making a new release? Thanks!

ckipp01 · 2023-07-21T13:35:16Z

It's always been @gslowikowski that has done the releases on these. Would it be possible for you to do a release @gslowikowski ?

jozic · 2023-07-28T21:40:33Z

@gslowikowski would you please be so kind to release a new version? thank you!

LvffY · 2023-08-17T05:04:19Z

@gslowikowski Any update on a new version for this plugin with the changes in this PR ?

dalbani · 2023-09-06T10:53:21Z

It looks like @gslowikowski is not active (on this project) these days, maybe you could take the release over, @ckipp01?
Thanks a lot.

ckipp01 · 2023-09-07T05:31:59Z

It looks like @gslowikowski is not active (on this project) these days, maybe you could take the release over, @ckipp01? Thanks a lot.

On vacation atm. When I get back lemme see what I can do. I should have all the access, just need to slowly go through everything and ensure that I do.

ckipp01 · 2023-09-18T14:03:26Z

Alright, I'm back and just gave this a spin, but something isn't working with the version of Maven I'm using and I have very little bandwith to debug something I don't use. Would someone that wants this out the door be willing to add a maven wrapper to this and instructions on what needs to be done?

dalbani · 2023-09-18T15:47:14Z

Maven wrapper

As in the Maven Wrapper from https://maven.apache.org/wrapper/?
If so, I've created this small PR to set it up: #134.
So you just need to call ./mvnw to use Maven 3.9.4 (latest version).
Or did you mean something else?

ckipp01 · 2023-09-19T08:54:00Z

🤔 I'm having some issues with:

    [INFO] BUILD FAILURE
    [INFO] ------------------------------------------------------------------------
    [INFO] Total time:  3.159 s
    [INFO] Finished at: 2023-09-19T10:49:11+02:00
    [INFO] ------------------------------------------------------------------------
    [ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.0.1:jar (attach-javadocs) on project scoverage-maven-plugin: MavenReportException: Error while generating Javadoc:
    [ERROR] Exit code: 1 - javadoc: warning - The old Doclet and Taglet APIs in the packages
    [ERROR] com.sun.javadoc, com.sun.tools.doclets and their implementations
    [ERROR] are planned to be removed in a future JDK release. These
    [ERROR] components have been superseded by the new APIs in jdk.javadoc.doclet.
    [ERROR] Users are strongly recommended to migrate to the new APIs.
    [ERROR] javadoc: error - invalid flag: -author

However I have no idea why -author keeps getting added to options. I'm on Java 11, and can't get the following goal to work:

org.apache.maven.plugins:maven-javadoc-plugin:3.0.1:jar

Anyone have any ideas on how to get this goal to succeed?

dalbani · 2023-09-19T09:43:13Z

I suggest upgrading to the latest version of the Maven plugin, i.e. 3.6.0, and see if that makes a difference.

dalbani · 2023-09-19T10:02:56Z

I suggest upgrading to the latest version of the Maven plugin, i.e. 3.6.0, and see if that makes a difference.

Nope, in and of itself, this change was not enough and I still got the same error when running ./mvnw javadoc:javadoc.
But with the following changes, it did work:

This UML graph plugin is like 10 years old: https://mvnrepository.com/artifact/org.umlgraph/umlgraph.
So I'm wondering if that's compatible with JDK 11.

PS: <link>https://docs.oracle.com/javase/8/docs/api/</link> probably needs updating as well.

dalbani · 2023-09-19T10:17:14Z

Oh, actually, I see that the CI setup is run with both Java 8 and 17.

@ckipp01: to better understand the situation, what process / command do you use to make the release?

ckipp01 · 2023-09-19T10:34:22Z

I was basically following https://maven.apache.org/guides/mini/guide-releasing.html but got stuck at release:prepare. I'll try ripping out the umlgraph stuff just to unblock this.

#136

dalbani · 2023-09-19T11:08:09Z

Oh, actually, I see that the CI setup is run with both Java 8 and 17.

For the record, running ./mvnw clean javadoc:javadoc works for me with JDK 8.
And that's the minimum version that the POM prescribes for several components:

...
                <plugin>
                    <groupId>org.apache.maven.plugins</groupId>
                    <artifactId>maven-compiler-plugin</artifactId>
                    <version>3.11.0</version>
                    <configuration>
                        <source>1.8</source>
                        <target>1.8</target>
                    </configuration>
                </plugin>
...
                <plugin>
                    <groupId>org.apache.maven.plugins</groupId>
                    <artifactId>maven-javadoc-plugin</artifactId>
                    <version>3.0.1</version>
                    <configuration>
                        <source>1.8</source>
...
                <plugin>
                    <groupId>org.apache.maven.plugins</groupId>
                    <artifactId>maven-pmd-plugin</artifactId>
                    <version>3.20.0</version>
                    <configuration>
                        <excludes>
                            <exclude>org/scoverage/plugin/HelpMojo.java</exclude>
                        </excludes>
                        <targetJdk>1.8</targetJdk>
                    </configuration>
                </plugin>
...
                    <execution>
                        <id>enforce-java</id>
                        <goals>
                            <goal>enforce</goal>
                        </goals>
                        <configuration>
                            <rules>
                                <requireJavaVersion>
                                    <version>1.8</version>
                                    <message>Java 1.8 or later required.</message>
                                </requireJavaVersion>
                            </rules>
                        </configuration>
                    </execution>

So what about simply using JDK 8 to make the release?

ckipp01 · 2023-09-19T11:26:40Z

I'm still hitting walls. At this point, I've sunk way more time into this than I want. @dalbani you are pretty active here, so if you want, I'm happy to add you as a maintainer here and just give you the creds to do the release. Is this something you're open to? If so, shoot me an email at open-source@chris-kipp.io

jozic · 2023-10-25T01:32:55Z

Hey @ckipp01 @dalbani
Any progress on this?
I can try to help if needed.

ckipp01 · 2023-10-25T08:54:25Z

@jozic I just sent you a request to add you to the maven team for scoverage. That should provide the access you need to merge and such. Feel free to shoot me an email if you want to help with the release and I can get you creds.

jozic · 2023-10-29T03:53:13Z

Hey @ckipp01
i've sent you an email to the address in your profile several days ago. Please lmk if i need to use another one. If not, then consider this just as a friendly ping :)

ckipp01 · 2023-10-29T09:12:42Z

👍🏼 send a message to both of you. You should have it and have everything you need.

dalbani · 2023-12-21T16:59:30Z

An immense thank you to @jozic for pulling it off with the release of the SCoverage Maven plugin v2.0.0!
It's our Christmas present a few days earlier 🎁

RPCMoritz reviewed Feb 13, 2023

View reviewed changes

ckipp01 requested changes Feb 14, 2023

View reviewed changes

samantmaharaj added 3 commits February 15, 2023 09:17

Added support for scoverage-compiler-plugin > 2.0.0

213462f

Explicitly handled the cases for the different versions of scalac-sco…

ea48709

…verage-plugin Improved comment explaining the artifact resolution process Removed unnecessary null check

Fixed typo where the major version check should have been greater tha…

63e0174

…n or equal.

samantmaharaj force-pushed the feature/scoverage2-support branch from d4014d0 to 63e0174 Compare February 14, 2023 20:19

Modified animal sniffer to target java 1.8 APIs

034e40c

samantmaharaj commented Feb 14, 2023

View reviewed changes

RPCMoritz reviewed Feb 15, 2023

View reviewed changes

RPCMoritz mentioned this pull request Feb 15, 2023

Bump maven-plugin-plugin.version from 3.6.0 to 3.7.1 #112

Closed

ckipp01 requested changes Feb 15, 2023

View reviewed changes

RPCMoritz reviewed Feb 20, 2023

View reviewed changes

ckipp01 approved these changes Mar 6, 2023

View reviewed changes

RPCMoritz reviewed Mar 29, 2023

View reviewed changes

Made plugin artifacts path separator platform independent

6ec7b6a

jozic approved these changes Jul 17, 2023

View reviewed changes

pom.xml Show resolved Hide resolved

ckipp01 approved these changes Jul 18, 2023

View reviewed changes

ckipp01 merged commit c14debc into scoverage:master Jul 18, 2023

samantmaharaj deleted the feature/scoverage2-support branch July 19, 2023 19:14

jozic mentioned this pull request Nov 7, 2023

Fix NPE by using scalac-scoverage-reporter plugin as a fallback #141

Closed

jozic mentioned this pull request Dec 21, 2023

New release for scoverage 2.x and Scala 3.x #106

Closed

		@@ -457,9 +461,9 @@ private Artifact getScalaScoveragePluginArtifact( String resolvedScalaVersion, S
		resolvedScalaVersion, resolvedScalacPluginVersion, scalaMainVersion, resolvedScalacPluginVersion ) );

		// for scalac-scoverage-plugin versions up to 1.4.1

		final ArtifactVersion pluginArtifactVersion = new DefaultArtifactVersion(resolvedScalacPluginVersion);
		if(pluginArtifactVersion.getMajorVersion() >= 2) {


		arg = PLUGIN_OPTION + pluginArtifact.getFile().getAbsolutePath();
		arg = PLUGIN_OPTION + pluginArtifacts.stream().map(x -> x.getFile().getAbsolutePath()).collect(Collectors.joining(":"));

Added support for scalac-scoverage-plugin > 2.0.0 #107

Added support for scalac-scoverage-plugin > 2.0.0 #107

Conversation

samantmaharaj commented Dec 14, 2022

samantmaharaj commented Dec 15, 2022

RPCMoritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckipp01 left a comment

Choose a reason for hiding this comment

samantmaharaj commented Feb 14, 2023

ckipp01 commented Feb 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckipp01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samantmaharaj commented Feb 16, 2023

ckipp01 commented Feb 17, 2023

RPCMoritz Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckipp01 left a comment

Choose a reason for hiding this comment

RPCMoritz commented Mar 29, 2023 • edited Loading

RPCMoritz Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samantmaharaj commented Apr 12, 2023 via email

gna-phetsarath commented May 12, 2023

LvffY commented Jun 7, 2023

samantmaharaj commented Jun 7, 2023 • edited Loading

jozic left a comment

Choose a reason for hiding this comment

jozic commented Jul 17, 2023

ckipp01 left a comment

Choose a reason for hiding this comment

dalbani commented Jul 21, 2023

ckipp01 commented Jul 21, 2023

jozic commented Jul 28, 2023

LvffY commented Aug 17, 2023

dalbani commented Sep 6, 2023

ckipp01 commented Sep 7, 2023

ckipp01 commented Sep 18, 2023

dalbani commented Sep 18, 2023

ckipp01 commented Sep 19, 2023

dalbani commented Sep 19, 2023

dalbani commented Sep 19, 2023

dalbani commented Sep 19, 2023

ckipp01 commented Sep 19, 2023 • edited Loading

dalbani commented Sep 19, 2023 • edited Loading

ckipp01 commented Sep 19, 2023

jozic commented Oct 25, 2023

ckipp01 commented Oct 25, 2023

jozic commented Oct 29, 2023

ckipp01 commented Oct 29, 2023

dalbani commented Dec 21, 2023

RPCMoritz Feb 20, 2023 •

edited

Loading

RPCMoritz commented Mar 29, 2023 •

edited

Loading

RPCMoritz Mar 29, 2023 •

edited

Loading

samantmaharaj commented Jun 7, 2023 •

edited

Loading

ckipp01 commented Sep 19, 2023 •

edited

Loading

dalbani commented Sep 19, 2023 •

edited

Loading