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

Subversion plugin doesn't adhere getAffectedPaths() contract

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Critical Critical
    • subversion-plugin
    • None

      While trying to get the maven incremental build to work, I noticed the subversion changesets reported always start at the root of the subversion server.

      In fact, maven (And the contract) expect the changesets to be relative to the checkout url.

      e.g.
      http://subversion/path/to/trunk/
      When changes are done in "modulename" inside trunk, the changeset reports:
      /path/to/trunk/modulename/changedfile.java
      While maven (And the contract) expect:
      modulename/changedfile.java

      This completely breaks the incremental case, and possibly some other (Yet undetected) cases.

          [JENKINS-16045] Subversion plugin doesn't adhere getAffectedPaths() contract

          Mathias Teugels added a comment - See https://github.com/jenkinsci/subversion-plugin/pull/23

          kutzi added a comment -

          In fact, I've successfully used Maven incremental builds with svn-plugin in the past. So you might have a different problem.
          Also I see code in the svn-plugin which seems to handle this already (preparePath method).
          Have you looked at this and can you explain why it does not work in your case?

          kutzi added a comment - In fact, I've successfully used Maven incremental builds with svn-plugin in the past. So you might have a different problem. Also I see code in the svn-plugin which seems to handle this already (preparePath method). Have you looked at this and can you explain why it does not work in your case?

          kutzi added a comment -

          Unfortunately, the contract of getAffectedPath() and getAffectedFiles() is not very clear (at least to me). https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/scm/ChangeLogSet.java

          In fact, it implies that paths should be relative to workspace root, but doesn't say so explicitely.
          Also getAffectedFiles() seems to be differ substantially in the svn implementation, because - AFAI can see - it doesn't use the preparePath method.
          This seems to be a big mess. However, unfortunately I cannot see, how to change this without severely breaking backwards compatibility.

          kutzi added a comment - Unfortunately, the contract of getAffectedPath() and getAffectedFiles() is not very clear (at least to me). https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/scm/ChangeLogSet.java In fact, it implies that paths should be relative to workspace root, but doesn't say so explicitely. Also getAffectedFiles() seems to be differ substantially in the svn implementation, because - AFAI can see - it doesn't use the preparePath method. This seems to be a big mess. However, unfortunately I cannot see, how to change this without severely breaking backwards compatibility.

          At the very least, it's supposed to be normalized to not start with a '/'
          Also, and this might not be seen as the best way of proceeding, but we might want to check out the implementations of other SCM plugins.

          Another source for information might be the usages of the function, where it is used that I know for sure is the maven plugin. This plugin assumes that the normalization at the very least has been taken care of (removing the leading '/'), and also assumes it is a relative path to the workspace. Perhaps this last assumption is wrong and might need fixing there, but in the end, one change we are sure about is the leading '/'!

          Mathias Teugels added a comment - At the very least, it's supposed to be normalized to not start with a '/' Also, and this might not be seen as the best way of proceeding, but we might want to check out the implementations of other SCM plugins. Another source for information might be the usages of the function, where it is used that I know for sure is the maven plugin. This plugin assumes that the normalization at the very least has been taken care of (removing the leading '/'), and also assumes it is a relative path to the workspace. Perhaps this last assumption is wrong and might need fixing there, but in the end, one change we are sure about is the leading '/'!

          Daniel Beck added a comment -

          Possible solution for this issue: https://github.com/jenkinsci/subversion-plugin/pull/80

          I'd like to get some feedback whether this actually works, so please check it out.

          Daniel Beck added a comment - Possible solution for this issue: https://github.com/jenkinsci/subversion-plugin/pull/80 I'd like to get some feedback whether this actually works, so please check it out.

          Code changed in jenkins
          User: Stephen Connolly
          Path:
          src/main/java/hudson/scm/DirAwareSVNXMLLogHandler.java
          src/main/java/hudson/scm/SubversionChangeLogBuilder.java
          src/main/java/hudson/scm/SubversionChangeLogSet.java
          src/test/java/hudson/scm/SubversionChangeLogParserTest.java
          src/test/resources/hudson/scm/changelog_relativepath.xml
          http://jenkins-ci.org/commit/subversion-plugin/5a2683ed4a7d8b384577188cb4733345afb14d71
          Log:
          Merge pull request #80 from daniel-beck/JENKINS-18574

          [RFC] FIX JENKINS-18574/JENKINS-16045

          Compare: https://github.com/jenkinsci/subversion-plugin/compare/6e4e39127eb7...5a2683ed4a7d

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Stephen Connolly Path: src/main/java/hudson/scm/DirAwareSVNXMLLogHandler.java src/main/java/hudson/scm/SubversionChangeLogBuilder.java src/main/java/hudson/scm/SubversionChangeLogSet.java src/test/java/hudson/scm/SubversionChangeLogParserTest.java src/test/resources/hudson/scm/changelog_relativepath.xml http://jenkins-ci.org/commit/subversion-plugin/5a2683ed4a7d8b384577188cb4733345afb14d71 Log: Merge pull request #80 from daniel-beck/ JENKINS-18574 [RFC] FIX JENKINS-18574 / JENKINS-16045 Compare: https://github.com/jenkinsci/subversion-plugin/compare/6e4e39127eb7...5a2683ed4a7d

          James Dumay added a comment -

          Assuming due to the lack of feedback here that the PR merged has fixed this issue. If that is not the case, please reopen.

          James Dumay added a comment - Assuming due to the lack of feedback here that the PR merged has fixed this issue. If that is not the case, please reopen.

            Unassigned Unassigned
            cpf_ Mathias Teugels
            Votes:
            1 Vote for this issue
            Watchers:
            6 Start watching this issue

              Created:
              Updated:
              Resolved: