add Java 1.8 easyconfig that can be updated in-place to more recent JDK 1.8.x #6712
Conversation
| @@ -12,6 +12,6 @@ sources = ['%(namelower)s-%(version)s-dist.zip'] | |||
| checksums = ['c730593916ef0ba62f3d113cc3a268e45f7e8039daf7b767c8641b6999bd49b1'] | |||
|
|
|||
| builddependencies = [('binutils', '2.30')] | |||
| dependencies = [('Java', '1.8.0_162', '', True)] | |||
There was a problem hiding this comment.
Note that this might not merge if the currently pending TensorFlow 1.10 gets merged first, since it uses _172
There was a problem hiding this comment.
I'm aware of that, I'm hoping #6677 gets merged first, and then I'll resolve the conflict here.
There was a problem hiding this comment.
If it doesn't, that's fine, then @akesandgren just need to update #6677 accordingly, the change will be trivial.
|
@boegel I have changed my mind about the wrapping. I was young back then... |
|
@vanzod So, to be clear, no objections from your side on this? |
| easyblock = 'Bundle' | ||
|
|
||
| name = 'Java' | ||
| version = '1.8' |
There was a problem hiding this comment.
Hmm, would 8 make more sense here? Whatever the source tarball may be named, the actual version is actually Java 8, right?
There was a problem hiding this comment.
I don't really care....
[j-perdue@tlogin-0502 (08:50) ~]$ ml Java/1.8.0
[j-perdue@tlogin-0502 (08:50) ~]$ java -version
java version "1.8.0_172"
Java(TM) SE Runtime Environment (build 1.8.0_172-b11)
Java HotSpot(TM) 64-Bit Server VM (build 25.172-b11, mixed mode)
I just went with the naming standard that EB already used. 1.8 seems to make some sense.
There was a problem hiding this comment.
might be a good idea for 9
[j-perdue@login2 (08:59) ~]$ ml Java/9.0
[j-perdue@login2 (09:00) ~]$ java -version
java version "9.0.4"
Java(TM) SE Runtime Environment (build 9.0.4+11)
Java HotSpot(TM) 64-Bit Server VM (build 9.0.4+11, mixed mode)
There was a problem hiding this comment.
This thought came up mostly due to the new Java 10, jumping from 1.8 to 10 would be sort of strange.
Also, I think using ('Java', '8') would make it more clear that we're using a wrapper, since you'd expect a major.minor...
There was a problem hiding this comment.
I went with 'java -version'... makes sense for 9 and 10 but Oracle was still calling it 1.8 even if every one else called it Java 8 (like they've have through at least five other past major releases). I'd stick to upstream versioning.
There was a problem hiding this comment.
At www.oracle.com/technetwork/java/javase/downloads they refer to it as Java SE 8 and Java 8u181.
What java -version prints is probably mostly a concern with backwards compatibility, which they broke in Java 9.x
There was a problem hiding this comment.
your call... I don't care either way
|
@boegel I'm a bit confused - why are all these other changes in here now? |
|
@verdurin Hmm, not sure what went wrong there... The other commits were all stuff that's already in |
| @@ -12,6 +12,6 @@ sources = ['%(namelower)s-%(version)s-dist.zip'] | |||
| checksums = ['c730593916ef0ba62f3d113cc3a268e45f7e8039daf7b767c8641b6999bd49b1'] | |||
|
|
|||
| builddependencies = [('binutils', '2.30')] | |||
| dependencies = [('Java', '1.8.0_162', '', True)] | |||
| easyblock = 'Bundle' | ||
|
|
||
| name = 'Java' | ||
| version = '8' |
There was a problem hiding this comment.
This makes quite a lot of changes all in one go - I feel this should be made more prominent somehow.
There was a problem hiding this comment.
Can you clarify? All we're changing here is using a wrapper for Java...
This change in policy will be clearly mentioned in the EasyBuild release notes, and maybe we should also highlight it with a separate post on the mailing list...
There was a problem hiding this comment.
Just meant that we're going to change the way the Java version is reported at the same time. I would agree with the mailing list post.
There was a problem hiding this comment.
OK, I just think it's good to start using this versioning scheme now with a switch to the wrapper... I've always found the 1.8.x versioning a bit awkward since it was Java (SE) 8.
Also, the JDK will still be versioned according to how the sources are versioned.
That said, I'm happy to roll this back to Java/1.8 if people think that makes more sense.
In any case, for Java 10.x it'll likely be Java/10...
There was a problem hiding this comment.
I don't disagree, just noting that we're making lots of simultaneous changes, so we should communicate this as widely as possible.
|
As highlighted by @vanzod, the current approach using using a So, if we want to go through with this we need to do it without a wrapper: a The downside of this approach is that it will take a little bit longer to do an in-place update using I see no other option, so we should discuss whether or not to go through with this. |
|
I don't like it. I specifically wrote my version for quick rebuild of the module while in live use. If "module save" has problems then lets talk to Robert about it. |
|
And I can update the damn versions... its only taken 10 months for this issue to come to some (wrong) resolution. |
|
@JackPerdue We can't change the policy w.r.t. how we deal with I consider the problem that @vanzod brought up w.r.t. module collections a blocker for going through with the wrapper approach: this would hit any user relying on module collections as soon as the We can let @rtmclay pitch in on this, but I'm not sure it matters too much: the check that Lmod does makes sense to me, since the user can't restore the module collection anymore exactly as it was saved... As already mentioned, the other option we have is to simply change the versioning scheme of the Your suggested approach is better since it only involves rewriting a module file for the wrapper, so it's fast, while now updating Come to think of it, another downside of going forward without the wrapper is that we'll effectively be removing an existing That may just be an academic concern though, reinstalling |
…proach, since that break restoring of module collections
|
Thanks to a suggestion by @mboisson, it looks like we do have a better option after all: effectively making |
|
Test report by @boegel |
|
Test report by @boegel |
|
test report coming soon, and also manually tested other easyconfigs (e.g. octave), looks good @bartoldeman did you mean to review this yourself? |
|
Test report by @migueldiascosta |
|
I am working on a test report myself, should be there in 2 hours or so
…On Fri, Sep 21, 2018, 05:48 Miguel Dias Costa ***@***.***> wrote:
Test report by @migueldiascosta <https://github.com/migueldiascosta>
*SUCCESS*
Build succeeded for 5 out of 5 (5 easyconfigs in this PR)
grc-cluster1 - Linux centos 6.9, Intel(R) Xeon(R) CPU E5-2640 0 @ 2.50GHz,
Python 2.7.14
See https://gist.github.com/173a52f3c8220195485142a40ff2cb92 for a full
test report.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6712 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKJFhiH6HZhaDuFbS7xu0l9ACMz1IIgqks5udLX9gaJpZM4V_ci8>
.
|
|
Test report by @bartoldeman |
|
going in, thanks @boegel ! |
cfr. discussion in #5203
I feel we should use this approach going forward (starting with the
*2018b*toolchain generation), but we shouldn't change older easyconfigs to also adapt to this policy since that will lead to too much confusion.We can easily switch to this policy for the
2018btoolchain generation since currently only 1 easyconfig already merged indevelopis affected (Bazel), and it wasn't included in an EasyBuild release yet.cc @JackPerdue
@verdurin, @vanzod: Since you were critical about following this approach, I'd like to hear back from you before this gets merged...
edit: now requires
easybuilders/easybuild-easyblocks#1503