-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
[JEP-228] Unforking XStream #4944
Conversation
Seems that the defense in #3248 is no longer necessary as of x-stream/xstream@d0ad410, though it is harmless to leave in. |
@@ -82,8 +81,7 @@ | |||
/** | |||
* {@link ToolProperty}s that are associated with this tool. | |||
*/ | |||
@XStreamSerializable | |||
private transient /*almost final*/ DescribableList<ToolProperty<?>,ToolPropertyDescriptor> properties | |||
private /*almost final*/ DescribableList<ToolProperty<?>,ToolPropertyDescriptor> properties |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a problem. DescribableList
is not Serializable
so removing transient
will prevent this class from being serialized over Remoting. Yet keeping transient
will prevent tool installations from saving their properties in XML. I have not found any way to get the effect of XStreamSerializable
without patching PureJavaReflectionProvider
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I am not sure why ToolInstallation
was ever made Serializable
to begin with. The normal usage is to have it define some PATH
entry on the controller side, which does not require the ToolInstallation
itself to ever be sent to the agent.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the current annotation was explicitly rejected upstream: x-stream/xstream#70 (comment)
Fixing MavenInstallation accordingly. AntInstallation was already fixed: jenkinsci/ant-plugin@c957dca
return this; | ||
} | ||
|
||
protected Object writeReplace() throws Exception { | ||
if (Channel.current() == null) { // XStream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, not nice, but seems to work well enough. I am trying to fix popular plugins to not need this hack.
Would prefer to get at least the following released first:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the pull request template. Changelog and upgrade guide drafts are required for this pull request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, seems to work with random plugins I tried out. Read through the associated JEP-228 docs as well.
Is anyone else wanting to review this or should we ship in the next weekly? |
This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback. Thanks! |
Given the PR size, might make sense to wait a few days longer, since the next weekly is still a week off anyway. |
Co-authored-by: Basil Crow <me@basilcrow.com>
I'll be merging this after #4848 is merged. |
…ns#4944 to be as well, to be released 2020-11-10
Cleanup: INFRA-2794 |
@@ -132,7 +132,7 @@ public void setJobFilters(List<ViewJobFilter> jobFilters) throws IOException { | |||
this.jobFilters.replaceBy(jobFilters); | |||
} | |||
|
|||
private Object readResolve() { | |||
protected Object readResolve() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this imply that all plugins must make the same change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the class is extended and the parent read resolve needs to be called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I attempted to search for extant plugins using an incorrect private
override, but I might have missed some of course.
Unforking XStream required the access level of this method to be protected. Fortunately this means the super method can be called directly instead of using reflection. jenkinsci/jenkins#4944
* refactor: Modernize to latest versions supported by Java 8 Use this link to re-run the recipe: https://app.moderne.io/recipes/org.openrewrite.jenkins.ModernizePluginForJava8?organizationId=SmVua2lucyBDSQ%3D%3D Co-authored-by: Moderne <team@moderne.io> * Raise access modifier of readResolve to protected Unforking XStream required the access level of this method to be protected. Fortunately this means the super method can be called directly instead of using reflection. jenkinsci/jenkins#4944 --------- Co-authored-by: Moderne <team@moderne.io> Co-authored-by: Steve Hill <sghill.dev@gmail.com>
JEP-228
Proposed changelog entries
Proposed upgrade guidelines
https://www.jenkins.io/blog/2020/11/10/spring-xstream/
See the compatibility table.
Desired reviewers
@jenkinsci/core
Maintainer checklist
Before the changes are marked as
ready-for-merge
:Proposed changelog entries
are correctupgrade-guide-needed
label is set and there is aProposed upgrade guidelines
section in the PR title. (example)lts-candidate
to be considered (see query).