[SPARK-28756][R][FOLLOW-UP] Specify minimum and maximum Java versions #25490
Conversation
|
Oh, thank you for the quick follow-up, @HyukjinKwon ! Could you review this, @felixcheung ? |
|
Looks fine. Like you said, not sure how SystemRequirements works. Very little documentation.
|
|
Test build #109315 has finished for PR 25490 at commit
|
|
retest this please |
|
Test build #109323 has finished for PR 25490 at commit
|
|
AppVeyor failure seems unrelated - it is already failed with the same error in other builds. |
|
Let me merge this. The failure is being investigated #25493 and the root cause is orthogonal. We don't run CRAN on Windows anyway. |
|
Merged to master. |
|
What’s the issue? Just want to call out CRAN does run R package tests on windows (and we have been rejected due to failing on windows before)
|
|
The issue seems to be related to scala-maven-package's version. When we jump up to 4.0.0, the build on Windows fails. The only radical changes I see between 3.4.6 and 4.0.0 are related tk incremental complication and zinc. It might be the cause. I'm running Appveyor builds at #25497. The current master is now fixed because I reverted scala-maven-plugin version bumpup |
|
Then might be ok with CRAN. In CRAN we don’t build spark source code, only download the release.
|
believe me, we couldn't "tell" them to do stuff... very long story. |
|
@felixcheung Oh yes I just meant specify in the CRAN config or whatever that it uses Java 8 only, if that causes it to test vs Java 8 |
|
I got you. However, we tried and found that wasn’t possible. SystemRequirements is not respected in CRAN tests. There is no way to tell CRAN which to use and in all likelihood, it only has 1 version of JDK.
Basically the only way we found was to skip all tests if the JDK is not the version that Spark supports (as in this change to check for max version)
That want to point out to get all of us on the same page...
|
|
I think Java needs to be bumped again in DESCRIPTION |
|
It's currently >= 8 and < 12, which seems accurate? (though should actually 99% work with Java 17 already in 3.2.0) |
|
PYSpark will work with 17 but not RSpark because of this |
|
Sounds like a good suggestion. Could you make a PR, @Bidek56 ? |
|
You can create a subtask JIRA under https://issues.apache.org/jira/browse/SPARK-33772. |
|
PR has been created for this issue already. |
|
Thanks, @Bidek56 . |
|
@Bidek56 why does the DESCRIPTION matter? it's just a metadata. Also JDK 17 isn't fully supported yet. |
What changes were proposed in this pull request?
This PR proposes to set minimum and maximum Java version specification. (see https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Writing-portable-packages).
Seems there is not the standard way to specify both given the documentation and other packages (see https://gist.github.com/glin/bd36cf1eb0c7f8b1f511e70e2fb20f8d).
I found two ways from existing packages on CRAN.
The latter seems closer to other standard notations such as
R (>= 2.14.0), R (>= r56550). So I have chosen the latter way.Why are the changes needed?
Seems the package might be rejected by CRAN. See #25472 (comment)
Does this PR introduce any user-facing change?
No.
How was this patch tested?
JDK 8
./build/mvn -DskipTests -Psparkr clean package ./R/run-tests.sh ... basic tests for CRAN: ............. ...JDK 11
./build/mvn -DskipTests -Psparkr -Phadoop-3.2 clean package ./R/run-tests.sh ... basic tests for CRAN: ............. ...