Skip to content

Disposer.requiresWorkspace is final#134

Merged
dwnusbaum merged 3 commits intojenkinsci:masterfrom
jglick:Disposer.requiresWorkspace
Nov 5, 2020
Merged

Disposer.requiresWorkspace is final#134
dwnusbaum merged 3 commits intojenkinsci:masterfrom
jglick:Disposer.requiresWorkspace

Conversation

@jglick
Copy link
Copy Markdown
Member

@jglick jglick commented Oct 23, 2020

#123 mistakenly kept an override of Disposer.requiresWorkspace, which in the ultimate version of the API was not necessary (this method need only be overridden in SimpleBuildWrapper), and is in fact final. Because of reflection, this mistake was not noticed until PCT uncovered it in jenkinsci/bom#339.

@jglick jglick requested review from Zastai and dwnusbaum October 23, 2020 11:04
@jglick jglick added the tests label Oct 23, 2020
jglick added a commit to jglick/bom that referenced this pull request Oct 23, 2020
Copy link
Copy Markdown
Contributor

@Zastai Zastai left a comment

Choose a reason for hiding this comment

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

Not sure it makes sense to do this without also changing the other TODOs related to 2.258, but I have nothing against it either.

@Zastai
Copy link
Copy Markdown
Contributor

Zastai commented Oct 23, 2020

Looks like with this change, the test fails on Jenkins < 2.258 (not unexpected) because SimpleBuildWrapper always requires a workspace there.

@jglick
Copy link
Copy Markdown
Member Author

jglick commented Oct 23, 2020

A follow up can bump the baseline to 2.263 and delete reflection, but I thought I would file the minimal patch needed for PCT.

@jglick
Copy link
Copy Markdown
Member Author

jglick commented Oct 23, 2020

the test fails on Jenkins < 2.258

Hmm because of

final Method requiresWorkspace = this.disposer.getClass().getMethod("requiresWorkspace");
. So the attempt to make this system usable by plugins with an older core dep never actually worked.

@jglick
Copy link
Copy Markdown
Member Author

jglick commented Oct 23, 2020

Time to get rid of reflection. I am just going to update the baseline already.

@jglick jglick requested a review from Zastai October 23, 2020 11:36
@jglick jglick added dependencies and removed tests labels Oct 23, 2020
Copy link
Copy Markdown
Contributor

@Zastai Zastai left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread src/main/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStep.java Outdated
@dwnusbaum
Copy link
Copy Markdown
Member

ReadWriteFileStepTest.testBinaryFileRoundtrip failed on Windows with CompositeIOException: Unable to delete 'C:\Users\jenkins\Work\workspace\rkflow-basic-steps-plugin_PR-134\target\tmp\j h6257972212631507846'. Tried 3 times (of a maximum of 3) waiting 0.1 sec between attempts., will try rebuilding.

@dwnusbaum dwnusbaum closed this Nov 4, 2020
@dwnusbaum dwnusbaum reopened this Nov 4, 2020
@dwnusbaum dwnusbaum merged commit 07b2ab1 into jenkinsci:master Nov 5, 2020
@jglick jglick deleted the Disposer.requiresWorkspace branch November 5, 2020 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants