• Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Major Major
    • core
    • None
    • Platform: All, OS: All

      Although I have the Include culprits option checked for Unstable builds, the
      emails still only go to the global + committer, but not the culprits.

          [JENKINS-4617] "Include culprits" option not working

          mcrooney added a comment -

          To elaborate, I have an Unstable trigger sent to email Recipients, Committers,
          and Culprits. Person A is on the global list, and person B commits and breaks a
          test. The build is now unstable and an email goes to A+B. Now person C commits,
          and the test still fails because no one has fixed it yet. An email goes to A+C,
          instead of A+B+C.

          So it seems that include culprits isn't working, which is very useful to remind
          the person who broke the test that it is affecting other committers and they
          should fix it in a timely manner.

          mcrooney added a comment - To elaborate, I have an Unstable trigger sent to email Recipients, Committers, and Culprits. Person A is on the global list, and person B commits and breaks a test. The build is now unstable and an email goes to A+B. Now person C commits, and the test still fails because no one has fixed it yet. An email goes to A+C, instead of A+B+C. So it seems that include culprits isn't working, which is very useful to remind the person who broke the test that it is affecting other committers and they should fix it in a timely manner.

          Alan Harder added a comment -

          Can you update the latest email-ext (released today) and see if this is still a problem? Also turn on Hudson's built-in email notification with "Send separate e-mails to individuals who broke the build" checked, and see if it has the same problem (this also sends to build.getCulprits()).

          Alan Harder added a comment - Can you update the latest email-ext (released today) and see if this is still a problem? Also turn on Hudson's built-in email notification with "Send separate e-mails to individuals who broke the build" checked, and see if it has the same problem (this also sends to build.getCulprits()).

          Alan Harder added a comment -

          Reminder to retest on current releases, thanks..

          Alan Harder added a comment - Reminder to retest on current releases, thanks..

          mcrooney added a comment -

          Still an issue with Hudson 1.341 and email-ext 2.4, indeed.

          mcrooney added a comment - Still an issue with Hudson 1.341 and email-ext 2.4, indeed.

          Alan Harder added a comment -

          what SCM are you using? I wasn't able to reproduce this problem with a subversion project. In a project where email-ext isn't emailing the right people, access /job/JOBNAME/BUILDNUMBER/api/xml and look at the culprit info listed here.. is this list accurate? (email-ext should be using that list)

          Alan Harder added a comment - what SCM are you using? I wasn't able to reproduce this problem with a subversion project. In a project where email-ext isn't emailing the right people, access /job/JOBNAME/BUILDNUMBER/api/xml and look at the culprit info listed here.. is this list accurate? (email-ext should be using that list)

          mcrooney added a comment -

          We are using SVN. Were you trying with sequential Unstable builds with different committers, not Failed builds? If I look at the api/xml it is indeed wrong there. The first unstable build it has the two committers as culprits (correct), but the next unstable build doesn't have those people but instead the two people that made commits in that revision set. It doesn't seem to be carrying over culprits.

          mcrooney added a comment - We are using SVN. Were you trying with sequential Unstable builds with different committers, not Failed builds? If I look at the api/xml it is indeed wrong there. The first unstable build it has the two committers as culprits (correct), but the next unstable build doesn't have those people but instead the two people that made commits in that revision set. It doesn't seem to be carrying over culprits.

          Alan Harder added a comment -

          What is your Hudson version and subversion plugin version?

          Moving out of email-ext component, as core determines the culprit list and email-ext just uses that info from the build. If the remote API also has incorrect data, the problem is not in email-ext.

          I did: failed build with personA, failed build with personB, unstable build
          I'll try now with only unstables..

          Alan Harder added a comment - What is your Hudson version and subversion plugin version? Moving out of email-ext component, as core determines the culprit list and email-ext just uses that info from the build. If the remote API also has incorrect data, the problem is not in email-ext. I did: failed build with personA, failed build with personB, unstable build I'll try now with only unstables..

          mcrooney added a comment -

          Hudson 1.341 with Subversion 1.8. I'll upgrade to 1.9 and test but I don't see anything relevant in the changes.

          mcrooney added a comment - Hudson 1.341 with Subversion 1.8. I'll upgrade to 1.9 and test but I don't see anything relevant in the changes.

          Alan Harder added a comment -

          Yup, sequence of unstables shows the problem. Odd that unstable following a failed build does pull cuprits from that previous failed build, but doesn't pull from previous unstable.

          Alan Harder added a comment - Yup, sequence of unstables shows the problem. Odd that unstable following a failed build does pull cuprits from that previous failed build, but doesn't pull from previous unstable.

          Alan Harder added a comment -

          Looks like "culprits" was coded to only be those causing failed builds (not unstable).. it has been that way since r4689 (Sep 2007). See the request in JENKINS-747.. no mention of unstable builds there.

          So, should the culprits calculated by Hudson core be since last stable build (to catch committers for unstable builds) or since last "successful" build (which may be unstable.. so only tracking failed builds, or aborted I guess).

          Alan Harder added a comment - Looks like "culprits" was coded to only be those causing failed builds (not unstable).. it has been that way since r4689 (Sep 2007). See the request in JENKINS-747 .. no mention of unstable builds there. So, should the culprits calculated by Hudson core be since last stable build (to catch committers for unstable builds) or since last "successful" build (which may be unstable.. so only tracking failed builds, or aborted I guess).

          mcrooney added a comment -

          I guess it is obvious since I filed this bug but intuitively to me someone who broke tests is a "culprit".

          mcrooney added a comment - I guess it is obvious since I filed this bug but intuitively to me someone who broke tests is a "culprit".

          I suppose it's a reasonable change to include those who broke the tests.

          The only thing I wonder is if we can do something clever in situations where you have some tests regularly failing, then people who committed a change that didn't increase the failure count wouldn't be counted toward culprits. But I suppose it's not possible since it collapses multiple abstraction layers.

          Kohsuke Kawaguchi added a comment - I suppose it's a reasonable change to include those who broke the tests. The only thing I wonder is if we can do something clever in situations where you have some tests regularly failing, then people who committed a change that didn't increase the failure count wouldn't be counted toward culprits. But I suppose it's not possible since it collapses multiple abstraction layers.

          kohsuke commented:
          >> The only thing I wonder is if we can do something clever in situations where you have some tests regularly failing, then people who committed a change that didn't increase the failure count wouldn't be counted toward culprits

          JENKINS-5282 requests this very change.

          Matthew Webber added a comment - kohsuke commented: >> The only thing I wonder is if we can do something clever in situations where you have some tests regularly failing, then people who committed a change that didn't increase the failure count wouldn't be counted toward culprits JENKINS-5282 requests this very change.

          mcrooney added a comment -

          Just as an update on this issue, I attempted to fix this locally and will commit if it works, though I'm not convinced as of yet.

          Index: core/src/main/java/hudson/model/AbstractBuild.java
          ===================================================================
          --- core/src/main/java/hudson/model/AbstractBuild.java	(revision 31234)
          +++ core/src/main/java/hudson/model/AbstractBuild.java	(working copy)
          @@ -263,7 +263,7 @@
                       R p = getPreviousCompletedBuild();
                       if (p !=null && isBuilding()) {
                           Result pr = p.getResult();
          -                if (pr!=null && pr.isWorseThan(Result.UNSTABLE)) {
          +                if (pr!=null && pr.isWorseOrEqualTo(Result.UNSTABLE)) {
                               // we are still building, so this is just the current latest information,
                               // but we seems to be failing so far, so inherit culprits from the previous build.
                               // isBuilding() check is to avoid recursion when loading data from old Hudson, which doesn't record
          

          mcrooney added a comment - Just as an update on this issue, I attempted to fix this locally and will commit if it works, though I'm not convinced as of yet. Index: core/src/main/java/hudson/model/AbstractBuild.java =================================================================== --- core/src/main/java/hudson/model/AbstractBuild.java (revision 31234) +++ core/src/main/java/hudson/model/AbstractBuild.java (working copy) @@ -263,7 +263,7 @@ R p = getPreviousCompletedBuild(); if (p != null && isBuilding()) { Result pr = p.getResult(); - if (pr!= null && pr.isWorseThan(Result.UNSTABLE)) { + if (pr!= null && pr.isWorseOrEqualTo(Result.UNSTABLE)) { // we are still building, so this is just the current latest information, // but we seems to be failing so far, so inherit culprits from the previous build. // isBuilding() check is to avoid recursion when loading data from old Hudson, which doesn't record

          Alan Harder added a comment -

          looks ok to me.

          Alan Harder added a comment - looks ok to me.

          mcrooney added a comment -

          Alas, it doesn't seem to be working but I'm not positive. This area was really confusing, especially how the block I changed is only executed if the previous build is building it seems. If anyone with some more insight wants to give that function (getCulprits) a peek to see, it would be appreciated.

          mcrooney added a comment - Alas, it doesn't seem to be working but I'm not positive. This area was really confusing, especially how the block I changed is only executed if the previous build is building it seems. If anyone with some more insight wants to give that function (getCulprits) a peek to see, it would be appreciated.

          Alan Harder added a comment -

          yes, that code is odd.. looks like it relies on the fact that getCulprits is called further down in AbstractBuild.java from post().. isBuilding() will be true in this call, right?

          Alan Harder added a comment - yes, that code is odd.. looks like it relies on the fact that getCulprits is called further down in AbstractBuild.java from post().. isBuilding() will be true in this call, right?

          m211 added a comment -

          In the getCulprits()-method the line
          Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousNotFailedBuild());
          may be wrong.
          I thing the
          getPreviousNotFailedBuild()
          must be replaced with
          getPreviousSuccessfulBuild()
          The getPreviousNotFailedBuild() provides the last unstable build. This can be the previous build. And then getDependencyChanges(..) provides only the dependency to the previous build and not to the last successful build. And the culprits from all previous unstable builds until to the last successful build will not be found.

          m211 added a comment - In the getCulprits()-method the line Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousNotFailedBuild()); may be wrong. I thing the getPreviousNotFailedBuild() must be replaced with getPreviousSuccessfulBuild() The getPreviousNotFailedBuild() provides the last unstable build. This can be the previous build. And then getDependencyChanges(..) provides only the dependency to the previous build and not to the last successful build. And the culprits from all previous unstable builds until to the last successful build will not be found.

          m211 added a comment -

          for me it works.
          here is the patch.

          diff --git a/core/src/main/java/hudson/model/AbstractBuild.java b/core/src/main/java/hudson/model/AbstractBuild.java
          index 1b776f1..b1f4144 100644
          --- a/core/src/main/java/hudson/model/AbstractBuild.java
          +++ b/core/src/main/java/hudson/model/AbstractBuild.java
          @@ -278,7 +278,7 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
                       if (upstreamCulprits) {
                           // If we have dependencies since the last successful build, add their authors to our list
                           if (getPreviousNotFailedBuild() != null) {
          -                    Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousNotFailedBuild());
          +                    Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousSuccessfulBuild());
                               for (AbstractBuild.DependencyChange dep : depmap.values()) {
                                   for (AbstractBuild<?,?> b : dep.getBuilds()) {
                                       for (Entry entry : b.getChangeSet()) {
          

          m211 added a comment - for me it works. here is the patch. diff --git a/core/src/main/java/hudson/model/AbstractBuild.java b/core/src/main/java/hudson/model/AbstractBuild.java index 1b776f1..b1f4144 100644 --- a/core/src/main/java/hudson/model/AbstractBuild.java +++ b/core/src/main/java/hudson/model/AbstractBuild.java @@ -278,7 +278,7 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs if (upstreamCulprits) { // If we have dependencies since the last successful build, add their authors to our list if (getPreviousNotFailedBuild() != null) { - Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousNotFailedBuild()); + Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousSuccessfulBuild()); for (AbstractBuild.DependencyChange dep : depmap.values()) { for (AbstractBuild<?,?> b : dep.getBuilds()) { for (Entry entry : b.getChangeSet()) {

          m211 added a comment -

          has anyone tried my patch?
          can the patch commited to hudson-core?

          m211 added a comment - has anyone tried my patch? can the patch commited to hudson-core?

          I notice you didn't get any response to the question "has anyone tried my patch?", so I guess you should release it and folks like me can give some feedback. I'm not in a position to test patches.

          Matthew Webber added a comment - I notice you didn't get any response to the question "has anyone tried my patch?", so I guess you should release it and folks like me can give some feedback. I'm not in a position to test patches.

          Alan Harder added a comment -

          m211, that change is only in the "if (upstreamCulprits)" block.. have you tested the simple case where there is no upstream job?

          Alan Harder added a comment - m211, that change is only in the "if (upstreamCulprits)" block.. have you tested the simple case where there is no upstream job?

          m211 added a comment -

          yes it works also for the simple case without any upstream job.

          m211 added a comment - yes it works also for the simple case without any upstream job.

          Code changed in jenkins
          User: marco
          Path:
          core/src/main/java/hudson/model/AbstractBuild.java
          http://jenkins-ci.org/commit/jenkins/502219cf1b8be65f91d4ebd81e590558eb8c5b26
          Log:
          JENKINS-4617
          we are looking for dependencies since the last successful build
          and not since the last previous not failed build

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: marco Path: core/src/main/java/hudson/model/AbstractBuild.java http://jenkins-ci.org/commit/jenkins/502219cf1b8be65f91d4ebd81e590558eb8c5b26 Log: JENKINS-4617 we are looking for dependencies since the last successful build and not since the last previous not failed build

          Code changed in jenkins
          User: Kohsuke Kawaguchi
          Path:
          changelog.html
          core/src/main/java/hudson/model/AbstractBuild.java
          test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java
          test/src/test/groovy/hudson/model/AbstractBuildTest.groovy
          http://jenkins-ci.org/commit/jenkins/e761d80f4d53a0f368fa8e9fbb06f01db247f8cc
          Log:
          [FIXED JENKINS-4617]
          this and the previous commit constitutes the fix.
          Also added a test case.

          Compare: https://github.com/jenkinsci/jenkins/compare/ad646cd...e761d80

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Kohsuke Kawaguchi Path: changelog.html core/src/main/java/hudson/model/AbstractBuild.java test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java test/src/test/groovy/hudson/model/AbstractBuildTest.groovy http://jenkins-ci.org/commit/jenkins/e761d80f4d53a0f368fa8e9fbb06f01db247f8cc Log: [FIXED JENKINS-4617] this and the previous commit constitutes the fix. Also added a test case. Compare: https://github.com/jenkinsci/jenkins/compare/ad646cd...e761d80

          dogfood added a comment -

          Integrated in jenkins_main_trunk #683
          JENKINS-4617
          [FIXED JENKINS-4617]

          Kohsuke Kawaguchi : 502219cf1b8be65f91d4ebd81e590558eb8c5b26
          Files :

          • core/src/main/java/hudson/model/AbstractBuild.java

          Kohsuke Kawaguchi : e761d80f4d53a0f368fa8e9fbb06f01db247f8cc
          Files :

          • test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java
          • test/src/test/groovy/hudson/model/AbstractBuildTest.groovy
          • core/src/main/java/hudson/model/AbstractBuild.java
          • changelog.html

          dogfood added a comment - Integrated in jenkins_main_trunk #683 JENKINS-4617 [FIXED JENKINS-4617] Kohsuke Kawaguchi : 502219cf1b8be65f91d4ebd81e590558eb8c5b26 Files : core/src/main/java/hudson/model/AbstractBuild.java Kohsuke Kawaguchi : e761d80f4d53a0f368fa8e9fbb06f01db247f8cc Files : test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java test/src/test/groovy/hudson/model/AbstractBuildTest.groovy core/src/main/java/hudson/model/AbstractBuild.java changelog.html

          Code changed in jenkins
          User: marco
          Path:
          core/src/main/java/hudson/model/AbstractBuild.java
          http://jenkins-ci.org/commit/jenkins/502219cf1b8be65f91d4ebd81e590558eb8c5b26
          Log:
          JENKINS-4617
          we are looking for dependencies since the last successful build
          and not since the last previous not failed build

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: marco Path: core/src/main/java/hudson/model/AbstractBuild.java http://jenkins-ci.org/commit/jenkins/502219cf1b8be65f91d4ebd81e590558eb8c5b26 Log: JENKINS-4617 we are looking for dependencies since the last successful build and not since the last previous not failed build

          Code changed in jenkins
          User: Kohsuke Kawaguchi
          Path:
          changelog.html
          core/src/main/java/hudson/model/AbstractBuild.java
          test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java
          test/src/test/groovy/hudson/model/AbstractBuildTest.groovy
          http://jenkins-ci.org/commit/jenkins/e761d80f4d53a0f368fa8e9fbb06f01db247f8cc
          Log:
          [FIXED JENKINS-4617]
          this and the previous commit constitutes the fix.
          Also added a test case.

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Kohsuke Kawaguchi Path: changelog.html core/src/main/java/hudson/model/AbstractBuild.java test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java test/src/test/groovy/hudson/model/AbstractBuildTest.groovy http://jenkins-ci.org/commit/jenkins/e761d80f4d53a0f368fa8e9fbb06f01db247f8cc Log: [FIXED JENKINS-4617] this and the previous commit constitutes the fix. Also added a test case.

          Code changed in jenkins
          User: marco
          Path:
          core/src/main/java/hudson/model/AbstractBuild.java
          http://jenkins-ci.org/commit/jenkins/502219cf1b8be65f91d4ebd81e590558eb8c5b26
          Log:
          JENKINS-4617
          we are looking for dependencies since the last successful build
          and not since the last previous not failed build

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: marco Path: core/src/main/java/hudson/model/AbstractBuild.java http://jenkins-ci.org/commit/jenkins/502219cf1b8be65f91d4ebd81e590558eb8c5b26 Log: JENKINS-4617 we are looking for dependencies since the last successful build and not since the last previous not failed build

          Code changed in jenkins
          User: Kohsuke Kawaguchi
          Path:
          changelog.html
          core/src/main/java/hudson/model/AbstractBuild.java
          test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java
          test/src/test/groovy/hudson/model/AbstractBuildTest.groovy
          http://jenkins-ci.org/commit/jenkins/e761d80f4d53a0f368fa8e9fbb06f01db247f8cc
          Log:
          [FIXED JENKINS-4617]
          this and the previous commit constitutes the fix.
          Also added a test case.

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Kohsuke Kawaguchi Path: changelog.html core/src/main/java/hudson/model/AbstractBuild.java test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java test/src/test/groovy/hudson/model/AbstractBuildTest.groovy http://jenkins-ci.org/commit/jenkins/e761d80f4d53a0f368fa8e9fbb06f01db247f8cc Log: [FIXED JENKINS-4617] this and the previous commit constitutes the fix. Also added a test case.

          Code changed in jenkins
          User: Kohsuke Kawaguchi
          Path:
          test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java
          http://jenkins-ci.org/commit/jenkins-test-harness/10e915577cde1696703840c9c57014db63b7755f
          Log:
          [FIXED JENKINS-4617]
          this and the previous commit constitutes the fix.
          Also added a test case.

          Originally-Committed-As: e761d80f4d53a0f368fa8e9fbb06f01db247f8cc

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Kohsuke Kawaguchi Path: test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java http://jenkins-ci.org/commit/jenkins-test-harness/10e915577cde1696703840c9c57014db63b7755f Log: [FIXED JENKINS-4617] this and the previous commit constitutes the fix. Also added a test case. Originally-Committed-As: e761d80f4d53a0f368fa8e9fbb06f01db247f8cc

            markltbaker markltbaker
            mcrooney mcrooney
            Votes:
            6 Vote for this issue
            Watchers:
            7 Start watching this issue

              Created:
              Updated:
              Resolved: