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

[NO_MERGE] [JENKINS-54905] - Experimental branch for Java 11 support #41

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
4 participants
@oleg-nenashev
Copy link
Member

commented Nov 30, 2018

Currently it crashes, but I am staging what I was able to achieve.

@jenkinsci/java11-support @varyvol @fcojfernandez

@@ -14,14 +14,14 @@ buildSettings:
source:
dir: ../..
docker:
base: "jenkins/jenkins:2.138.2"
base: "jenkins/jenkins-experimental:latest-jdk11"

This comment has been minimized.

Copy link
@batmat

batmat Nov 30, 2018

Member

Don't you want to use jenkins/jenkins now we got the official JDK 11 support there, like you did above?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Nov 30, 2018

Author Member

agreed

@@ -1,7 +1,7 @@
ARG JENKINS_VERSION=2.121.1
ARG JENKINS_VERSION=jdk11

This comment has been minimized.

Copy link
@batmat

batmat Nov 30, 2018

Member

Do you really intend to modify JFR's master branch with using JDK11 as default? Or maybe we don't care because packagers are always expected to pass this ARG?

@@ -1 +1,2 @@
pipeline-model-definition:latest
workflow-support:incrementals;org.jenkins-ci.plugins.workflow;2.21-rc591.43d37d4d080a

This comment has been minimized.

Copy link
@batmat

batmat Nov 30, 2018

Member

Might make sense to bump to 2.23-rc674.73bb98187744 for clarity and consistency with the one we ended up releasing and use on Evergreen: https://github.com/jenkins-infra/evergreen/blob/36a60c268be9529c0f5c019b9ac1b0f192b3032a/services/essentials.yaml#L171

context.setClassLoader(getClass().getClassLoader());
String javaVersion = System.getProperty("java.version");

// if (javaVersion.startsWith("1.")) {

This comment has been minimized.

Copy link
@batmat

batmat Nov 30, 2018

Member

Let's remove the commented out code?

Also, if you mean to keep it, then I guess you'd use isPostJava8()

@@ -46,7 +54,16 @@ protected ServletContext createWebServer() throws Exception {
server = new Server(queuedThreadPool);

WebAppContext context = new WebAppContext(warDir.getPath(), contextPath);
context.setClassLoader(getClass().getClassLoader());
String javaVersion = System.getProperty("java.version");

This comment has been minimized.

Copy link
@batmat

batmat Nov 30, 2018

Member

unused, I guess leftover from the new isPostJava8() method introduced above.

// if (javaVersion.startsWith("1.")) {
// context.setClassLoader(getClass().getClassLoader());
// } else {
ClassLoader platform = (ClassLoader) ClassLoader.class.getMethod("getPlatformClassLoader").invoke(null);

This comment has been minimized.

Copy link
@batmat

batmat Nov 30, 2018

Member

Can we have a comment explaining the intent a bit here?

This comment has been minimized.

Copy link
@varyvol

varyvol Jan 3, 2019

Contributor

Yes please!

This comment has been minimized.

Copy link
@fcojfernandez

fcojfernandez Jan 3, 2019

Contributor

IIUC, the goal here is get exactly the same classloader as in previous JDK versions. Since this piece of code is executed always and not only with java11, what will happen if this code is executed with java8? The getPlatformClassLoader is only available from java9 (if I'm not wrong), so it will throw an exception

@@ -14,14 +14,14 @@ buildSettings:
source:
dir: ../..
docker:
base: "jenkins/jenkins:2.138.2"
base: "jenkins/jenkins-experimental:latest-jdk11"

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Nov 30, 2018

Author Member

agreed

// This module is multi-licensed and may be used under the terms
// of any of the following licenses:
//
// LGPL, GNU Lesser General Public License, V2 or later, http://www.gnu.org/licenses/lgpl.html

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Nov 30, 2018

Author Member

Note that this code is not compatible w.r.t the license. I put it here only for the experiment (which does not work so far), and it should not be merged as is

@oleg-nenashev oleg-nenashev changed the title [JENKINS-54905] - Experimental branch for Java 11 support [NO_MERGE] [JENKINS-54905] - Experimental branch for Java 11 support Nov 30, 2018

@batmat

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

This is now conflicted FTR.

@varyvol

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

@oleg-nenashev how do you want to proceed with this PR?

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

@varyvol I think the best way is to...

  1. Backup the branch in my local fork
  2. Delete the jdk11 branch from this repo so that we can reuse it for actual changes
@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

Moved the code to https://github.com/oleg-nenashev/jenkinsfile-runner/tree/jdk11-experimental . After patches by @varyvol I doubt these patches will be actually needed

@oleg-nenashev oleg-nenashev deleted the jdk11 branch Jan 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.