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.

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

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

Closed
wants to merge 1 commit into from

Conversation

jozic
Copy link
Collaborator

@jozic jozic commented Nov 7, 2023

In #107 scalac-scoverage-plugin artifact was replaced by scalac-scoverage-reporter and thus scalac plugin version can't be resolved properly and causes an NPE (on line 451) when tested on https://github.com/scoverage/scoverage-maven-samples.
This PR attempts to check both plugin and reporter artifacts, to resolve correct version.

@jozic jozic marked this pull request as ready for review November 7, 2023 03:47
@jozic jozic requested review from dalbani and ckipp01 November 7, 2023 03:48
{
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

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.

@jozic
Copy link
Collaborator Author

jozic commented Nov 25, 2023

as i mentioned, this indeed most likely won't work, so I'm trying to drop 2.10/2.11 in #143 to move forward
@ckipp01 please take a look there when you have some time

@jozic jozic closed this Nov 25, 2023
@jozic jozic deleted the use-reporter-artifact-to-avoid-npe branch November 25, 2023 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants