[SPARK-33560][test-maven][BUILD] Add "unused-import" check to Maven compilation process by LuciferYang · Pull Request #30784 · apache/spark

LuciferYang · 2020-12-15T14:34:34Z

What changes were proposed in this pull request?

Similar to SPARK-33441, this pr add unused-import check to Maven compilation process. After this pr unused-import will trigger Maven compilation error.

For Scala 2.13 profile, this pr also left TODO(SPARK-33499) similar to SPARK-33441 because scala.language.higherKinds no longer needs to be imported explicitly since Scala 2.13.1

Why are the changes needed?

Let Maven build also check for unused imports as compilation error.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass the Jenkins or GitHub Action
Local manual test：add an unused import intentionally to trigger maven compilation error.

LuciferYang · 2020-12-15T14:36:12Z

pom.xml

+                  <compilerPlugin>
+                    <groupId>com.github.ghik</groupId>
+                    <artifactId>silencer-plugin_${scala.version}</artifactId>
+                    <version>1.6.0</version>


Missing com.github.ghik:silencer-plugin_2.12.10:1.7.1 in Maven central repository

https://mvnrepository.com/artifact/com.github.ghik/silencer-plugin

LuciferYang · 2020-12-15T14:38:59Z

cc @HyukjinKwon

SparkQA · 2020-12-15T15:24:20Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37437/

SparkQA · 2020-12-15T15:53:48Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37437/

srowen · 2020-12-15T17:02:18Z

pom.xml

+            <plugin>
+              <groupId>net.alchim31.maven</groupId>
+              <artifactId>scala-maven-plugin</artifactId>
+              <version>4.4.0</version>


You don't need to repeat the version

done, Address 36268b4 fix this

pom.xml

srowen · 2020-12-15T17:03:48Z

pom.xml

            <recompileMode>incremental</recompileMode>
            <useZincServer>true</useZincServer>
            <args>
-              <arg>-unchecked</arg>


OK so the idea is we can't apply these to both 2.12 and 2.13? why not unchecked though?

With -unchecked, Scala 2.13 has compilation error as follow:

[ERROR] /spark-source/sql/core/src/test/scala/org/apache/spark/sql/streaming/sources/StreamingDataSourceV2Suite.scala:293: a pure expression does nothing in statement position

So Scala 2.12 use -unchecked, Scala 2.13 use -Wconf:cat=unchecked:wv

Oh yeah, I recall this one has been around forever. But I'm pretty sure another change fixed something just like this by, well, just removing that dangling statement, which doesn't seem to do what it means to anyway. Maybe just fix that here directly rather than remove it?

Address be95a1b fix this

SparkQA · 2020-12-15T17:08:15Z

Test build #132835 has finished for PR 30784 at commit c6d2926.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-12-16T01:10:41Z

Thank you @LuciferYang for tackling this in Maven too. @MaxGekk too FYI.

SparkQA · 2020-12-16T04:00:24Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37455/

SparkQA · 2020-12-16T04:32:21Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37455/

SparkQA · 2020-12-16T05:46:50Z

Test build #132853 has finished for PR 30784 at commit 36268b4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-12-16T06:05:26Z

pom.xml

+                  <arg>-Wconf:cat=scaladoc:wv</arg>
+                  <arg>-Wconf:cat=unchecked:wv</arg>
+                  <arg>-Wconf:cat=feature:wv</arg>
+                  // TODO(SPARK-33499): Enable this option when Scala 2.12 is no longer supported.


I think we should use  XML syntax for comments.

Thx~ Address e4b4e0f fix this

HyukjinKwon · 2020-12-16T06:09:34Z

retest this please

SparkQA · 2020-12-16T06:56:28Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37464/

SparkQA · 2020-12-16T07:02:24Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37461/

SparkQA · 2020-12-16T07:26:47Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37464/

SparkQA · 2020-12-16T07:38:32Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37461/

SparkQA · 2020-12-16T10:10:51Z

Test build #132881 has started for PR 30784 at commit dc58f8a.

SparkQA · 2020-12-16T10:54:56Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37483/

SparkQA · 2020-12-16T11:30:36Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37483/

SparkQA · 2020-12-16T13:52:19Z

Test build #132859 has finished for PR 30784 at commit 0ab5f23.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-16T14:16:06Z

Test build #132862 has finished for PR 30784 at commit e4b4e0f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2020-12-16T15:07:12Z

pom.xml

            <recompileMode>incremental</recompileMode>
            <useZincServer>true</useZincServer>
            <args>
-              <arg>-unchecked</arg>


Oh yeah, I recall this one has been around forever. But I'm pretty sure another change fixed something just like this by, well, just removing that dangling statement, which doesn't seem to do what it means to anyway. Maybe just fix that here directly rather than remove it?

srowen · 2020-12-16T15:08:36Z

pom.xml


    <profile>
      <id>scala-2.12</id>
+      <activation>


This might be problematic - yes it should be the default, but this doesn't activate if any other profiles are activated, which may be surprising. What happens if neither is specified?

Generally the "2.12" configuration goes in the main POM and then is overridden for non-default profiles here. So if 2.13 needs different config, that's fine, but 2.12's config should be up top. If the problem is overwriting Maven config in a profile, I can show you how to do that.

What happens if neither is specified?

The extra args defined in scala-2.12 profile will not appended If not activated by default.

Generally the "2.12" configuration goes in the main POM and then is overridden for non-default profiles here. So if 2.13 needs different config, that's fine, but 2.12's config should be up top. If the problem is overwriting Maven config in a profile, I can show you how to do that.

I'll try to solve it first

What I mean is: you put 2.12's config outside a profile. 2.13's config needs to replace it, and you can set this in the Maven XML a certain way to prevent it from just appending, instead making it replace. Otherwise these args won't be there at all for a 2.12 build where any profile is activated, without activating the 2.12 profile explicitly.

Address 4fe256 changed a way to achieve the purpose, but still unsatisfactory :(

SparkQA · 2020-12-17T15:13:59Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37560/

srowen · 2020-12-17T15:19:06Z

pom.xml

+            <plugin>
+              <groupId>net.alchim31.maven</groupId>
+              <artifactId>scala-maven-plugin</artifactId>
+              <configuration combine.self="override">


do you need combine.self="override" here or on just the children that are overridden?
(But yes this is what I was getting at)

Yes, we need combine.self="override" here(merge is default value), merge and default will introduce additional configuration of compilerPlugins, but com.github.ghik:silencer-plugin_2.13.4:1.6.0 not exists in Maven central repository.

Or we can introduce com.github.ghik:silencer-plugin_2.13.4:1.7.1 manually and remove combine.self="override", although we don't really need i

What about combine.self="override" on compilerPlugins, and leave it empty? does that not effectively remove the config in 2.13?

Address fcad7a4 move combine.self="override" on compilerPlugins and leave it empty. Maven can pass and wait GA sbt build

SparkQA · 2020-12-17T15:46:07Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37560/

SparkQA · 2020-12-17T17:39:53Z

Test build #132948 has finished for PR 30784 at commit e7b9e18.

This patch fails from timeout after a configured wait of 500m.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-17T21:53:02Z

Test build #132957 has finished for PR 30784 at commit be95a1b.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-18T05:05:26Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37586/

SparkQA · 2020-12-18T05:18:06Z

Test build #132986 has finished for PR 30784 at commit fcad7a4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-18T05:35:08Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37586/

srowen · 2020-12-18T12:53:59Z

pom.xml

+              <artifactId>scala-maven-plugin</artifactId>
+              <configuration>
+                <scalaVersion>${scala.version}</scalaVersion>
+                <checkMultipleScalaVersions>true</checkMultipleScalaVersions>


OK now do these settings need to be repeated? I'd have thought they don't now.

Address a8d432f remove redundant configs, seems only Scalac args and compilerPlugins are needed

@srowen Is there anything else that needs to be fixed? thx ~

SparkQA · 2020-12-21T09:58:18Z

Test build #133124 has finished for PR 30784 at commit fe9d279.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-12-21T11:26:22Z

retest this please

SparkQA · 2020-12-21T19:33:01Z

Test build #133151 has finished for PR 30784 at commit fe9d279.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2020-12-22T14:26:39Z

pom.xml

+              <artifactId>scala-maven-plugin</artifactId>
+              <configuration>
+                <args>
+                  <arg>-unchecked</arg>


Can the additional args just be appended in this config?
Or do you need to override them to remove some, thus, need to duplicate some compiler flags here?
This is going to be OK, just making sure we're keeping it as simple as we can.

OK~ Will try it later. a little busy at the moment

if we set <args combine.children="append"> in Scala 2.13, the effective pom will be

<args combine.children="append"> <arg>-unchecked</arg> <arg>-deprecation</arg> <arg>-feature</arg> <arg>-explaintypes</arg> <arg>-target:jvm-1.8</arg> <arg>-Xfatal-warnings</arg> <arg>-Ywarn-unused:imports</arg> <arg>-P:silencer:globalFilters=.*deprecated.*</arg> <arg>-Wconf:cat=deprecation:wv,any:e</arg> ...

The are 3 flags shouldn't be in Scala 2.13

-P:silencer:globalFilters=.*deprecated.*: This is the compilation option for silencer-plugin, compilation will fail because this plugin does not exist in Scala 2.13

-Xfatal-warnings and -Ywarn-unused:imports: unused-imports check not enabled in Scala 2.13, compilation will fail because scala.language.higherKinds no longer needs to be imported explicitly since Scala 2.13.1. Or need to add rules to suppress them.

So I think we may need to keep these duplicate flags here, or I haven't thought of an effective way yet

srowen · 2020-12-23T14:56:41Z

OK that's fine if this is as simple as it can get.

LuciferYang · 2020-12-24T02:39:11Z

thx~ @srowen

srowen · 2020-12-24T14:11:44Z

And should this be in 3.1 - is it probably necessary to build with Scala 2.13?

LuciferYang · 2020-12-25T02:11:16Z

@srowen I think it should be merged to 3.1 too, need me to give an additional pr?

HyukjinKwon · 2020-12-26T07:28:19Z

Yes, merging it into branch-3.1 makes sense to me too. SPARK-33441 was for SBT only (It's fixed in branch-3.1). This is for Maven only. Should be easier to maintain if we merge this to branch-3.1.

srowen · 2020-12-26T23:40:41Z

Merged to master/3.1

…ompilation process ### What changes were proposed in this pull request? Similar to SPARK-33441, this pr add `unused-import` check to Maven compilation process. After this pr `unused-import` will trigger Maven compilation error. For Scala 2.13 profile, this pr also left TODO(SPARK-33499) similar to SPARK-33441 because `scala.language.higherKinds` no longer needs to be imported explicitly since Scala 2.13.1 ### Why are the changes needed? Let Maven build also check for unused imports as compilation error. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? - Pass the Jenkins or GitHub Action - Local manual test：add an unused import intentionally to trigger maven compilation error. Closes #30784 from LuciferYang/SPARK-33560. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Sean Owen <srowen@gmail.com> (cherry picked from commit 37ae0a6) Signed-off-by: Sean Owen <srowen@gmail.com>

LuciferYang · 2020-12-28T01:15:23Z

thx ~ @srowen @HyukjinKwon

add unused-imports check for maven

c6d2926

LuciferYang commented Dec 15, 2020

View reviewed changes

github-actions bot added the BUILD label Dec 15, 2020

srowen reviewed Dec 15, 2020

View reviewed changes

remove dup version

36268b4

format

0ab5f23

HyukjinKwon reviewed Dec 16, 2020

View reviewed changes

HyukjinKwon changed the title ~~[SPARK-33560][BUILD] Add "unused-import" check to Maven compilation process~~ [SPARK-33560][test-maven][BUILD] Add "unused-import" check to Maven compilation process Dec 16, 2020

fix syntax for comments

e4b4e0f

Merge branch 'upmaster' into SPARK-33560

dc58f8a

HyukjinKwon approved these changes Dec 16, 2020

View reviewed changes

srowen requested changes Dec 16, 2020

View reviewed changes

fix unchecked compile error

be95a1b

github-actions bot added SQL STRUCTURED STREAMING labels Dec 17, 2020

srowen reviewed Dec 17, 2020

View reviewed changes

LuciferYang marked this pull request as ready for review December 17, 2020 15:25

move self.overrdie to compileplugins

fcad7a4

srowen reviewed Dec 18, 2020

View reviewed changes

LuciferYang added 2 commits December 21, 2020 09:49

remove redundant config

a8d432f

Merge branch 'upmaster' into SPARK-33560

fe9d279

srowen reviewed Dec 22, 2020

View reviewed changes

srowen closed this in 37ae0a6 Dec 26, 2020

LuciferYang deleted the SPARK-33560 branch June 6, 2022 03:45

Conversation

LuciferYang commented Dec 15, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

LuciferYang Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang commented Dec 15, 2020

Uh oh!

SparkQA commented Dec 15, 2020

Uh oh!

SparkQA commented Dec 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 15, 2020

Uh oh!

HyukjinKwon commented Dec 16, 2020

Uh oh!

SparkQA commented Dec 16, 2020

Uh oh!

SparkQA commented Dec 16, 2020

Uh oh!

SparkQA commented Dec 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Dec 16, 2020

Uh oh!

SparkQA commented Dec 16, 2020

Uh oh!

SparkQA commented Dec 16, 2020

Uh oh!

SparkQA commented Dec 16, 2020

Uh oh!

SparkQA commented Dec 16, 2020

Uh oh!

SparkQA commented Dec 16, 2020

Uh oh!

SparkQA commented Dec 16, 2020

Uh oh!

SparkQA commented Dec 16, 2020

Uh oh!

SparkQA commented Dec 16, 2020

Uh oh!

SparkQA commented Dec 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Dec 15, 2020 •

edited

Loading

LuciferYang Dec 18, 2020 •

edited

Loading