[SPARK-14867][BUILD] Remove `--force` option in `build/mvn` #12631

dongjoon-hyun · 2016-04-23T05:15:45Z

What changes were proposed in this pull request?

Currently, build/mvn provides a convenient option, --force, in order to use the recommended version of maven without changing PATH environment variable. However, there were two problems.

dev/lint-java does not use the newly installed maven.

$ ./build/mvn --force clean
$ ./dev/lint-java 
Using `mvn` from path: /usr/local/bin/mvn

It's not easy to type --force option always.

If '--force' option is used once, we had better prefer the installed maven recommended by Spark.
This PR makes build/mvn check the existence of maven installed by --force option first.

According to the comments, this PR aims to the followings:

Detect the maven version from pom.xml.
Install maven if there is no or old maven.
Remove --force option.

How was this patch tested?

Manual.

$ ./build/mvn --force clean
$ ./dev/lint-java 
Using `mvn` from path: /Users/dongjoon/spark/build/apache-maven-3.3.9/bin/mvn
...
$ rm -rf ./build/apache-maven-3.3.9/
$ ./dev/lint-java 
Using `mvn` from path: /usr/local/bin/mvn

SparkQA · 2016-04-23T07:11:09Z

Test build #56780 has finished for PR 12631 at commit 56355fa.

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

srowen · 2016-04-23T09:03:56Z

Yes, on a system with Maven < 3.3.9 installed, you always have to pass --force. This would at least make it optional on the second or subsequent run.

Ideally we could even auto-detect the mvn version, which isn't too hard, and then download and/or use the downloaded copy only if the installed one is too old. What do you think about trying that? I wouldn't want to do it if it ends up being a lot of complexity, but ought to be a few more lines. We could even remove --force then, I suppose, since you have to download it if your local maven is too old.

dongjoon-hyun · 2016-04-23T17:44:47Z

That sounds good to me. No problem!
I'll update today.
Thank you, @srowen !

SparkQA · 2016-04-23T18:58:13Z

Test build #56808 has finished for PR 12631 at commit afd7f03.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-04-23T19:03:07Z

Test build #56809 has finished for PR 12631 at commit ee883e0.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

markhamstra · 2016-04-23T19:43:22Z

build/mvn

This really should be grep'd from the pom file, as is done with scala_version in install_scala. Also, make sure that the correct maven is downloaded and installed in the case that there isn't any mvn available when this script is run.

Thank you, @markhamstra .
I'll fix that to read "3.3.9" from pom file.

By the way, what do you mean by "make sure that the correct maven is downloaded and installed in the case that there isn't any mvn available when this script is run"?

I mean that using this build/mvn script should work even if there isn't any version of maven currently installed on the machine (which might be the case, e.g., when using an automated tool to set up a new build node.) I believe that would work with --force previously, and it should work when there isn't a --force option anymore.

Ah. For that, I tested already. When there is no maven, this script installs the correct maven. Thank you again!

SparkQA · 2016-04-23T21:02:53Z

Test build #56810 has finished for PR 12631 at commit d593607.

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

dongjoon-hyun · 2016-04-24T04:06:31Z

Rebased.

SparkQA · 2016-04-24T05:37:39Z

Test build #56829 has finished for PR 12631 at commit cfe9b4e.

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

SparkQA · 2016-04-24T05:47:05Z

Test build #56830 has finished for PR 12631 at commit 8504cce.

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

SparkQA · 2016-04-24T08:33:02Z

Test build #56836 has finished for PR 12631 at commit b11de8a.

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

srowen · 2016-04-24T08:36:33Z

build/mvn

Since you use awk below, would this find the version correctly? awk -F '[<>]' '{print $3}' Looks like it, but I don't know if it's much simpler. You might also use "-n1" consistently like below in the head command.

It works as the Jenkins runs.

spark:SPARK-14867$ grep "<maven.version>" pom.xml <maven.version>3.3.9</maven.version>

For simplification, I'm not sure since I borrowed it from the following install_scala() code in the same script. Should I change them both for consistency?

install_scala() { # determine the Scala version used in Spark local scala_version=`grep "scala.version" "${_DIR}/../pom.xml" | \ head -1 | cut -f2 -d'>' | cut -f1 -d'<'`

Please let me know. If needed, I will update both.

SparkQA · 2016-04-24T19:34:55Z

Test build #56855 has finished for PR 12631 at commit 128618e.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-04-24T19:59:33Z

Test build #56857 has finished for PR 12631 at commit 2b54019.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-04-24T21:43:41Z

Test build #56859 has finished for PR 12631 at commit 497dcb5.

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

dongjoon-hyun · 2016-04-24T22:51:11Z

The test failure seems to be irrelevant to this PR.

[info] Exception encountered when attempting to run a suite with class name: org.apache.spark.streaming.MapWithStateSuite *** ABORTED *** (41 seconds, 299 milliseconds)
[info]   java.io.IOException: Failed to delete: /home/jenkins/workspace/SparkPullRequestBuilder/streaming/checkpoint/spark-d2368db9-6d2a-43ba-b79f-372e51a41ce0

dongjoon-hyun · 2016-04-24T22:51:59Z

I updated the commit time of the last commit to retrigger Jenkins.

SparkQA · 2016-04-25T00:47:48Z

Test build #56865 has finished for PR 12631 at commit 55e4c5b.

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

…it exist.

dongjoon-hyun · 2016-04-25T08:58:39Z

Hi, @srowen .
Now, it's ready for review again.
I think I've addressed all your comments. The last one was about awk command that you recommended.
Currently, Jenkins passes the followings.

========================================================================
Running build tests
========================================================================
exec: curl -s -L https://downloads.typesafe.com/zinc/0.3.9/zinc-0.3.9.tgz
exec: curl -s -L https://downloads.typesafe.com/scala/2.11.8/scala-2.11.8.tgz
exec: curl -s -L https://www.apache.org/dyn/closer.lua?action=download&filename=/maven/maven-3/3.3.9/binaries/apache-maven-3.3.9-bin.tar.gz
...

Thank you for your review always. If there is anything to do more, please let me know.

srowen · 2016-04-25T09:00:45Z

Yeah that looks good to me.

dongjoon-hyun · 2016-04-25T09:04:48Z

Thank you, @srowen !

SparkQA · 2016-04-25T10:49:23Z

Test build #56890 has finished for PR 12631 at commit 20a520a.

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

dongjoon-hyun · 2016-04-27T18:27:48Z

Hi, @srowen .
Could you merge this PR?

srowen · 2016-04-27T19:56:35Z

Merged to master

dongjoon-hyun · 2016-04-27T19:57:52Z

Thank you, @srowen !

dongjoon-hyun changed the title ~~[SPARK-14867][BUILD] Make build/mvn to use the downloaded maven if it exists.~~ [SPARK-14867][BUILD] Remove --force option in build/mvn Apr 23, 2016

markhamstra reviewed Apr 23, 2016
View reviewed changes

dongjoon-hyun mentioned this pull request Apr 24, 2016

[SPARK-14868][BUILD] Enable NewLineAtEofChecker in checkstyle and fix lint-java errors #12632

Closed

srowen reviewed Apr 24, 2016
View reviewed changes

dongjoon-hyun added 8 commits April 25, 2016 01:43

[SPARK-14867][BUILD] Make build/mvn to use the downloaded maven if …

cb137ad

…it exist.

Choose Maven based on versions.

27605ba

Change to local variables.

b1cfc12

Add a code for backward compatability.

6fdf92d

Determine the maven version from pom.xml.

1f4b786

Add a deprecation message for option --force

d1356b8

Update test-dependecies.sh.

9c4add2

Use awk command according to the comments.

20a520a

asfgit closed this in f405de8 Apr 27, 2016

dongjoon-hyun mentioned this pull request May 8, 2016

[SPARK-15207][BUILD] Use Travis CI for Java Linter and JDK7/8 compilation test #12980

Closed

dongjoon-hyun deleted the SPARK-14867 branch May 12, 2016 01:00

[SPARK-14867][BUILD] Remove --force option in build/mvn #12631

[SPARK-14867][BUILD] Remove --force option in build/mvn #12631

Uh oh!

Conversation

dongjoon-hyun commented Apr 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Apr 23, 2016

Uh oh!

srowen commented Apr 23, 2016

Uh oh!

dongjoon-hyun commented Apr 23, 2016

Uh oh!

SparkQA commented Apr 23, 2016

Uh oh!

SparkQA commented Apr 23, 2016

Uh oh!

markhamstra Apr 23, 2016 • 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!

dongjoon-hyun Apr 23, 2016

Choose a reason for hiding this comment

Uh oh!

markhamstra Apr 23, 2016

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Apr 23, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 23, 2016

Uh oh!

dongjoon-hyun commented Apr 24, 2016

Uh oh!

SparkQA commented Apr 24, 2016

Uh oh!

SparkQA commented Apr 24, 2016

Uh oh!

SparkQA commented Apr 24, 2016

Uh oh!

srowen Apr 24, 2016

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Apr 24, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 24, 2016

Uh oh!

SparkQA commented Apr 24, 2016

Uh oh!

SparkQA commented Apr 24, 2016

Uh oh!

dongjoon-hyun commented Apr 24, 2016

Uh oh!

dongjoon-hyun commented Apr 24, 2016

Uh oh!

SparkQA commented Apr 25, 2016

Uh oh!

dongjoon-hyun commented Apr 25, 2016

Uh oh!

srowen commented Apr 25, 2016

Uh oh!

dongjoon-hyun commented Apr 25, 2016

Uh oh!

SparkQA commented Apr 25, 2016

Uh oh!

dongjoon-hyun commented Apr 27, 2016

Uh oh!

srowen commented Apr 27, 2016

Uh oh!

dongjoon-hyun commented Apr 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

[SPARK-14867][BUILD] Remove `--force` option in `build/mvn` #12631

[SPARK-14867][BUILD] Remove `--force` option in `build/mvn` #12631

dongjoon-hyun commented Apr 23, 2016 •

edited

Loading

markhamstra Apr 23, 2016 •

edited

Loading