[SPARK-9015] [BUILD] Clean project import in scala ide

[jendap]

Cleanup maven for a clean import in scala-ide / eclipse.

• remove groovy plugin which is really not needed at all
• add-source from build-helper-maven-plugin is not needed as recent version of scala-maven-plugin do it automatically
• add lifecycle-mapping plugin to hide a few useless warnings from ide

[srowen]

I would not bother adding these. This is a silly javac lint warning and should be disabled at the compiler level, IMHO.

[jendap]

sure, I can do that

[jendap]

Actually, I put this there to have absolutely zero warnings across the whole project in scala-ide. We should, ihmo, not change the compiler settings because spark uses java serialization by default. Not warning on missing serialization in new classes would broke spark.

But these two classes are tests anyway.

Should we keep it in the PR or remove it?

[srowen]

The warning is bogus though, about serialVersionUID? it should actually not be specified by default. Does the Spark build actually enable this warning though, or is it just Eclipse? I didn't think -Xlint was on.

This can't be the only warning :) I see a load still, that I've been meaning to fix again again.

[jendap]

Ok, I'll but there the id instead of warning.

There's a lot of warnings when you run maven build. But I believe they were not shown by default when you clicked "import maven project" in fresh installation of scala-ide two month ago. Only these serialization warnings. Fixing more warnings would be for some other PR.

[jendap]

There are ton of classes with serializable but missing uid. I'm just going to remove this.

[jendap]

I have double checked all this with "mvn install". It generates exactly the same output in local repo - bit by bit - with exception of the pom xml and property files in META-INF (inside the jars) and the two test classes with @SuppressWarnings("serial").

BTW: This is general cleanup and it speedup the build as well. The only ide specific thing is that lifecycle-mapping.

[SparkQA]

Test build #1059 has finished for PR 7375 at commit 312007e.

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

[JoshRosen]

Note that removing this from the dependencies list will remove Groovy as a provided dependency from all of the Spark project POMs. I always thought it was really confusing how the published Maven artifacts would declare a dependency on Groovy, so I'm glad to see this cleaned up.

[srowen]

This is looking good, though I'm still skeptical about the Eclipse addition. Does it work, but just shows a spurious error when opening the project the first time? then that is probably fine to leave as is rather than work around.

[jendap]

It does work without lifecycle-mapping but it shows errors. Not just after import but all the way until you resolve them.

It is not needed. On the other hand it makes for better experience for some developers. No downside other than 50 lines in pom.xml and is (unfortunately) not much for a pom file.

We can remove it. It is your call!

BTW: Note it is in 'pluginManagement' only. It does never get executed. It is just like a comment in source code.

[srowen]

It's a fair argument. I also think the build is horribly complex enough already and would like to simplify at all costs. Does anyone else have any strong opinion on this?

[jendap]

I can see you do not want to add any extra line to the pom file :-) I think the build should primary work. Batteries included. For that reason we should merge the lifecycle-mappings.

There are other things that would, imho, simplify the build! Namely:

1. remove "change-version-to-X.sh" scripts calling sed command
2. not running twice for integration tests - once with "-DskipTests" and then without it (that would actually add 50 lines to pom file but improve speed and usability)
3. workaround issues with file name lengths for some folks
   ... and the best of all ...
4. we can covert pom.xml to pom.scala and get a rid of the xml :-)

and even with all those done the lifecycle-mappings still belongs there.

[srowen]

The change-version scripts are being improved in SPARK-8401. Still ugly, but I'm not sure we have a better way given how Maven works and how Scala artifact conventions unfortunately work. I also am not sure if the build-then-test situation can be fixed unless the things that need the assembly are properly moved to integration-test phase. What's the file name issue? And what on earth is pom.scala?

[jendap]

None of those are part of this PR. We should take that discussion elsewhere.

BTW: I have a prototype solving the artifactId conventions inside maven. Integrations tests should be tagged and scalatest-maven-plugin should use tagsToInclude and tagsToExclude. Name issue is a detail of a few linux users with encrypted Private directory. pom.scala is something you will love once you see it. Not any time soon.

[srowen]

Yeah side discussions. The encrypted file system thing is really an end-user env limitation and not something the build is going to work around -- this was discussed earlier elsewhere.

Anyway, still interested in any opinions on the Eclipse stanza

[SparkQA]

Test build #1063 has finished for PR 7375 at commit 5a83e07.

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

[SparkQA]

Test build #1065 has finished for PR 7375 at commit 5a83e07.

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

[jendap]

@srowen do you understand what is going on with the builds? 3 failed builds is pretty convincing argument something is not ok. But looking at their output - 3 build 3 different failures. Can it be some other problems outside of this PR?

[srowen]

Yeah, I was wondering the same. The first two might have been spurious; the last is more 'convincing' but I don't get how the build would not have hit this the first time. Spin the wheel again --

[SparkQA]

Test build #1066 has finished for PR 7375 at commit 5a83e07.

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

[srowen]

Passes now. Well, let's leave in the eclipse bit. It's still a net simplification. There is one very trivial whitespace issue -- think you have tab instead of spaces on one line that was changed -- but don't worry about it unless you have a moment.

Any other thoughts on this one?

[jendap]

Sure! Whitespaces are fixed now.

[jendap]

thanks @srowen