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

[SPARK-34762][BUILD] Fix the build failure with Scala 2.13 which is related to commons-* with better solution #31880

Closed
wants to merge 5 commits into from
Closed

[SPARK-34762][BUILD] Fix the build failure with Scala 2.13 which is related to commons-* with better solution #31880

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0be2ecf
Fix for SPARK-34762 with better solution.
sarutak Mar 18, 2021
7bc154b
Revert the change in pomlxml.
sarutak Mar 18, 2021
4cdc8c5
Merge branch 'master' of https://github.com/apache/spark into fix-com…
sarutak Mar 18, 2021
3c66069
Modify change-scala-version.sh to choose scala.version from <profile>.
sarutak Mar 18, 2021
9660460
Modify comment.
sarutak Mar 18, 2021
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
9 changes: 3 additions & 6 deletions dev/change-scala-version.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,11 @@ BASEDIR=$(dirname $0)/..
find "$BASEDIR" -name 'pom.xml' -not -path '*target*' -print \
-exec bash -c "sed_i 's/\(artifactId.*\)_'$FROM_VERSION'/\1_'$TO_VERSION'/g' {}" \;

# dependency:get is workaround for SPARK-34762 to download the JAR file of commons-cli.
# Without this, build with Scala 2.13 using SBT will fail because the help plugin used below downloads only the POM file.
COMMONS_CLI_VERSION=`build/mvn help:evaluate -Dexpression=commons-cli.version -q -DforceStdout`
build/mvn dependency:get -Dartifact=commons-cli:commons-cli:${COMMONS_CLI_VERSION} -q

# Update <scala.version> in parent POM
# First find the right full version from the profile's build
SCALA_VERSION=`build/mvn help:evaluate -Pscala-${TO_VERSION} -Dexpression=scala.version -q -DforceStdout`
# NOTE: We used to fetch the value of scala.version before but sed is used now. This is a workaround for SPARK-34762.
ESCAPED_TO_VERSION=$(echo $TO_VERSION | sed -n "s/\./\\\\./gp")
SCALA_VERSION=$(sed -n "s;^.*<scala-$ESCAPED_TO_VERSION\.version>\(.*\)</scala-$ESCAPED_TO_VERSION\.version>.*$;\1;p" pom.xml)
sed_i '1,/<scala\.version>[0-9]*\.[0-9]*\.[0-9]*</s/<scala\.version>[0-9]*\.[0-9]*\.[0-9]*</<scala.version>'$SCALA_VERSION'</' \
"$BASEDIR/pom.xml"

Expand Down
6 changes: 4 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,9 @@
<commons.math3.version>3.4.1</commons.math3.version>
<!-- managed up from 3.2.1 for SPARK-11652 -->
<commons.collections.version>3.2.2</commons.collections.version>
<scala.version>2.12.10</scala.version>
<scala-2.12.version>2.12.10</scala-2.12.version>
<scala-2.13.version>2.13.5</scala-2.13.version>
Copy link
Member Author

Choose a reason for hiding this comment

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

For reviewers:
I confirmed this solution works even for branch-3.1 but branch-3.1 uses 2.13.4 rather than 2.13.5 so I'll open a backport PR after this PR merged.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, why do we need both versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's better to have both versions to avoid issues like SPARK-34774.

Copy link
Member

Choose a reason for hiding this comment

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

I don't quite see why it is necessary for this change?

Copy link
Member Author

@sarutak sarutak Mar 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It's to preserve one version in pom.xml even after overwrite it withchange-scala-version.sh.
But now that #31865 introduces <scala.version> in <profile>, we can take another solution (3c66069) without having both <scala-2.12.version> and <scala-2.13.version>.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see the advantage or need - surely the build has one scala version? otherwise you have to flip even which property is referenced in the whole build?

Copy link
Member Author

@sarutak sarutak Mar 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The first time I open this PR, <scala.version>2.12.10</scala.version> is absent from the profile <id>scala-2.12</id> so, if we change the version with change-scala-version.sh 2.13, there will be no more 2.12.10 because <scala.version>2.12.10</scala.version> is overwritten with <scala.version>2.13.5</scala.version>.
Then, change-scala-version.sh 2.12 will fail to modify pom.xml.

If we have both versions in properties (<scala-2.12.version> and <scala-2.13-version>) which will not be overwritten by change-scala-version.sh, it's easy to choose scala.version to change.

But, as I mentioned above, now we don't need such properties.

<scala.version>${scala-2.12.version}</scala.version>
<scala.binary.version>2.12</scala.binary.version>
<scalatest-maven-plugin.version>2.0.0</scalatest-maven-plugin.version>
<scalafmt.parameters>--test</scalafmt.parameters>
Expand Down Expand Up @@ -3317,7 +3319,7 @@
<profile>
<id>scala-2.13</id>
<properties>
<scala.version>2.13.5</scala.version>
<scala.version>${scala-2.13.version}</scala.version>
<scala.binary.version>2.13</scala.binary.version>
</properties>
<dependencyManagement>
Expand Down