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

[JENKINS-52282] Add isJavaWebStartSupported to JNLPLauncher #3766

Merged

Conversation

MarkEWaite
Copy link
Contributor

@MarkEWaite MarkEWaite commented Nov 17, 2018

See JENKINS-52282, Java 9 and later do not support Java Web Start.

No automated test because I don't have time to write a test for the existence of a button in the user interface.

Proposed changelog entries

  • Entry 1: Don't show the "Java Web Start" button if master is running on Java 11
  • ...

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@jenkinsci/java11-support

@oleg-nenashev oleg-nenashev added the java11 Java 11 support in Jenkins label Nov 17, 2018
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Thanks a lot @MarkEWaite! Some minor patches are needed IMHO, but I think we can land it in the weekly release on Nov 26

${%Launch agent from browser}
</p>
</li>
</j:if>
Copy link
Member

Choose a reason for hiding this comment

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

else add some warning to the WebUI? Otherwise we may end up with users who report the issue after reading the docs and failing to fond the button

${%Launch agent from browser}
</p>
</li>
<j:if test="${it.launcher.javaWebStartSupported}">
Copy link
Member

Choose a reason for hiding this comment

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

I know it's jelly, but could we incorporate a link to a resource (wiki, blog, etc) to say why the web starter is not available. So having a <j:choose> rather than a <j:if> so we can have an else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll need help from someone else to make those jelly changes. I'm not skilled with jelly and need to spend my weekends and nights working on the git plugin and git client plugin.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's

<j:choose>
  <j:when test="${it.launcher.javaWebStartSupported}">
[...]
  </j:when>
  <j:otherwise>
    <div>There is no Web starter for the JVM version used to run Jenkins</div>
  </j:otherwise>
</j:choose>

Copy link
Member

Choose a reason for hiding this comment

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

Same as https://github.com/jenkinsci/jenkins/pull/3766/files#r234416298 .
I believe we need to have a Wiki or jenkins.io page to list known Jenkins core incompatibilities.
There is https://jenkins.io/doc/administration/requirements/java/ , but probably it worth having a separate page.

Copy link
Member

Choose a reason for hiding this comment

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

Let's just add a redirect page on Jenkins.io? This way we can modify the final destination later?

Like https://jenkins.io/redirect/java-web-start ? Or a more general https://jenkins.io/redirect/java-11 ?

Copy link
Member

Choose a reason for hiding this comment

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

Let's do https://jenkins.io/redirect/java11-java-web-start or so. It can be finally linking to the anchor on a page, but it can just link to the JIRA ticket for now

Copy link
Member

Choose a reason for hiding this comment

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

I agree, +1 on redirect and it can be changed to wiki page in the future. @MarkEWaite do you want me to test / commit the jelly code I provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alecharp that would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the conditional text and redirect link as suggested.

* @since FIXME
*/
public boolean isJavaWebStartSupported() {
return System.getProperty("java.version", "1.8").startsWith("1.8");
Copy link
Member

Choose a reason for hiding this comment

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

I was looking for https://docs.oracle.com/javase/10/docs/api/java/lang/Runtime.Version.html but it's only available since Java 9, sadly. So I guess this startWith is really all we can do here..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I saw as well. I would have preferred a better check, but...

Copy link
Member

Choose a reason for hiding this comment

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

This is a hack we usually do. Probably we need to move these checks to a utility class

@MarkEWaite MarkEWaite force-pushed the hide-java-web-start-on-jdk-11 branch from 0351a49 to 867ccd5 Compare November 19, 2018 14:20
${%Launch agent from browser}
</p>
</li>
<j:if test="${it.launcher.javaWebStartSupported}">
Copy link
Member

Choose a reason for hiding this comment

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

Let's just add a redirect page on Jenkins.io? This way we can modify the final destination later?

Like https://jenkins.io/redirect/java-web-start ? Or a more general https://jenkins.io/redirect/java-11 ?

Seems cleaner to use JNLPLauncher than to use Functions.  Narrows the
scope of the method so that callers know it is specific to
JNLPLauncher.
@MarkEWaite MarkEWaite force-pushed the hide-java-web-start-on-jdk-11 branch from 867ccd5 to f33e5f8 Compare November 23, 2018 21:03
@MarkEWaite
Copy link
Contributor Author

@alecharp I placed your proposed changes (with a phrasing change from me) in as f33e5f8 . If that's not workable for you or if I've made a mistake, feel free to remove that commit or to do whatever else is needed to include the Java Web Start conditional in the next weekly build of Jenkins.

@batmat batmat requested a review from alecharp November 23, 2018 21:10
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

better UI would be nice, but IMO it can be merged once the redirect link is actually created

@oleg-nenashev oleg-nenashev added the on-hold This pull request depends on another event/release, and it cannot be merged right now label Nov 23, 2018
MarkEWaite added a commit to MarkEWaite/jenkins.io that referenced this pull request Nov 23, 2018
Oracle announced in March 2018 that it will not include Java Web Start
in Java SE 11.  Jenkins provides a "Java Web Start" button to simplify
launching of clients with Java Web Start.  That simplified start technique
is often used for Windows agents.  Jenkins on Java 11 can't use that
button because the underlying Java implementation does not support it.

The communication protocol used by agent.jar continues to be fully
supported and operational with Java 11.  Only the Java Web Start button
from the web page is not supported in Java 11.

See [JENKINS-52282] and jenkinsci/jenkins#3766
for more details.
@MarkEWaite
Copy link
Contributor Author

MarkEWaite commented Nov 23, 2018

See the jenkins.io pull request for the proposed redirect to Wikipedia. The Wikipedia destination was chosen because it states the deprecation very simply and includes a link to the authoritative PDF file.

When we add documentation on agents to jenkins.io, we can include Java Web Start and its deprecation in that section. Alternately, it could be placed in a jenkins.io section dedicated to Java 11.

@oleg-nenashev oleg-nenashev removed the on-hold This pull request depends on another event/release, and it cannot be merged right now label Nov 24, 2018
@oleg-nenashev
Copy link
Member

Upstream redirect was integrated, merging

@oleg-nenashev oleg-nenashev merged commit 217b293 into jenkinsci:master Nov 24, 2018
@MarkEWaite MarkEWaite deleted the hide-java-web-start-on-jdk-11 branch November 24, 2018 21:35
* This flag is checked in {@code config.jelly} before displaying the
* Java Web Start button.
* @return {@code true} if Java Web Start button should be displayed.
* @since FIXME
Copy link
Member

Choose a reason for hiding this comment

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

No need to @since something that's @Restricted

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

Successfully merging this pull request may close these issues.

5 participants