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

[SPARK-45144][BUILD] Downgrade scala-maven-plugin to 4.7.1 #42899

Closed
wants to merge 2 commits into from

Conversation

LuciferYang
Copy link
Contributor

@LuciferYang LuciferYang commented Sep 13, 2023

What changes were proposed in this pull request?

This pr downgrade scala-maven-plugin to version 4.7.1 to avoid it automatically adding the -release option as a Scala compilation argument.

Why are the changes needed?

The scala-maven-plugin versions 4.7.2 and later will try to automatically append the -release option as a Scala compilation argument when it is not specified by the user:

  1. 4.7.2 and 4.8.0: try to add the -release option for Scala versions 2.13.9 and higher.
  2. 4.8.1: try to append the -release option for Scala versions 2.12.x/2.13.x/3.1.1, and append -java-output-version for Scala 3.1.2.

The addition of the -release option has caused issues mentioned in SPARK-44376 | #41943 and #40442 (comment). This is because the -release option has stronger compilation restrictions than -target, ensuring not only bytecode format, but also that the API used in the code is compatible with the specified version of Java. However, many APIs in the sun.* package are not exports in Java 11, 17, and 21, such as sun.nio.ch.DirectBuffer, sun.util.calendar.ZoneInfo, and sun.nio.cs.StreamDecoder, making them invisible when compiling across different versions.

For discussions within the Scala community, see scala/bug#12643, scala/bug#12824, scala/bug#12866, but this is not a bug.

I have also submitted an issue to the scala-maven-plugin community to discuss the possibility of adding additional settings to control the addition of the -release option: davidB/scala-maven-plugin#722.

For Apache Spark 4.0, in the short term, I suggest downgrading scala-maven-plugin to version 4.7.1 to avoid it automatic adding the -release option as a Scala compilation argument. In the long term, we should reduce use of APIs that are not exports for compatibility with the -release compilation option due to -target already deprecated after Scala 2.13.9.

Does this PR introduce any user-facing change?

No

How was this patch tested?

  • Pass GitHub Actions
  • Manual check

run git revert 656bf36363c466b60d00452399994ccaaa654ed8 to revert to using Scala 2.13.11 and run dev/change-scala-version.sh 2.13 to change Scala to 2.13

  1. Run build/mvn clean install -DskipTests -Pscala-2.13 -X to check the Scala compilation arguments.

Before

[[DEBUG] [zinc] Running cached compiler 1992eaf4 for Scala compiler version 2.13.11
[DEBUG] [zinc] The Scala compiler is invoked with:
  -unchecked
  -deprecation
  -feature
  -explaintypes
  -target:jvm-1.8
  -Wconf:cat=deprecation:wv,any:e
  -Wunused:imports
  -Wconf:cat=scaladoc:wv
  -Wconf:cat=lint-multiarg-infix:wv
  -Wconf:cat=other-nullary-override:wv
  -Wconf:cat=other-match-analysis&site=org.apache.spark.sql.catalyst.catalog.SessionCatalog.lookupFunction.catalogFunction:wv
  -Wconf:cat=other-pure-statement&site=org.apache.spark.streaming.util.FileBasedWriteAheadLog.readAll.readFile:wv
  -Wconf:cat=other-pure-statement&site=org.apache.spark.scheduler.OutputCommitCoordinatorSuite.<local OutputCommitCoordinatorSuite>.futureAction:wv
  -Wconf:msg=^(?=.*?method|value|type|object|trait|inheritance)(?=.*?deprecated)(?=.*?since 2.13).+$:s
  -Wconf:msg=^(?=.*?Widening conversion from)(?=.*?is deprecated because it loses precision).+$:s
  -Wconf:msg=Auto-application to \`\(\)\` is deprecated:s
  -Wconf:msg=method with a single empty parameter list overrides method without any parameter list:s
  -Wconf:msg=method without a parameter list overrides a method with a single empty one:s
  -Wconf:cat=deprecation&msg=procedure syntax is deprecated:e
  -Wconf:cat=unchecked&msg=outer reference:s
  -Wconf:cat=unchecked&msg=eliminated by erasure:s
  -Wconf:msg=^(?=.*?a value of type)(?=.*?cannot also be).+$:s
  -Wconf:cat=unused-imports&src=org\/apache\/spark\/graphx\/impl\/VertexPartitionBase.scala:s
  -Wconf:cat=unused-imports&src=org\/apache\/spark\/graphx\/impl\/VertexPartitionBaseOps.scala:s
  -Wconf:msg=Implicit definition should have explicit type:s
  -release
  8
  -bootclasspath
...

After

[DEBUG] [zinc] Running cached compiler 72dd4888 for Scala compiler version 2.13.11
[DEBUG] [zinc] The Scala compiler is invoked with:
  -unchecked
  -deprecation
  -feature
  -explaintypes
  -target:jvm-1.8
  -Wconf:cat=deprecation:wv,any:e
  -Wunused:imports
  -Wconf:cat=scaladoc:wv
  -Wconf:cat=lint-multiarg-infix:wv
  -Wconf:cat=other-nullary-override:wv
  -Wconf:cat=other-match-analysis&site=org.apache.spark.sql.catalyst.catalog.SessionCatalog.lookupFunction.catalogFunction:wv
  -Wconf:cat=other-pure-statement&site=org.apache.spark.streaming.util.FileBasedWriteAheadLog.readAll.readFile:wv
  -Wconf:cat=other-pure-statement&site=org.apache.spark.scheduler.OutputCommitCoordinatorSuite.<local OutputCommitCoordinatorSuite>.futureAction:wv
  -Wconf:msg=^(?=.*?method|value|type|object|trait|inheritance)(?=.*?deprecated)(?=.*?since 2.13).+$:s
  -Wconf:msg=^(?=.*?Widening conversion from)(?=.*?is deprecated because it loses precision).+$:s
  -Wconf:msg=Auto-application to \`\(\)\` is deprecated:s
  -Wconf:msg=method with a single empty parameter list overrides method without any parameter list:s
  -Wconf:msg=method without a parameter list overrides a method with a single empty one:s
  -Wconf:cat=deprecation&msg=procedure syntax is deprecated:e
  -Wconf:cat=unchecked&msg=outer reference:s
  -Wconf:cat=unchecked&msg=eliminated by erasure:s
  -Wconf:msg=^(?=.*?a value of type)(?=.*?cannot also be).+$:s
  -Wconf:cat=unused-imports&src=org\/apache\/spark\/graphx\/impl\/VertexPartitionBase.scala:s
  -Wconf:cat=unused-imports&src=org\/apache\/spark\/graphx\/impl\/VertexPartitionBaseOps.scala:s
  -Wconf:msg=Implicit definition should have explicit type:s
  -target:8
  -bootclasspath
  ...

After downgrading the version, the -release option should no longer appear in the compilation arguments.

  1. Maven can build the project with Java 17 without the issue described in [SPARK-44376][BUILD] Fix maven build using scala 2.13 and Java 11 or later #41943. And after this pr, we can re-upgrade Scala 2.13 to Scala 2.13.11.

Was this patch authored or co-authored using generative AI tooling?

No

@LuciferYang LuciferYang marked this pull request as draft September 13, 2023 05:56
@github-actions github-actions bot added the BUILD label Sep 13, 2023
@LuciferYang
Copy link
Contributor Author

I am writing the PR description, will be updated later

@LuciferYang LuciferYang marked this pull request as ready for review September 13, 2023 07:03
@LuciferYang
Copy link
Contributor Author

updated

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM.

@srowen
Copy link
Member

srowen commented Sep 13, 2023

Is the idea that we can update this again, along with Scala to 2.13.11, after we drop Java 8 support?
I'm still a little bit confused in that this is reporting an error building vs Java 11, but we do not do that or support that, because we support Java 8 and thus builds need to be compiled with Java 8. They then should run on Java 11, 17, etc.
Is this a problem now, given that?

@LuciferYang
Copy link
Contributor Author

@srowen After this downgrade, we can upgrade to Scala 2.13.11 again without waiting for dropping Java 8, because the lower version of the plugin will not automatically adding the -release option anymore. We can still build with Java 8, and then test with Java 11/17/21.

@srowen
Copy link
Member

srowen commented Sep 13, 2023

OK that's useful if so.

@dongjoon-hyun
Copy link
Member

Ya, I agree with @LuciferYang and hope we can have Scala 2.13.11 back after this PR.

@dongjoon-hyun
Copy link
Member

The UT failure is irrelevant one which is tracked here (#42908).
Merged to master for Apache Spark 4.0.0. Thank you, @LuciferYang and all.

@LuciferYang
Copy link
Contributor Author

Thanks @dongjoon-hyun @HyukjinKwon @srowen , I will re-upgrade Scala 2.13.11 later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants