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-55681] - Make JAXB a detached plugin only on Java 11 #3865

Merged
merged 15 commits into from Feb 3, 2019

Conversation

@batmat
Copy link
Member

commented Jan 25, 2019

JENKINS-55681

  • First commit refactors ClassicPluginStrategy to move the detached plugins logic into a dedicated DetachedPluginsManager class
  • Second commit introduces code to allow a plugin to be detached only if the Java runtime greather or equal to a given version.

Proposed changelog entries

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

@jglick

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

Try fiddling with core/src/main/resources/jenkins/split-plugin-cycles.txt? Or implement JENKINS-28942 already?

@batmat batmat changed the title [WIP] Make JAXB a detached plugin only on Java 11 Make JAXB a detached plugin only on Java 11 Jan 27, 2019

batmat added some commits Jan 25, 2019

Move detached plugins logic into a dedicated class
ClassicPluginStrategy is already long enough, so trying to make things a
bit more focused before adding Java 11 logic. (and even if not, the change
still splits a class that is almost 1k lines).

@batmat batmat force-pushed the batmat:detach-jaxb-on-Java-11 branch 2 times, most recently from afd2f53 to 7cca69c Jan 27, 2019

@batmat

This comment has been minimized.

Copy link
Member Author

commented Jan 27, 2019

Given the PR was conflicted, I chose to rebase and force-push it while I was marking it a non-WIP anymore. This is ready for review.

@oleg-nenashev
Copy link
Member

left a comment

Need to test it manually before approving. Why did you need a massive refactoring to the utility class in this PR?

@batmat

This comment has been minimized.

Copy link
Member Author

commented Jan 28, 2019

Why did you need a massive refactoring to the utility class in this PR?

Please see the first commit. I made sure to have a separate one without functional change, and it was essentially a cut and paste. Simply because the code in this class was soon 1k lines, and I feel that the logic around detache plugins was a nice and simple case to have separate and slightly raise the understandability/maintainability.

@batmat batmat closed this Jan 28, 2019

@batmat batmat reopened this Jan 28, 2019

@MRamonLeon
Copy link
Contributor

left a comment

A couple of comments

*/
@Restricted(NoExternalUse.class)
public static @Nonnull
List<DetachedPlugin> getDetachedPlugins() {
return DETACHED_LIST;
return DETACHED_LIST.stream()

This comment has been minimized.

Copy link
@MRamonLeon

MRamonLeon Jan 28, 2019

Contributor

Can't this be done in the initialization of the DETACHED_LIST directly? If possible, we don't need extra changes along the class changing DETACHED_LIST by getDetachedPlugins

This comment has been minimized.

Copy link
@batmat

batmat Jan 28, 2019

Author Member

Yeah, I considered it already, not 100% sure why I chose this path. I'll have another look.

Show resolved Hide resolved core/src/main/java/jenkins/plugins/DetachedPluginsManager.java Outdated
Remove duplicate @restricted
Given the enclosing class already has it.
@batmat

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2019

Just merged with master to retrigger a build and see if I can finally get an Incremental release now it seems to have been fixed. Also because this was marked as failing on CI, for I believe a reason unrelated to the change at stake here.

@oleg-nenashev
Copy link
Member

left a comment

https://github.com/jenkinsci/jenkins/pull/3865/files#r251290617 needs to be resolved somehow. I also do not think that a massive refactoring was needed in this PR, but it is too late. Generally it's better to have it in a separate PR

batmat added some commits Jan 30, 2019

Rename DetachedPluginsManager as DetachedPluginsUtil
as requested by Oleg and Jesse. Mainly because of the misleading
relationship (there's no inheritance) with the existing `PluginManager`
class.
@batmat

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2019

I fully agree about the refactoring as a separate PR. TBH my plan was to do it initially, but I was quite struggling internally as to also provide a demonstration of where this would be useful, hence the additional commits.

But again, granted, I will be more careful next time, as I'm a strong proponent for this too.
I can also totally rebase all the things here and force push. I'm just not doing up front to not confuse people who already spent time reviewing here (because GH would then present everything as new).

@batmat

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2019

So for the record, in addition to the existing tests, my manual testing does confirm this works, at least on a fresh instance. I just confirmed the following works perfectly:

  • Start a fresh Jenkins instance using a build of Jenkins WAR for this PR, on Java 11 runtime
  • install SlocCount plugin as usual from the plugin manager UI, see that JAXB gets installed automatically, despite the plugin does not declare JAXB dependency
  • Use the plugin, and see that there's not ClassNotFoundException anymore

What I plan to confirm works too, and seems critical, but didn't test yet:

upgrade on previous version of runtime & Jenkins

  • run on Java 8 a previous Jenkins version, configure the job and check this works fine
  • update to Java 11 on the WAR for this PR
  • check the JAXB plugin gets installed and the job still works

upgrade of Java runtime only

  • Run PR's WAR on Java 8, configure the job, etc.
  • Upgrade Java runtime to 11 only
  • check the JAXB plugin gets installed and the job still works
@batmat

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2019

For anyone interested, I recorded a video demonstrating the effect of this PR: https://www.youtube.com/watch?v=OWfurCu-ODU

See also https://github.com/batmat/jaxb-java11-demo#use-this-image for an easy way to test it yourself

@oleg-nenashev
Copy link
Member

left a comment

Looks solid && almost ready to go, thanks @batmat . I added few comments/proposals

@@ -864,6 +705,5 @@ protected Class defineClassFromData(File container, byte[] classData, String cla
}

public static boolean useAntClassLoader = SystemProperties.getBoolean(ClassicPluginStrategy.class.getName()+".useAntClassLoader");
private static final Logger LOGGER = Logger.getLogger(ClassicPluginStrategy.class.getName());

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jan 30, 2019

Member

CC @varyvol @fcojfernandez This change likely invalidates the logging tweak in jenkinsci/jenkinsfile-runner#49 / JENKINS-54425 . I am not sure which logger is used in line 692 after this removal

Show resolved Hide resolved core/src/main/resources/jenkins/split-plugins.txt Outdated
Show resolved Hide resolved core/src/main/java/jenkins/plugins/DetachedPluginsUtil.java
Show resolved Hide resolved core/src/main/resources/jenkins/split-plugin-cycles.txt

oleg-nenashev and others added some commits Jan 31, 2019

Clarify minimum Java version column
Co-Authored-By: batmat <bmathus@gmail.com>
Merge branch 'master' into detach-jaxb-on-Java-11
Also to trigger the creation of an Incremental release.
@oleg-nenashev
Copy link
Member

left a comment

Looks good to me. I will create a follow to JFR to restore the logging supression after this change is landed

@alecharp
Copy link
Member

left a comment

just one / two questions of code style but overall LGTM.

* @see JavaUtils#getCurrentJavaRuntimeVersionNumber()
*/
public static @Nonnull
List<DetachedPlugin> getDetachedPlugins() {

This comment has been minimized.

Copy link
@alecharp

alecharp Jan 31, 2019

Member

why this format of method definition? It wasn't like that before either.

This comment has been minimized.

Copy link
@batmat

batmat Feb 1, 2019

Author Member

Cut and paste then reformatted automatically indeed.

This comment has been minimized.

Copy link
@batmat

batmat Feb 1, 2019

Author Member

I also quite dislike this format a bit BTW, but as it was this way before, I'm inclined to say: why not fix it, but rather in a followup.

This comment has been minimized.

Copy link
@jglick

jglick Feb 1, 2019

Member

Sounds like there was a bug in someone’s IDE, may as well correct it now.

* @see #getDetachedPlugins()
*/
public static @Nonnull
List<DetachedPlugin> getDetachedPlugins(@Nonnull VersionNumber since) {

This comment has been minimized.

Copy link
@alecharp

alecharp Jan 31, 2019

Member

same question

Show resolved Hide resolved core/src/main/java/jenkins/plugins/DetachedPluginsUtil.java
}

@Override
public String toString() {

This comment has been minimized.

Copy link
@alecharp

alecharp Jan 31, 2019

Member

I'd have a small preference to have the toString method after all getters. But it really doesn't matter.

@MRamonLeon
Copy link
Contributor

left a comment

Good job!

@fcojfernandez
Copy link
Contributor

left a comment

LGTM

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

As discussed at the @jenkinsci/sig-platform meeting yesterday, we are interested to land this pull request into the incoming weekly so that there is an opportunity to have Java 11 support in the next LTS baseline (maybe in .1 or .2)

@jenkinsci/core please let me know if there any concerns about it being merged.

If no, I will probably land it tomorrow

assertFalse( "weird, split-plugins.txt only has comments?", linesWithoutComments.isEmpty());

//
assertFalse("no whitespaces only lines allowed" ,linesWithoutComments.stream()

This comment has been minimized.

Copy link
@alecharp

alecharp Feb 1, 2019

Member
Suggested change
assertFalse("no whitespaces only lines allowed" ,linesWithoutComments.stream()
assertFalse("no whitespaces only lines allowed", linesWithoutComments.stream()
.anyMatch(line -> !line.isEmpty()));


assertTrue( "max 4 columns is supported", linesWithoutComments.stream()

This comment has been minimized.

Copy link
@alecharp

alecharp Feb 1, 2019

Member
Suggested change
assertTrue( "max 4 columns is supported", linesWithoutComments.stream()
assertTrue("max 4 columns is supported", linesWithoutComments.stream()
@jglick

jglick approved these changes Feb 1, 2019

Copy link
Member

left a comment

Minor comments but looks good overall.

*/
@Deprecated // since TODO

This comment has been minimized.

Copy link
@jglick

jglick Feb 1, 2019

Member

We usually use FIXME for planned @since tags (easier to search for), but anyway I think there is no need to give a date/version on this. Just replace @see with a @deprecated tag.

@@ -483,7 +324,7 @@ public void initializeComponents(PluginWrapper plugin) {
* See {@link ExtensionFinder#scout(Class, Hudson)} for the dead lock issue and what this does.
*/
if (LOGGER.isLoggable(Level.FINER))
LOGGER.log(Level.FINER,"Scout-loading ExtensionList: "+type, new Throwable());
LOGGER.log(Level.FINER, "Scout-loading ExtensionList: "+type, new Throwable());

This comment has been minimized.

Copy link
@jglick

This comment has been minimized.

Copy link
@batmat

batmat Feb 1, 2019

Author Member

🤦‍♂ unrelated ide reformatting I'll have missed I think ..

@@ -0,0 +1,224 @@
package jenkins.plugins;

This comment has been minimized.

Copy link
@jglick

jglick Feb 1, 2019

Member

license header

* <p>
* This code was originally moved from {@link ClassicPluginStrategy}.
*
* @since TODO

This comment has been minimized.

Copy link
@jglick

jglick Feb 1, 2019

Member

Omit for a @Restricted class—it is nobody’s business when it happened to be added.

}

/**
* Get the list of all plugins that have ever been {@link DetachedPlugin detached} from Jenkins core, applicable to the current Java runtime.

This comment has been minimized.

Copy link
@jglick

jglick Feb 1, 2019

Member

@linkplain?

* </li>
* </ul>
*/
@Restricted(NoExternalUse.class)

This comment has been minimized.

Copy link
@jglick

jglick Feb 1, 2019

Member

Unnecessary: the enclosing class was already restricted.

* Gets the minimum required version for the current version of Jenkins.
*
* @return the minimum required version for the current version of Jenkins.
* @since 2.16

This comment has been minimized.

Copy link
@jglick

jglick Feb 1, 2019

Member

delete

@@ -1,6 +1,6 @@
# See ClassicPluginStrategy.DETACHED_LIST. As of JENKINS-47634 also used by plugin-compat-tester.

# Columns are: plugin ID, last core release still containing the plugin's functionality, plugin version that's implied.
# Columns are: plugin ID, last core release still containing the plugin's functionality, plugin version that's implied, minimum Java specification version where to install the plugin

This comment has been minimized.

Copy link
@jglick

jglick Feb 1, 2019

Member

Getting harder to read—would suggest moving to JSON but, no comments! And we have no YAML lib in jenkins-core AFAIK. Sigh…

@@ -38,20 +39,20 @@

@Test
public void test_getDetachedPlugins() {

This comment has been minimized.

Copy link
@jglick

jglick Feb 1, 2019

Member

Does this just belong in DetachedPluginsUtilTest now?

}

/**
* Checks the format of the <code>/jenkins/split-plugins.txt</code> file has maximum 4 columns.

This comment has been minimized.

Copy link
@jglick

jglick Feb 1, 2019

Member

Could just enforce these things with assertions while parsing.

@oleg-nenashev oleg-nenashev changed the title Make JAXB a detached plugin only on Java 11 [JENKINS-55681] - Make JAXB a detached plugin only on Java 11 Feb 3, 2019

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

I have created https://issues.jenkins-ci.org/browse/JENKINS-55930 as a follow-up so that @jglick's comments can be addressed after the merge

Since there is no negative votes in the PR, I will proceed with merge. It will produce experimental Docker images which can be used for for testing of outstanding use-cases like upgradeability

@oleg-nenashev oleg-nenashev merged commit a78312e into jenkinsci:master Feb 3, 2019

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@batmat batmat deleted the batmat:detach-jaxb-on-Java-11 branch Feb 3, 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.