You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by JoshRosen <gi...@git.apache.org> on 2016/03/30 23:40:10 UTC
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
GitHub user JoshRosen opened a pull request:
https://github.com/apache/spark/pull/12073
[SPARK-14281][TESTS] Fix java8-tests and simplify their build
This patch fixes a compilation / build break in Spark's `java8-tests` and refactors their POM to simplify the build. See individual commit messages for more details.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/JoshRosen/spark fix-java8-tests
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/12073.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #12073
----
commit 9ee6a73c4bc221ff996b165a9f238ed23504a96c
Author: Josh Rosen <jo...@databricks.com>
Date: 2016-03-30T19:37:43Z
Fix java8-tests compilation.
commit 499c2c6f9bfd89249d4eb41667685bca719bae37
Author: Josh Rosen <jo...@databricks.com>
Date: 2016-03-30T19:45:23Z
Remove unnecessary maven-jar-plugin invocation from java8-tests profile.
We now build test-jars for all subprojects, so this is redundant.
commit 6d4122658e4e8e8acdfb69ee626427fee4ed307d
Author: Josh Rosen <jo...@databricks.com>
Date: 2016-03-30T19:46:50Z
Rename project in POM, since the old name looked funny in logs.
commit 1995d7ce1a341693fa450b5f67e536c8e98c1751
Author: Josh Rosen <jo...@databricks.com>
Date: 2016-03-30T19:53:54Z
Automatically activate java8-tests profile based on detected JDK.
commit 526832660193a649dc1b379c5b31d87a859e7958
Author: Josh Rosen <jo...@databricks.com>
Date: 2016-03-30T19:58:21Z
Update README to reflect auto-enabling of profile.
commit cf50a68ca078901724c025315d66281c08318be9
Author: Josh Rosen <jo...@databricks.com>
Date: 2016-03-30T20:27:42Z
Remove unnecessary <profiles> section in java8-tests's own POM.
commit a463d5b195f5d9e05def531b9b137897ea28f48a
Author: Josh Rosen <jo...@databricks.com>
Date: 2016-03-30T20:44:50Z
Remove redundant maven-surefire-plugin configuration.
commit 16b782345a6ecd6fea99893abac2eb342cf694ec
Author: Josh Rosen <jo...@databricks.com>
Date: 2016-03-30T21:06:09Z
Simplifications of Maven compiler configuration in java8-tests module.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/12073#issuecomment-204064777
+1. "Oops". Yeah we probably want to keep these tests in any event since they specifically test Java 8. Bigger question is just how far we want to go to update and streamline Java code and tests to use lambdas.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/12073#issuecomment-203698595
Out of curiosity, isn't all this work mostly throw away if we're really dropping jdk7 support? At that point you could just move the source files to core/src/test, and get rid of all the build-related code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/12073#discussion_r57968943
--- Diff: pom.xml ---
@@ -1922,6 +1922,7 @@
<JAVA_HOME>${test.java.home}</JAVA_HOME>
</environmentVariables>
<systemProperties>
+ <log4j.configuration>file:src/test/resources/log4j.properties</log4j.configuration>
--- End diff --
This seems to be necessary for some unexplained reason; without this, the removal of the explicit surefire configuration in the `java8-tests` POM leads to tons of log spam.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12073#issuecomment-203646431
**[Test build #54555 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/54555/consoleFull)** for PR 12073 at commit [`16b7823`](https://github.com/apache/spark/commit/16b782345a6ecd6fea99893abac2eb342cf694ec).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12073#issuecomment-203687986
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/12073#discussion_r57968820
--- Diff: external/java8-tests/pom.xml ---
@@ -87,74 +82,26 @@
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
- <artifactId>maven-surefire-plugin</artifactId>
- <executions>
- <execution>
- <id>test</id>
- <goals>
- <goal>test</goal>
- </goals>
- </execution>
- </executions>
- <configuration>
- <systemPropertyVariables>
- <!-- For some reason surefire isn't setting this log4j file on the
- test classpath automatically. So we add it manually. -->
- <log4j.configuration>
- file:src/test/resources/log4j.properties
- </log4j.configuration>
- </systemPropertyVariables>
- <skipTests>false</skipTests>
- <includes>
- <include>**/Suite*.java</include>
- <include>**/*Suite.java</include>
- </includes>
- </configuration>
- </plugin>
- <plugin>
- <groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
- <executions>
- <execution>
- <id>test-compile-first</id>
- <phase>process-test-resources</phase>
- <goals>
- <goal>testCompile</goal>
- </goals>
- </execution>
- </executions>
<configuration>
- <fork>true</fork>
- <verbose>true</verbose>
<forceJavacCompilerUse>true</forceJavacCompilerUse>
<source>1.8</source>
- <compilerVersion>1.8</compilerVersion>
<target>1.8</target>
- <encoding>UTF-8</encoding>
- <maxmem>1024m</maxmem>
+ <compilerVersion>1.8</compilerVersion>
</configuration>
</plugin>
<plugin>
- <!-- disabled -->
<groupId>net.alchim31.maven</groupId>
<artifactId>scala-maven-plugin</artifactId>
- <executions>
--- End diff --
AFAIK the idea behind this logic was to ensure that Java files would not be compiled by the Scala maven plugin, but disabling this has the side-effect of preventing this module's Scala test source file from being compiled and run in the Maven build.
Instead, I think it's okay to just modify the Scala Maven Plugin's `javacArgs` configuration.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/12073#issuecomment-204106588
Go ahead, it's a net improvement on the current state.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12073#issuecomment-203687696
**[Test build #54555 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/54555/consoleFull)** for PR 12073 at commit [`16b7823`](https://github.com/apache/spark/commit/16b782345a6ecd6fea99893abac2eb342cf694ec).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/12073#discussion_r57969003
--- Diff: pom.xml ---
@@ -1937,6 +1938,14 @@
<failIfNoTests>false</failIfNoTests>
<excludedGroups>${test.exclude.tags}</excludedGroups>
</configuration>
+ <executions>
--- End diff --
Without this change, `mvn -pl :java8-tests_2.11 test` wasn't triggering these tests.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12073#issuecomment-203687989
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/54555/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/12073#discussion_r57969055
--- Diff: pom.xml ---
@@ -2345,27 +2355,12 @@
<profile>
<id>java8-tests</id>
- <build>
- <plugins>
- <!-- Needed for publishing test jars as it is needed by java8-tests -->
--- End diff --
We now publish test JARs by default, so this is redundant.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/12073#issuecomment-204047788
Yep, this would be irrelevant if we dropped JDK 7. However, I'm doing some other work which may have an impact on Java 8 lambda compatibility so I wanted to fix these tests to make my life a little easier.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/12073#discussion_r57969019
--- Diff: pom.xml ---
@@ -2345,27 +2355,12 @@
<profile>
<id>java8-tests</id>
- <build>
- <plugins>
- <!-- Needed for publishing test jars as it is needed by java8-tests -->
- <plugin>
- <groupId>org.apache.maven.plugins</groupId>
- <artifactId>maven-jar-plugin</artifactId>
- <executions>
- <execution>
- <goals>
- <goal>test-jar</goal>
- </goals>
- </execution>
- </executions>
- </plugin>
- </plugins>
- </build>
-
+ <activation>
+ <jdk>[1.8,)</jdk>
--- End diff --
Here's where the JDK auto-detection is performed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/12073#issuecomment-203647813
I have updated the pull request builder Jenkins job to use a Java 8 JDK, so these tests should now be run.
It just occurred to me that I may want to configure the `dev/sparktestsupport/modules.py` file to make sure that these tests are automatically run if only a subset of tests would otherwise be run.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14281][TESTS] Fix java8-tests and simpl...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/12073#issuecomment-204105446
Any objections to these build changes or is this now good to merge?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org