Fix `RV_RETURN_VALUE_IGNORED_BAD_PRACTICE` SpotBugs violations #6025

basil · 2021-12-07T04:21:16Z

Fixes most RV_RETURN_VALUE_IGNORED_BAD_PRACTICE, mostly by switching to NIO.2 where possible.

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

(If applicable) Jira issue is well described
Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
- Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
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

@StefanSpieker

Maintainer checklist

Before the changes are marked as ready-for-merge:

There are at least 2 approvals for the pull request and no outstanding requests for change
Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
Changelog entries in the PR title and/or Proposed changelog entries are correct
Proper changelog labels are set so that the changelog can be generated automatically
If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

core/src/main/java/hudson/model/labels/LabelAtom.java

basil · 2021-12-09T06:05:08Z

This PR was tested successfully in a passing Windows CI run in #6036.

…ORED_BAD_PRACTICE

basil · 2021-12-09T22:15:24Z

This PR was tested successfully in a passing BOM run in jenkinsci/bom#757.

jglick

Thanks—long overdue! Some places I think we wanted to recover gracefully rather than throwing an exception (but silently ignoring the error was bad).

core/src/main/java/hudson/PluginManager.java

core/src/main/java/hudson/WebAppMain.java

jglick · 2021-12-09T22:51:38Z

core/src/main/java/hudson/model/FileParameterValue.java

+            try {
+                Files.deleteIfExists(file.toPath());
+            } catch (IOException e) {
+                throw new UncheckedIOException(e);


Maybe just log instead? I do not think callers would be expecting an unchecked exception here.

Just a few lines above, we convert IOException into Error, so I feel like all bets are off with this code path. I made both that path and this one throw UncheckedIOException, as at least that seems better than Error.

jglick · 2021-12-09T22:52:51Z

core/src/main/java/hudson/model/UpdateCenter.java

@@ -1882,12 +1883,8 @@ protected void _run() throws IOException, InstallationStatus {
         */
        protected void replace(File dst, File src) throws IOException {
            File bak = Util.changeExtension(dst,".bak");
-            bak.delete();
-            dst.renameTo(bak);
-            dst.delete(); // any failure up to here is no big deal


Note: behavioral change.

Yeah, this seemed like more straightforward behavior to me.

core/src/main/java/hudson/util/io/ReopenableRotatingFileOutputStream.java

core/src/main/java/hudson/util/io/RewindableRotatingFileOutputStream.java

core/src/main/java/jenkins/fingerprints/FileFingerprintStorage.java

src/spotbugs/spotbugs-excludes.xml

…ORED_BAD_PRACTICE

basil · 2021-12-10T20:39:07Z

core/src/main/java/hudson/XmlFile.java

+    public void mkdirs() throws IOException {
+        Files.createDirectories(Util.fileToPath(file.getParentFile()));


Technically an API change, but there are no plugin consumers of this API, and all the consumers in core were already throwing IOException anyway.

basil · 2021-12-10T20:41:02Z

core/src/main/java/hudson/util/TextFile.java

+    public void delete() throws IOException {
+        Files.deleteIfExists(Util.fileToPath(file));
    }


Technically an API change, but there are no plugin consumers of this API, and all the consumers in core were already throwing IOException anyway.

basil · 2021-12-10T20:45:28Z

core/src/main/java/hudson/XmlFile.java

+    public void delete() throws IOException {
+        Files.deleteIfExists(Util.fileToPath(file));
    }


Technically an API change that affects 7 plugins at compile-time, but I don't think the addition of the throws should affect those seven plugins at runtime. I think this is probably fine, but if people feel strongly otherwise I can convert this to an UncheckedIOException in order to preserve the existing method signature for those 7 plugins.

Which plugins?

@jglick Did you have any opinion about this part? This is what I'm the most unsure about, so would be good to hear if you think this is OK or if you'd prefer the UncheckedIOException approach here.

Probably fine—may cause a compilation error on upgrade, but trivially resolved.

copyartifact ecutest gerrit-trigger google-container-registry-auth jqs-monitoring nodejs nuget xvfb

jglick

Did not do a very close review but looks good.

basil · 2021-12-11T00:59:01Z

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!

…ORED_BAD_PRACTICE

Compatibility with jenkinsci/jenkins#6025

Compatibility with jenkins-2.325 and later (jenkinsci/jenkins#6025)

timja · 2022-01-18T08:44:20Z

core/src/main/java/hudson/util/TextFile.java

@@ -96,7 +96,7 @@ public String read() throws IOException {
     * Overwrites the file by the given string.
     */
    public void write(String text) throws IOException {
-        file.getParentFile().mkdirs();
+        Files.createDirectories(Util.fileToPath(file.getParentFile()));


caused https://issues.jenkins.io/browse/JENKINS-67624

I guess createDirectories was meant here?

basil added the skip-changelog Should not be shown in the changelog label Dec 7, 2021

basil force-pushed the RV_RETURN_VALUE_IGNORED_BAD_PRACTICE branch 3 times, most recently from 50bc009 to c6814e9 Compare December 8, 2021 22:42

basil mentioned this pull request Dec 9, 2021

Restore Windows CI runs #6024

Merged

Fix RV_RETURN_VALUE_IGNORED_BAD_PRACTICE SpotBugs violations

d0f5060

basil force-pushed the RV_RETURN_VALUE_IGNORED_BAD_PRACTICE branch from c6814e9 to d0f5060 Compare December 9, 2021 03:01

basil commented Dec 9, 2021

View reviewed changes

core/src/main/java/hudson/model/labels/LabelAtom.java Outdated Show resolved Hide resolved

timja approved these changes Dec 9, 2021

View reviewed changes

StefanSpieker approved these changes Dec 9, 2021

View reviewed changes

basil added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Dec 9, 2021

Merge remote-tracking branch 'origin/master' into RV_RETURN_VALUE_IGN…

354de20

…ORED_BAD_PRACTICE

basil added a commit to basil/bom that referenced this pull request Dec 9, 2021

Testing jenkinsci/jenkins#6025

475dd52

basil requested a review from a team December 9, 2021 22:15

jglick reviewed Dec 9, 2021

View reviewed changes

basil added 2 commits December 10, 2021 12:14

Review feedback

8509f37

Merge remote-tracking branch 'origin/master' into RV_RETURN_VALUE_IGN…

20728cd

…ORED_BAD_PRACTICE

basil commented Dec 10, 2021

View reviewed changes

jglick approved these changes Dec 10, 2021

View reviewed changes

basil requested a review from jglick December 10, 2021 20:45

basil added a commit to basil/copyartifact-plugin that referenced this pull request Dec 10, 2021

Compatibility with jenkinsci/jenkins#6025

5066ecc

basil added a commit to basil/copyartifact-plugin that referenced this pull request Dec 10, 2021

Compatibility with jenkinsci/jenkins#6025

ddfdf1c

basil mentioned this pull request Dec 10, 2021

Compatibility with jenkins-2.325 and later (jenkinsci/jenkins#6025) jenkinsci/copyartifact-plugin#143

Merged

basil added a commit to basil/xvfb-plugin that referenced this pull request Dec 10, 2021

Compatibility with jenkinsci/jenkins#6025

5095e15

basil mentioned this pull request Dec 10, 2021

Compatibility with jenkinsci/jenkins#6025 jenkinsci/xvfb-plugin#15

Merged

basil added 2 commits December 10, 2021 14:46

No need to use atomic move for logs

89b38c8

Synchronize UpdateCenter operations

b1d7bb8

basil added 3 commits December 10, 2021 14:46

Gracefully fall back to non-atomic move if atomic move is unsupported

047a7fe

No need to use atomic move

fd2b622

Remove unnecessary copy options

8ba0b3c

basil added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 11, 2021

Merge remote-tracking branch 'origin/master' into RV_RETURN_VALUE_IGN…

8f37630

…ORED_BAD_PRACTICE

basil merged commit 165d559 into jenkinsci:master Dec 11, 2021

basil deleted the RV_RETURN_VALUE_IGNORED_BAD_PRACTICE branch December 11, 2021 20:51

zregvart added a commit to jenkinsci/xvfb-plugin that referenced this pull request Dec 13, 2021

Merge pull request #15 from basil/IOException

88cb84e

Compatibility with jenkinsci/jenkins#6025

basil mentioned this pull request Dec 15, 2021

[JENKINS-67372] Restore support for symbolic links in the Jenkins home directory #6059

Merged

10 tasks

ikedam added a commit to jenkinsci/copyartifact-plugin that referenced this pull request Dec 19, 2021

Merge pull request #143 from basil/IOException

cce305c

Compatibility with jenkins-2.325 and later (jenkinsci/jenkins#6025)

timja reviewed Jan 18, 2022

View reviewed changes

basil added a commit to basil/bitbucket-branch-source-plugin that referenced this pull request Aug 8, 2022

Adapting to jenkinsci/jenkins#6025

de7fe9c

basil mentioned this pull request Aug 8, 2022

Adapting to https://github.com/jenkinsci/jenkins/pull/6025 jenkinsci/bitbucket-branch-source-plugin#629

Merged

lifeofguenter pushed a commit to jenkinsci/bitbucket-branch-source-plugin that referenced this pull request Aug 9, 2022

Adapting to jenkinsci/jenkins#6025 (#629)

e724eb4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `RV_RETURN_VALUE_IGNORED_BAD_PRACTICE` SpotBugs violations #6025

Fix `RV_RETURN_VALUE_IGNORED_BAD_PRACTICE` SpotBugs violations #6025

basil commented Dec 7, 2021

basil commented Dec 9, 2021

basil commented Dec 9, 2021

jglick left a comment

jglick Dec 9, 2021

basil Dec 10, 2021

jglick Dec 9, 2021

basil Dec 10, 2021

basil Dec 10, 2021

basil Dec 10, 2021

basil Dec 10, 2021

jglick Dec 10, 2021

basil Dec 10, 2021

jglick Dec 10, 2021

basil Dec 10, 2021

jglick left a comment

basil commented Dec 11, 2021

timja Jan 18, 2022

jglick Jan 18, 2022

		public void mkdirs() throws IOException {
		Files.createDirectories(Util.fileToPath(file.getParentFile()));

Fix RV_RETURN_VALUE_IGNORED_BAD_PRACTICE SpotBugs violations #6025

Fix RV_RETURN_VALUE_IGNORED_BAD_PRACTICE SpotBugs violations #6025

Conversation

basil commented Dec 7, 2021

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

basil commented Dec 9, 2021

basil commented Dec 9, 2021

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

basil commented Dec 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix `RV_RETURN_VALUE_IGNORED_BAD_PRACTICE` SpotBugs violations #6025

Fix `RV_RETURN_VALUE_IGNORED_BAD_PRACTICE` SpotBugs violations #6025