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

CVS plugin excluded regions no longer match on the file path

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Resolved (View Workflow)
    • Priority: Major
    • Resolution: Fixed
    • Component/s: cvs-plugin
    • Labels:
      None
    • Similar Issues:

      Description

      After upgrading from version 1.6 of the CVS plugin to 2.6 (and then to 2.7), our "excludes regions" have stopped working. That's because we have a pattern like

      [^/]*\.ext
      

      to prevent changes to .ext files in the root directory of the checkout from triggering builds, and we have patterns like

      directoryToExclude/.*
      

      to prevent any file in directoryToExclude from triggering a build. But we do expect a build to be triggered by changes to a .ext file that is in a directory that is not excluded. That was true of version 1.6, but not after upgrading to 2.6.

      Not that you don't already know this, but looking at the source code for version 2.6 of the CVS plugin, it looks like the CvsFile is constructed (in CvsLog) using only file.getSimpleName() and not file.getFullName() which means that the loop over excludePatterns in AbstractCvs is not taking the full path into account. (However, I wouldn't know what negative side effects there would be, if any, to making the single change.)

        Attachments

          Activity

          Hide
          mc1arke Michael Clarke added a comment -

          Can you confirm what isn't working here? Are files being excluded that shouldn't or are files not being excluded that should be?

          The code uses CvsFile#getName() which returns the full file path from the module root (not just the file name) so should be matching the expected file names.

          Are you able to attach a job config (with any sensitive details) obscured?

          Show
          mc1arke Michael Clarke added a comment - Can you confirm what isn't working here? Are files being excluded that shouldn't or are files not being excluded that should be? The code uses CvsFile#getName() which returns the full file path from the module root (not just the file name) so should be matching the expected file names. Are you able to attach a job config (with any sensitive details) obscured?
          Hide
          jeremy Jeremy Michelson added a comment -

          Sorry I wasn't clear. The issue is that the build doesn't run if excluded regions are set.

          Suppose you've the hierarchy

          file.c
          dir1/
          dir1/file1.c
          dir2/
          dir2/file2.c
          ...
          

          With version 1.6 of the CVS plugin, one could set an excluded region of "[^/]*.c". Then changes to "file.c" would (correctly!) not trigger a build, but changes to "dir1/file1.c" would trigger a build. Now, neither triggers a build.

          I'm not sure how useful the job config would be. Because of this bug, we had to remove the excluded regions (at the cost of a large increase in spurious builds) in order to get the builds we needed, due to changes in subdirectories.

          Show
          jeremy Jeremy Michelson added a comment - Sorry I wasn't clear. The issue is that the build doesn't run if excluded regions are set. Suppose you've the hierarchy file.c dir1/ dir1/file1.c dir2/ dir2/file2.c ... With version 1.6 of the CVS plugin, one could set an excluded region of " [^/] *.c". Then changes to "file.c" would (correctly!) not trigger a build, but changes to "dir1/file1.c" would trigger a build. Now, neither triggers a build. I'm not sure how useful the job config would be. Because of this bug, we had to remove the excluded regions (at the cost of a large increase in spurious builds) in order to get the builds we needed, due to changes in subdirectories.
          Hide
          mc1arke Michael Clarke added a comment -

          I can't see why there would be an issue here - the upgraded CVS plugin uses Matcher#matches so looks for the full String to match. Could you try re-applying your exclude regions and see if you still get the same issue?

          Show
          mc1arke Michael Clarke added a comment - I can't see why there would be an issue here - the upgraded CVS plugin uses Matcher#matches so looks for the full String to match. Could you try re-applying your exclude regions and see if you still get the same issue?
          Hide
          jeremy Jeremy Michelson added a comment -

          I've attached a fairly heavily scrubbed config.xml. Notice the

                    <hudson.scm.ExcludedRegion>
                      <pattern>^[^/]*\.m$</pattern>
                    </hudson.scm.ExcludedRegion>
          

          With that config.xml, when I made a commit of

          • NotExcluded/file1.m
          • NotExcluded/file2.m

          no build happened, and the CVS polling log read

          ...
          cvs rlog: Logging ADDITIONAL/PATH/EXCLUDED1
          cvs rlog: Logging ADDITIONAL/PATH/EXCLUDED1/subdir
          ...
          cvs rlog: Logging ADDITIONAL/PATH/NotExcluded
          ...
          Done. Took 3.3 sec
          No changes
          

          I don't know enough about the plugin to know if the EXCLUDED paths should even be checked and included in the polling log. But that it is included seems consistent with my claim that only file basenames, and not full paths, are being checked. My claim is also consistent with the lack of a build, since "file1.m" matches "^[^/]*\.m$" even though "NotExcluded/file1.m" does not.

          Thanks.

          Show
          jeremy Jeremy Michelson added a comment - I've attached a fairly heavily scrubbed config.xml. Notice the <hudson.scm.ExcludedRegion> <pattern> ^[^/]*\.m$ </pattern> </hudson.scm.ExcludedRegion> With that config.xml, when I made a commit of NotExcluded/file1.m NotExcluded/file2.m no build happened, and the CVS polling log read ... cvs rlog: Logging ADDITIONAL/PATH/EXCLUDED1 cvs rlog: Logging ADDITIONAL/PATH/EXCLUDED1/subdir ... cvs rlog: Logging ADDITIONAL/PATH/NotExcluded ... Done. Took 3.3 sec No changes I don't know enough about the plugin to know if the EXCLUDED paths should even be checked and included in the polling log. But that it is included seems consistent with my claim that only file basenames, and not full paths, are being checked. My claim is also consistent with the lack of a build, since "file1.m" matches "^ [^/] *\.m$" even though "NotExcluded/file1.m" does not. Thanks.
          Hide
          scm_issue_link SCM/JIRA link daemon added a comment -

          Code changed in jenkins
          User: Michael Clarke
          Path:
          src/test/java/hudson/scm/CVSSCMTest.java
          http://jenkins-ci.org/commit/cvs-plugin/3aa040bb0f5486b98d51c595c8dc90b722e780aa
          Log:
          Add tests to prove JENKINS-15826 works


          You received this message because you are subscribed to the Google Groups "Jenkins Commits" group.
          To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-commits+unsubscribe@googlegroups.com.
          For more options, visit https://groups.google.com/groups/opt_out.

          Show
          scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Michael Clarke Path: src/test/java/hudson/scm/CVSSCMTest.java http://jenkins-ci.org/commit/cvs-plugin/3aa040bb0f5486b98d51c595c8dc90b722e780aa Log: Add tests to prove JENKINS-15826 works – You received this message because you are subscribed to the Google Groups "Jenkins Commits" group. To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-commits+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out .
          Hide
          mc1arke Michael Clarke added a comment -

          I've run some tests and can't replicate your issue. Could you try installing the snapshot version from https://buildhive.cloudbees.com/job/jenkinsci/job/cvs-plugin/88/org.jenkins-ci.plugins$cvs/artifact/org.jenkins-ci.plugins/cvs/2.9-SNAPSHOT/cvs-2.9-SNAPSHOT.hpi and check what the logs show?

          To answer your above comment: The plugin polls the full repository then checks each changed file to see if its path matches the exclude region. I've add in a log message to show which pattern any skipped files have matched to.

          Show
          mc1arke Michael Clarke added a comment - I've run some tests and can't replicate your issue. Could you try installing the snapshot version from https://buildhive.cloudbees.com/job/jenkinsci/job/cvs-plugin/88/org.jenkins-ci.plugins$cvs/artifact/org.jenkins-ci.plugins/cvs/2.9-SNAPSHOT/cvs-2.9-SNAPSHOT.hpi and check what the logs show? To answer your above comment: The plugin polls the full repository then checks each changed file to see if its path matches the exclude region. I've add in a log message to show which pattern any skipped files have matched to.
          Hide
          mc1arke Michael Clarke added a comment -

          I'm unable to replicate this either manually or through unit tests. Please re-open if you are still affected by this issue and have further information to help resolve it.

          Show
          mc1arke Michael Clarke added a comment - I'm unable to replicate this either manually or through unit tests. Please re-open if you are still affected by this issue and have further information to help resolve it.
          Hide
          jasonobrien Jason O'Brien added a comment -

          Michael, I believe your test case sidesteps the issue here by manually building the changed file list. CvsLog.java uses file.getSimpleName(), and so the CvsFile objects never get created with the full path - just the filename.

          So where you have done:
          new CvsFile("subdir/subdir2/test.ext", "1.1", false)
          CvsLog.java does:
          new CvsFile(file.getSimpleName(), file.getRevision(), file.isDead())

          Where file.getSimpleName just returns "text.ext" as specified in CVSChangeLogSet.java, where the doc is:

          Gets just the last component of the path, like "zot.c"

          I think using getFullName instead, as Jeremy suggested, might fix it, but I'm not sure if other code would be affected.

          Show
          jasonobrien Jason O'Brien added a comment - Michael, I believe your test case sidesteps the issue here by manually building the changed file list. CvsLog.java uses file.getSimpleName(), and so the CvsFile objects never get created with the full path - just the filename. So where you have done: new CvsFile("subdir/subdir2/test.ext", "1.1", false) CvsLog.java does: new CvsFile(file.getSimpleName(), file.getRevision(), file.isDead()) Where file.getSimpleName just returns "text.ext" as specified in CVSChangeLogSet.java, where the doc is: Gets just the last component of the path, like "zot.c" I think using getFullName instead, as Jeremy suggested, might fix it, but I'm not sure if other code would be affected.
          Hide
          jasonobrien Jason O'Brien added a comment - - edited
            • duplicate, my apologies
          Show
          jasonobrien Jason O'Brien added a comment - - edited duplicate, my apologies
          Hide
          scm_issue_link SCM/JIRA link daemon added a comment -

          Code changed in jenkins
          User: Michael Clarke
          Path:
          src/main/java/hudson/scm/CvsLog.java
          http://jenkins-ci.org/commit/cvs-plugin/9d1021271f95842c85dda35a6aec19ff768b3554
          Log:
          [FIXED JENKINS-15826] use full name for matching file exclude regions

          Compare: https://github.com/jenkinsci/cvs-plugin/compare/e1be53655e72...9d1021271f95

          Show
          scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Michael Clarke Path: src/main/java/hudson/scm/CvsLog.java http://jenkins-ci.org/commit/cvs-plugin/9d1021271f95842c85dda35a6aec19ff768b3554 Log: [FIXED JENKINS-15826] use full name for matching file exclude regions Compare: https://github.com/jenkinsci/cvs-plugin/compare/e1be53655e72...9d1021271f95

            People

            • Assignee:
              mc1arke Michael Clarke
              Reporter:
              jeremy Jeremy Michelson
            • Votes:
              2 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: