Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-47324

FilePath API in Jenkins should propagate errors

      Almost all methods in https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/FilePath.java use obsolete pre-Java7 API, which does not propagate errors.

      • The code should be updated to java.nio.Files: https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html
      • Methods should propagate errors via IOExceptions where possible (and log errors to java.util.logging.Logger otherwise)
      • Runtime exceptions from the new API should be caught if the methods throw them (e.g. InvalidPathException)

          [JENKINS-47324] FilePath API in Jenkins should propagate errors

          Devin Nusbaum added a comment -

          krishbhasin Do you still plan on filing a PR for this?

          Devin Nusbaum added a comment - krishbhasin Do you still plan on filing a PR for this?

          dnusbaum Yeah sorry it's been a busy few weeks (holiday then knocked out with flu). I will get to this before the end of the year, I'm still recovering from the illness.

          Krishan Bhasin added a comment - dnusbaum Yeah sorry it's been a busy few weeks (holiday then knocked out with flu). I will get to this before the end of the year, I'm still recovering from the illness.

          Devin Nusbaum added a comment -

          krishbhasin Ok that sounds good. I hope you're feeling better soon.

          Devin Nusbaum added a comment - krishbhasin Ok that sounds good. I hope you're feeling better soon.

          Code changed in jenkins
          User: Krishan Bhasin
          Path:
          core/src/main/java/hudson/FilePath.java
          core/src/main/java/hudson/util/IOUtils.java
          http://jenkins-ci.org/commit/jenkins/09bcc5d6538b3cfffbf71228ebd1679e3e20d8b2
          Log:
          JENKINS-47324 - Reduce usage of File.mkdirs() in FilePath and IOUtils (#3173)

          • Move an instance of renameTo() to Files.move()
          • Replace an instance of File.toURI() with an instance of Path.toUri()
          • Replace mkdirs() with Files.createDirectories()
            Replace mkdir() with Files.createTempDirectory()
          • Undo addition of createTempDirectory() as per review comments
          • Return to use of FilePath#mkdirs(File) and instead modify it to use the new API.
          • Undo addition of toPath() in a URI conversion as it brings no benefits.
          • Replace new uses of toPath() with Util.fileToPath() to pre-handle runtime exceptions
          • Remove * import.
            move mkdirs() to using FilePath method instead of File method.
          • Make IOUtils.mkdirs(File) use Java7 API calls
          • Add back accidentally removed imports.
          • Fixed use of wildcard import
          • Use utility method fileToPath() to handle potential exception thrown

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Krishan Bhasin Path: core/src/main/java/hudson/FilePath.java core/src/main/java/hudson/util/IOUtils.java http://jenkins-ci.org/commit/jenkins/09bcc5d6538b3cfffbf71228ebd1679e3e20d8b2 Log: JENKINS-47324 - Reduce usage of File.mkdirs() in FilePath and IOUtils (#3173) Move an instance of renameTo() to Files.move() Replace an instance of File.toURI() with an instance of Path.toUri() Replace mkdirs() with Files.createDirectories() Replace mkdir() with Files.createTempDirectory() Undo addition of createTempDirectory() as per review comments Return to use of FilePath#mkdirs(File) and instead modify it to use the new API. Undo addition of toPath() in a URI conversion as it brings no benefits. Replace new uses of toPath() with Util.fileToPath() to pre-handle runtime exceptions Remove * import. move mkdirs() to using FilePath method instead of File method. Make IOUtils.mkdirs(File) use Java7 API calls Add back accidentally removed imports. Fixed use of wildcard import Use utility method fileToPath() to handle potential exception thrown

          Daniel Beck added a comment -

          oleg_nenashev Is this issue fixed towards 2.96 or still WIP?

          Daniel Beck added a comment - oleg_nenashev Is this issue fixed towards 2.96 or still WIP?

          Oleg Nenashev added a comment -

          danielbeck IIUC there are still some changes to be done in this ticket

          Oleg Nenashev added a comment - danielbeck IIUC there are still some changes to be done in this ticket

          I feel like the acceptance criteria are slightly unclear, then it makes it hard to be sure this is done or not. I see at least the PR from krishbhasin and dnusbaum in https://github.com/jenkinsci/jenkins/pull/3135.

           oleg_nenashev given you filed this, *when you have time* could you possibly add acceptance criteria so it's easily assessable by anyone if/when this would be done?

          Spassiba!

          Baptiste Mathus added a comment - I feel like the acceptance criteria are slightly unclear, then it makes it hard to be sure this is done or not. I see at least the PR from krishbhasin and dnusbaum  in https://github.com/jenkinsci/jenkins/pull/3135.   oleg_nenashev given you filed this, * when you have time * could you possibly add acceptance criteria so it's easily assessable by anyone if/when this would be done? Spassiba!

          Baptiste Mathus added a comment - - edited

          krishbhasin would you be BTW be available/willing to handle the possible remainder of this task, if any? We're happy to provide any guidance for this. Thanks!

          Baptiste Mathus added a comment - - edited krishbhasin would you be BTW be available/willing to handle the possible remainder of this task, if any? We're happy to provide any guidance for this. Thanks!

          Devin Nusbaum added a comment -

          Krishan submitted https://github.com/jenkinsci/jenkins/pull/3173, https://github.com/jenkinsci/jenkins/pull/3135 from me was more related to JENKINS-36088. There are likely other things that could be addressed, but I think we got most of the lowest-hanging fruit looking at the related issues and glancing quickly through FilePath.java.

          Devin Nusbaum added a comment - Krishan submitted https://github.com/jenkinsci/jenkins/pull/3173 , https://github.com/jenkinsci/jenkins/pull/3135 from me was more related to JENKINS-36088 . There are likely other things that could be addressed, but I think we got most of the lowest-hanging fruit looking at the related issues and glancing quickly through FilePath.java .

          OK, then let's consider this fixed until proven otherwise. Like I wrote above anyway, it can be reopened anytime once we know about left things to be still fixed. Thanks for the feedback Devin.

          Baptiste Mathus added a comment - OK, then let's consider this fixed until proven otherwise. Like I wrote above anyway, it can be reopened anytime once we know about left things to be still fixed. Thanks for the feedback Devin.

            krishbhasin Krishan Bhasin
            oleg_nenashev Oleg Nenashev
            Votes:
            1 Vote for this issue
            Watchers:
            7 Start watching this issue

              Created:
              Updated:
              Resolved: