Uh oh!
There was an error while loading. Please reload this page.
[SPARK-45272][BUILD][INFRA][DOCS] Remove Scala version specific comments, and scala-2.13 profile usage - #43049
[SPARK-45272][BUILD][INFRA][DOCS] Remove Scala version specific comments, and scala-2.13 profile usage#43049HyukjinKwon wants to merge 2 commits into
Conversation
HyukjinKwon
commented
Sep 22, 2023
I didn't cleanup |
HyukjinKwon
commented
Sep 22, 2023
and I am intentionally using the same JIRA to make it easier to manage together (e.g., reverting). cc @dongjoon-hyun and @LuciferYang FYI |
| <groupId>org.apache.spark</groupId> | ||
| <artifactId>spark-tags_${scala.binary.version}</artifactId> | ||
| </dependency> | ||
| <!-- #if scala-2.13 --> |
There was a problem hiding this comment.
Are we sure about deleting these comments? I was hesitant before, because I didn't know if we would need to use a similar approach to dynamically change dependencies when supporting Scala 3, so I took a conservative approach and kept them.
There was a problem hiding this comment.
Do we need to change scala-parallel-collections for Scala 3 upgrade? I would think we'd need it for other libraries. I am not removing the mechanism.
There was a problem hiding this comment.
It seems that for Scala 3 the scala.binary.version is always 3? Then it looks like we can delete this comment, the corresponding dependency for Scala 3 is
<dependency>
<groupId>org.scala-lang.modules</groupId>
<artifactId>scala-parallel-collections_3</artifactId>
<version>1.0.4</version>
</dependency>
It seems that this dependency no longer needs to be dynamically changed.
dongjoon-hyun
commented
Sep 22, 2023
I have the same concern. I'd like to keep these because of Scala 3. |
| SPARK_PROFILES=${1:-"-Pscala-2.13 -Pmesos -Pkubernetes -Pyarn -Pspark-ganglia-lgpl -Pkinesis-asl -Phive-thriftserver -Phive"} | ||
| TOOLS_CLASSPATH="$(build/sbt -Pscala-2.13 -DcopyDependencies=false "export tools/fullClasspath" | grep jar | tail -n1)" | ||
| OLD_DEPS_CLASSPATH="$(build/sbt -Pscala-2.13 -DcopyDependencies=false $SPARK_PROFILES "export oldDeps/fullClasspath" | grep jar | tail -n1)" |
There was a problem hiding this comment.
This is good. What I conceded is only Oh, sorry. I missed that this is also directly related.pom.xml file changes.
There was a problem hiding this comment.
While cleaning this up, I suggest removing 2.13 from VALID_VERSIONS in the change-scala-version.sh script too, otherwise if we execute dev/change-scala-version.sh 2.13, these files will generate some unnecessary git diff.
spark/dev/change-scala-version.sh
Line 22 in 5f643ee
There was a problem hiding this comment.
I commented dev/change-scala-version.sh usage out in the README.md :-).
Uh oh!
There was an error while loading. Please reload this page.
dongjoon-hyun
commented
Sep 22, 2023
BTW, it's surprising to me that the diff size is very small. |
c2e7d81 to
88ba14bCompare@HyukjinKwon and @LuciferYang . If the proposed removal is only 53 lines, I believe this could be a cleaner way and we can recover it back easily. To recover only this, shall we use an independent JIRA issue ID, @HyukjinKwon ? |
HyukjinKwon
commented
Sep 22, 2023
Sure, I am fine |
| <!-- | ||
| SPARK-34774 Add this property to ensure change-scala-version.sh can replace the public `scala.version` | ||
| property correctly. | ||
| --> |
There was a problem hiding this comment.
Nesting a comment doesn't work .. so I took it out.
dongjoon-hyun
commented
Sep 22, 2023
Many builds successful including Java 21 Maven build. Let me merge this~ ;) |
What changes were proposed in this pull request?
This PR is a followup of #43008 that cleanups Scala version specific comments, and
scala-2.13profile usage.See also #43008 (comment)
Why are the changes needed?
To remove unnecessary profile specifications
Does this PR introduce any user-facing change?
No.
How was this patch tested?
CI in this PR should test them out. For README.md, I manually checked via GitHub.
Was this patch authored or co-authored using generative AI tooling?
No.