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-1812] Adjust build system and tests to work with scala 2.11+ repl port. #2615

Closed
wants to merge 8 commits into from

Conversation

ScrapCodes
Copy link
Member

No description provided.

@SparkQA
Copy link

SparkQA commented Oct 1, 2014

QA tests have started for PR 2615 at commit b9389db.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 1, 2014

QA tests have finished for PR 2615 at commit b9389db.

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

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21104/

@ScrapCodes ScrapCodes changed the title Adjust build system and tests to work with scala 2.11+ repl port. [ SPARK-1812] Adjust build system and tests to work with scala 2.11+ repl port. Oct 1, 2014
@SparkQA
Copy link

SparkQA commented Oct 1, 2014

QA tests have started for PR 2615 at commit 83998bf.

  • This patch merges cleanly.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21105/

@SparkQA
Copy link

SparkQA commented Oct 1, 2014

Tests timed out for PR 2615 at commit 83998bf after a configured wait of 120m.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21106/

@pwendell
Copy link
Contributor

pwendell commented Oct 1, 2014

Hey @ScrapCodes thanks for posting the whole thing here. In terms of the way this interacts with the build - I really think we'll need to write a maven plugin to make this all work. The reason is that we need to publish separate artifacts for Scala 2.11 and 2.10 - we can't publish a single artifact that relies on profile activation. The reason is that most build tools (Maven, SBT) won't respect profiles from other pom's you are linking against. To test this yourself you can publish locally for Scala 2.11 and then try to write a project that links against it - does it work? My guess is that it won't work.

Really I think what we need is a build plug-in that re-writes our published poms to do two things:

  1. Set the correct Scala version in the artifact ID.
  2. Advertise the correct set of dependencies for 2.10 vs 2.11.

You said earlier that it's not possible for maven build plug-ins to modify the build, but I'm pretty sure it is - because the maven-shade-plugin that we use does exactly this. It modifies our published pom to exclude guava (as @vanzin can tell you since he wrote this). I would take a look and see if you can mimic how that works. I looked quickly and there is a bunch of logic related to pom re-writing.

https://github.com/apache/maven-plugins/blob/trunk/maven-shade-plugin/src/main/java/org/apache/maven/plugins/shade/pom/MavenJDOMWriter.java#L1668

@ScrapCodes
Copy link
Member Author

Hey Patrick, thanks for looking at this. I did not say it is not possible. I just said the best(easiest ) way I could come up was to modify the maven install plugin.

@ScrapCodes
Copy link
Member Author

And this https://github.com/ScrapCodes/scala-install-plugin plugin takes care of publishing correct poms too.

@ScrapCodes
Copy link
Member Author

So now to try this patch just `mvn install - this plugin. After that look at the published poms.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21188/

@ScrapCodes
Copy link
Member Author

I think you mean to try replaceFile when you said maven-shade-plugin. Let me try that as well.

@ScrapCodes
Copy link
Member Author

Ahh wait, I would still need to alter maven-install-plugin. Because, there has to be someway to tell install plugin that it has to install at location which has _2.10 at the end. Which means we will anyway substitute install plugin with something that does this. And I guess then best thing to do is use scala-install-plugin.

@ScrapCodes
Copy link
Member Author

Hey @pwendell, I have updated this patch to include effective pom changes. So that you can try it out. Also I think this is ready for review !

@@ -264,6 +284,10 @@
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.twitter</groupId>
<artifactId>chill-java</artifactId>
</dependency>
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: remove it.

@SparkQA
Copy link

SparkQA commented Oct 20, 2014

QA tests have started for PR 2615 at commit 812db5b.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 20, 2014

Tests timed out for PR 2615 at commit 812db5b after a configured wait of 120m.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21918/
Test FAILed.

@retronym
Copy link

Was any other design other than a wholesale copy/paste of the REPL considered? The commit message doesn't reveal much.

We'd be happy to help out over in scala-internals if you want to find a better way.

@ScrapCodes
Copy link
Member Author

Hey Jason,
Thanks for offering to help, In short, I need help from scala-internals so that it is possible to customize wrappers. I tried my bit in SI-7747. There is some background there. That bug got fixed but unfortunately, the wrapper was changed a bit(from what I suggested) before it was merged. And this modified wrapper is of no help to us. I can go ahead and keep explaining what our wrappers are like and how the classBasedWrapper is not useful. If you had like me to ? Actual help would be to allow us plugin our own wrappers and ofcourse access mechanism(for accessing those created variables) should be equally customizable. This is why, the wholesale copy paste was needed. Since most things are private.

@retronym
Copy link

It is a pity that we didn't realise that the finally accepted PR was insufficient for Spark.

Would you be interested in proposing another PR against scala/scala to open things up sufficiently? Perhaps collaborate with @som-snytt to make this happen?

My interest in this is making future Scala-version upgrades in Spark decoupled from compiler implementation details.

@som-snytt
Copy link

I had a version for that SI-7747 that moved the scripting logic to a separate compiler phase which could be replaced by a user plugin. (But that also moved the repl away from text-based templating to tree transforms, so it was broader scope. Also, not sure if a plugin satisfies "decoupled from compiler details.") The work for the PR was only to satisfy Spark's needs as a repl consumer, so why not reopen the issue.

@SparkQA
Copy link

SparkQA commented Oct 23, 2014

QA tests have started for PR 2615 at commit 897ec60.

  • This patch merges cleanly.

@ScrapCodes
Copy link
Member Author

Sure @retronym and @som-snytt, I will give it an another try soon.

case None => backwardCompatibility
case Some(v) =>
if (backwardCompatibility.nonEmpty)
println("Note: We ignore environment variables, when use of profile is detected in " +
"conjunction with environment variable.")
v.split("(\\s+|,)").filterNot(_.isEmpty).map(_.trim.replaceAll("-P", "")).toSeq
}
if(profiles.exists(_.contains("scala"))) {
Copy link
Member Author

Choose a reason for hiding this comment

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

note to self: scala-

@SparkQA
Copy link

SparkQA commented Oct 28, 2014

Test build #22353 has started for PR 2615 at commit 4754873.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 28, 2014

Test build #22353 has finished for PR 2615 at commit 4754873.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22353/
Test FAILed.

@SparkQA
Copy link

SparkQA commented Oct 28, 2014

Test build #22356 has started for PR 2615 at commit d8aa8c9.

  • This patch merges cleanly.

@ScrapCodes
Copy link
Member Author

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Oct 28, 2014

Test build #22358 has started for PR 2615 at commit d8aa8c9.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 28, 2014

Test build #22356 timed out for PR 2615 at commit d8aa8c9 after a configured wait of 120m.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22356/
Test FAILed.

@SparkQA
Copy link

SparkQA commented Oct 28, 2014

Test build #22358 timed out for PR 2615 at commit d8aa8c9 after a configured wait of 120m.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22358/
Test FAILed.

@SparkQA
Copy link

SparkQA commented Oct 29, 2014

Test build #22436 has started for PR 2615 at commit 95c2e8e.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 29, 2014

Test build #22441 has started for PR 2615 at commit df2b19e.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 29, 2014

Test build #22441 has finished for PR 2615 at commit df2b19e.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22441/
Test FAILed.

@SparkQA
Copy link

SparkQA commented Oct 29, 2014

Test build #22444 has started for PR 2615 at commit 472bbcf.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 29, 2014

Test build #22444 has finished for PR 2615 at commit 472bbcf.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22444/
Test FAILed.

@ScrapCodes
Copy link
Member Author

Jenkins, retest this please. (I might get lucky this time, Looks like the compilation failure is random.)

@SparkQA
Copy link

SparkQA commented Oct 29, 2014

Test build #22445 has started for PR 2615 at commit 472bbcf.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 29, 2014

Test build #22445 has finished for PR 2615 at commit 472bbcf.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22445/
Test FAILed.

@SparkQA
Copy link

SparkQA commented Oct 29, 2014

Test build #22436 timed out for PR 2615 at commit 95c2e8e after a configured wait of 120m.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22436/
Test FAILed.

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

Successfully merging this pull request may close these issues.

6 participants