Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 13 additions & 22 deletions src/main/java/org/scoverage/plugin/SCoveragePreCompileMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,22 +426,27 @@ private void setProperty( Properties projectProperties, String propertyName, Str
}
}

private List<Artifact> getScalaScoveragePluginArtifacts(String resolvedScalaVersion, String scalaMainVersion )
throws ArtifactNotFoundException, ArtifactResolutionException
{
String resolvedScalacPluginVersion = scalacPluginVersion;
if ( resolvedScalacPluginVersion == null || "".equals( resolvedScalacPluginVersion ) )
// if scalacPluginVersion is not set explicitly or previously resolved, try to resolve it using pluginArtifacts
private String getScalacPluginVersion() {
if( scalacPluginVersion == null || scalacPluginVersion.isEmpty())
{
for ( Artifact artifact : pluginArtifacts )
{
if ( "org.scoverage".equals( artifact.getGroupId() )
&& artifact.getArtifactId().startsWith( "scalac-scoverage-plugin_" ) )
&& artifact.getArtifactId().matches("^scalac-scoverage-(plugin|reporter)_.*" ) )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So again I don't fully know the context of how this all works in the Maven plugin, but this smells off to me. Keep in mind that we want and need to resolve the plugin one way and the report/runtime/domain differently. As the plugin is published as a compiler plugin and always needs to be resolved for the exact Scala version being used. The others only need to be the binary version that is being used. So having this check here inside of getScalacPluginVersion seems odd to me.

Copy link
Collaborator Author

@jozic jozic Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking it Chris!
as you know I'm not an original author of maven plugin either, but from my understanding, maven plugin pretends (for the sake of usability) like there were no breaking changes in scalac-scoverge-plugin which it uses under the hood.
Change one it tries to hide is the version of the Scala (exact vs binary/compat, 2.12.18 vs 2.12) for compiler plugin and runtime, and the second change is that scalac-scoverage-plugin has been split into more components (serializer, reporter, domain).
Moreover those components are not backward compatible, so I agree that this smells off. As most likely the full solution will be to use different versions of those components if we still want maven plugin to be able to handle projects with Scala 2.10/2.11.
Or we just say maven plugin also drops Scala 2.10/2.11 and only supported versions of scalac-scoverage-plugin are 2.x.x+
I can be wrong of course, so I would love to hear more opinions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I mean scoverage itself dropped support for 2.11 when I released 2.0. I think that's fine. In my opinion, if people are on that old of a Scala version, they can also just use the old scoverage version. However, that does mean that the plugin needs some detection to know that if it's 2.11 or older it needs to handle resolution one way and anything newer handles it the new way. That's the way we do it in the sbt plugin allowing for both usecases to still work. Ideally we'd do the same here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess i'm missing something... What do you mean by

if it's 2.11 or older it needs to handle resolution one way and anything newer handles it the new way.

?
It seem like if we drop 2.10/2.11 we won't need to resolve anything. And the only reason to check for 2.11 and older is maybe to return a meaningful error suggesting to use older version of the plugin.
For Scala 3 support (which is currently not implemented afaics) we will need to make the difference, but for 2.12/2.13 that should just work, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So regarding the differences in resolution, I have them outlined here. Even though you drop support for 2.11 in theory if the Maven plugin is using an old version of the scoverage plugin, you might still want it to work. That's all I mean. Scala 3 is supported, but only some of the artifacts are needed since the Scala 3 compiler emits coverage data natively.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ckipp01 sorry, but i guess I'm still missing something
from what I see, in sbt-scoverage, there's no way to specify which version of scalac-scoverage-plugin to use. One either uses sbt-scoverage 2.x (which supports only Scala 2.12+) and it can read scoverage reports of version 3.x or they use previous versions if they need Scala 2.11 support. Is that correct?

If so, then it seems we should be doing the same for maven plugin. If someone needs maven scovcerage for Scala 2.11, they can use current version (1.x).If we drop 2.11 we won't need to support scalac-scoverage-plugin lower than 2.x and thus the resolution can be much easier. If someone sets scoverage.scalacPluginVersion explicitly to lower than 2.x, we can fail the build with a suggestion to use an older version of the plugin.

Like now in maven plugin code we have things like

        // There are 3 cases depending on the version of scalac-scoverage-plugin
        // * Version 2.0.0 onwards - 3 artifacts, plugin using resolvedScalaVersion
        // * Version 1.4.2         - 1 artifact, plugin using resolvedScalaVersion falling back to scalaMainVersion
        // * Version 1.4.1 older   - 1 artifact, plugin using scalaMainVersion
        //

if we drop Scala 2.11 and require 2.x of scalac-scoverage-plugin, then no need to think about 1.4.2, 1.4.1 and no need to try reading scoverage reports of different versions (this won't be trivial i think)

Hopefully it makes sense, if not we can consider a quick zoom/google call. If yes, i can come up with another PR dropping 2.11- and scalac-scoverage-plugin 1.x.

{
resolvedScalacPluginVersion = artifact.getVersion();
scalacPluginVersion = artifact.getVersion();
break;
}
}
}
return scalacPluginVersion;
}

private List<Artifact> getScalaScoveragePluginArtifacts(String resolvedScalaVersion, String scalaMainVersion )
throws ArtifactNotFoundException, ArtifactResolutionException
{
String resolvedScalacPluginVersion = getScalacPluginVersion();

// There are 3 cases depending on the version of scalac-scoverage-plugin
// * Version 2.0.0 onwards - 3 artifacts, plugin using resolvedScalaVersion
Expand Down Expand Up @@ -482,23 +487,9 @@ private List<Artifact> getScalaScoveragePluginArtifacts(String resolvedScalaVers
private Artifact getScalaScoverageRuntimeArtifact( String scalaMainVersion )
throws ArtifactNotFoundException, ArtifactResolutionException
{
String resolvedScalacRuntimeVersion = scalacPluginVersion;
if ( resolvedScalacRuntimeVersion == null || "".equals( resolvedScalacRuntimeVersion ) )
{
for ( Artifact artifact : pluginArtifacts )
{
if ( "org.scoverage".equals( artifact.getGroupId() )
&& artifact.getArtifactId().startsWith( "scalac-scoverage-plugin_" ) )
{
resolvedScalacRuntimeVersion = artifact.getVersion();
break;
}
}
}

return getResolvedArtifact(
"org.scoverage", "scalac-scoverage-runtime_" + scalaMainVersion,
resolvedScalacRuntimeVersion );
getScalacPluginVersion());
}

/**
Expand Down