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

Reference build is wrong for PRs

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Major Major
    • git-forensics-plugin
    • None

      The discoverGitReferenceBuild step appears to discover the build in the reference job that includes the last common commit to the branch. This doesn't make sense for PR builds though, since they are merged with the head of the target branch, which means the more appropriate build for comparing metrics against is usually going to be the most recent build of the reference job.

      Reproduce Steps:

      • Create a new git repo
      • Add s single commit, with a Jenkinsfile that checks out the code and runs the discoverGitReferenceBuild step
      • Create a multibranch pipeline job in Jenkins for the new repository and build the main branch
      • Create a new branch. Add one commit (maybe updating the Jenkinsfile to specify main as the defaultBranch - not sure if that's supposed to be automatic, but I've always had to set it to env.CHANGE_TARGET in my tests)
      • Check out the main branch and commit another change.
      • Create a PR from the branch to main
      • Rescan the repository
      • Build main again (so now there should be two builds of main, and a PR with no builds)
      • Build the PR

      The logs will indicate that the PR was merged against the latest commit of main, but the reference build was build #1 of main.

       

      I'm calling this one "major" only because I'm pretty sure the publishCoverage step in an older Jenkins used to handle this before it used git-forensics, and I haven't figured out a workaround yet. (although admittedly I didn't actually test this specific combination when I was experimenting coverage diffs in multibranch pipelines in Jenkins a year ago - I only thought to test this after I read the code while tracking down the other bug I filed)

          [JENKINS-66480] Reference build is wrong for PRs

          Christopher created issue -

          Kalle Niemitalo added a comment - - edited

          AFAIK, the target branch of a pull request can be read from ChangeRequestSCMHead.getTarget(), but neither git-forensics-plugin nor forensics-api-plugin calls that. In contrast, code-coverage-api-plugin used to call that until PR #199 made it use forensics-api-plugin instead.

          So the fix would be to make ReferenceRecorder in forensics-api-plugin call ChangeRequestSCMHead.getTarget() if available, and use the result as the reference branch, instead of the default branch (which can be set with ReferenceRecorder.setDefaultBranch or defaults to "master"). SimpleReferenceRecorder.setReferenceJob would still bypass the multibranch support altogether, and use exactly the job that you specify.

          Until that is implemented, I think one can work around the issue by making the pipeline run discoverGitReferenceBuild defaultBranch: env.CHANGE_TARGET if CHANGE_TARGET is set.

          cetlan, what type of branch source are you using in the multibranch project; GitHub or something else?

          Kalle Niemitalo added a comment - - edited AFAIK, the target branch of a pull request can be read from ChangeRequestSCMHead.getTarget() , but neither git-forensics-plugin nor forensics-api-plugin calls that. In contrast, code-coverage-api-plugin used to call that until PR #199 made it use forensics-api-plugin instead. So the fix would be to make ReferenceRecorder in forensics-api-plugin call ChangeRequestSCMHead.getTarget() if available, and use the result as the reference branch, instead of the default branch (which can be set with ReferenceRecorder.setDefaultBranch or defaults to "master"). SimpleReferenceRecorder.setReferenceJob would still bypass the multibranch support altogether, and use exactly the job that you specify. Until that is implemented, I think one can work around the issue by making the pipeline run discoverGitReferenceBuild defaultBranch: env.CHANGE_TARGET if CHANGE_TARGET is set. cetlan , what type of branch source are you using in the multibranch project; GitHub or something else?

          Kalle Niemitalo added a comment -

          Oh, I see I completely misunderstood the issue. You are already setting defaultBranch.

          Kalle Niemitalo added a comment - Oh, I see I completely misunderstood the issue. You are already setting defaultBranch.

          Kalle Niemitalo added a comment - - edited

          I should file a separate feature request for making ReferenceRecorder use the target branch of a pull request as the reference branch.

          Does your multibranch project build pull requests by "Merging the pull request with the current target branch revision" (ChangeRequestCheckoutStrategy.MERGE)? In that case, I agree the reference build should be a build of the current target branch revision, rather than a build of the last common commit.

          For "The current pull request revision" though, the last common commit is better.

          These can be distinguished by calling ChangeRequestSCMRevision.isMerge. However, GitCheckoutListener already calls that.

          Does your multibranch pipeline use any pipeline libraries from SCM? I'm not sure but perhaps those could add another SCMRevisionAction to the Run and cause the wrong SCMRevision to be checked.

          Kalle Niemitalo added a comment - - edited I should file a separate feature request for making ReferenceRecorder use the target branch of a pull request as the reference branch. Does your multibranch project build pull requests by "Merging the pull request with the current target branch revision" ( ChangeRequestCheckoutStrategy.MERGE )? In that case, I agree the reference build should be a build of the current target branch revision, rather than a build of the last common commit. For "The current pull request revision" though, the last common commit is better. These can be distinguished by calling ChangeRequestSCMRevision.isMerge . However, GitCheckoutListener already calls that. Does your multibranch pipeline use any pipeline libraries from SCM? I'm not sure but perhaps those could add another SCMRevisionAction to the Run and cause the wrong SCMRevision to be checked.

          Kalle Niemitalo added a comment - - edited

          I think the fix would be to use SCMRevisionAction.getRevision(SCMSource, Actionable) in GitCheckoutListener.isMerge:

          SCMSource source = SCMSource.SourceByItem.findSource(build.getParent());
if (source != null) {
    SCMRevision revision = SCMRevisionAction.getRevision(source, build);
    // then the instanceof check
}

          And remove the build.getAction(SCMRevisionAction.class) call because I assume you cannot have merge revisions without an SCM source.

          That would prevent pipeline libraries from fooling the isMerge check, but I'm not sure they caused the problem. cetlan, does your build log include any messages from the if (isMergeCommit) branch in GitCommitsCollector.findHeadCommit? If so, that would mean GitCheckoutListener.isMerge correctly returned true and the problem is somewhere else.

          Kalle Niemitalo added a comment - - edited I think the fix would be to use SCMRevisionAction.getRevision(SCMSource, Actionable) in GitCheckoutListener.isMerge: SCMSource source = SCMSource.SourceByItem.findSource(build.getParent()); if (source != null ) { SCMRevision revision = SCMRevisionAction.getRevision(source, build); // then the instanceof check } And remove the build.getAction(SCMRevisionAction.class) call because I assume you cannot have merge revisions without an SCM source. That would prevent pipeline libraries from fooling the isMerge check, but I'm not sure they caused the problem. cetlan , does your build log include any messages from the if (isMergeCommit) branch in GitCommitsCollector.findHeadCommit ? If so, that would mean GitCheckoutListener.isMerge correctly returned true and the problem is somewhere else.

          Christopher added a comment - - edited

          kon I don't think any other plugins are causing the problem - or at least if they are they must be part of the standard config, since I was testing this on a clean Jenkins. I don't quite know how to answer some of your questions, since I'm still just learning my way around the pipelines and multibranch projects, but if it helps, I've got the project I used for testing this on github at https://github.com/cetlan/git-forensics-pr-test, (although you can't really drop that into a Jenkins for testing, since the test requires the first build to be run before the last commit was pushed, but at least you can see the pipeline). The Job configuration is just "Multibranch pipeline", using a github repository, with otherwise default settings (except I turned off auto-building to make sure I build the root and PRs in the "right" order to keep the commit recorder/scan happy: first commit of main, then second commit of main, then PR based off first commit of main).

          If it helps, here's the build log (if anyone looked at this comment earlier I looked at the wrong log - it looks like the messages are in there):

          Started by user admin
01:00:00 Connecting to https://api.github.com with no credentials, anonymous access
01:00:00 Jenkins-Imposed API Limiter: Current quota for Github API usage has 41 remaining (2 over budget). Next quota of 60 in 43 min. Sleeping for 6 min 4 sec.
01:00:00 Jenkins is attempting to evenly distribute GitHub API requests. To configure a different rate limiting strategy, such as having Jenkins restrict GitHub API requests only when near or above the GitHub rate limit, go to "GitHub API usage" under "Configure System" in the Jenkins settings.
01:03:01 Jenkins-Imposed API Limiter: Still sleeping, now only 3 min 2 sec remaining.
01:06:02 Jenkins-Imposed API Limiter: Still sleeping, now only 1.2 sec remaining.
Obtained Jenkinsfile from 372ed74bf3c5263a95a238cf522f88f49ee40456+a5f87cb236d06095dd9304ddc09f62158d1d7417 (c8bc8c6fa1353e90820065d22a07301cef78191c)
Running in Durability level: MAX_SURVIVABILITY
[Pipeline] Start of Pipeline
[Pipeline] node
Running on Jenkins in /var/jenkins_home/workspace/git-forensics-pr-test_PR-1
[Pipeline] {
[Pipeline] stage
[Pipeline] { (Detect reference branch)
[Pipeline] checkout
The recommended git tool is: NONE
No credentials specified
Cloning the remote Git repository
Cloning with configured refspecs honoured and without tags
Cloning repository https://github.com/cetlan/git-forensics-pr-test.git
 > git init /var/jenkins_home/workspace/git-forensics-pr-test_PR-1 # timeout=10
Fetching upstream changes from https://github.com/cetlan/git-forensics-pr-test.git
 > git --version # timeout=10
 > git --version # 'git version 2.30.2'
 > git fetch --no-tags --force --progress -- https://github.com/cetlan/git-forensics-pr-test.git +refs/pull/1/head:refs/remotes/origin/PR-1 +refs/heads/main:refs/remotes/origin/main # timeout=10
 > git config remote.origin.url https://github.com/cetlan/git-forensics-pr-test.git # timeout=10
 > git config --add remote.origin.fetch +refs/pull/1/head:refs/remotes/origin/PR-1 # timeout=10
 > git config --add remote.origin.fetch +refs/heads/main:refs/remotes/origin/main # timeout=10
 > git config remote.origin.url https://github.com/cetlan/git-forensics-pr-test.git # timeout=10
Fetching without tags
Fetching upstream changes from https://github.com/cetlan/git-forensics-pr-test.git
 > git fetch --no-tags --force --progress -- https://github.com/cetlan/git-forensics-pr-test.git +refs/pull/1/head:refs/remotes/origin/PR-1 +refs/heads/main:refs/remotes/origin/main # timeout=10
Merging remotes/origin/main commit a5f87cb236d06095dd9304ddc09f62158d1d7417 into PR head commit 372ed74bf3c5263a95a238cf522f88f49ee40456
 > git config core.sparsecheckout # timeout=10
 > git checkout -f 372ed74bf3c5263a95a238cf522f88f49ee40456 # timeout=10
 > git remote # timeout=10
 > git config --get remote.origin.url # timeout=10
 > git merge a5f87cb236d06095dd9304ddc09f62158d1d7417 # timeout=10
 > git rev-parse HEAD^{commit} # timeout=10
Merge succeeded, producing 3cefdb535e9796a46b13705581d2cf7e34c7f258
Checking out Revision 3cefdb535e9796a46b13705581d2cf7e34c7f258 (PR-1)
 > git config core.sparsecheckout # timeout=10
 > git checkout -f 3cefdb535e9796a46b13705581d2cf7e34c7f258 # timeout=10
Commit message: "Merge commit 'a5f87cb236d06095dd9304ddc09f62158d1d7417' into HEAD"
First time build. Skipping changelog.
The recommended git tool is: NONE
No credentials specified
 > git rev-parse HEAD^{commit} # timeout=10
The recommended git tool is: NONE
No credentials specified
[GitCheckoutListener] Recording commits of 'git https://github.com/cetlan/git-forensics-pr-test.git'
[GitCheckoutListener] Found no previous build with recorded Git commits
[GitCheckoutListener] -> Starting initial recording of commits
[GitCheckoutListener] -> Multiple parent commits found - skipping commits of local merge '3cefdb5'
[GitCheckoutListener] -> Using parent commit '372ed74' of local merge as starting point
[GitCheckoutListener] -> Storing target branch head 'a5f87cb' (second parent of local merge) 
[GitCheckoutListener] -> Git commit decorator successfully obtained 'hudson.plugins.git.browser.GithubWeb@3d4c6f91' to render commit links
[GitCheckoutListener] -> Recorded 2 new commits
[Pipeline] discoverGitReferenceBuild
[ReferenceFinder] Reference job inferred from toplevel project 'git-forensics-pr-test'
[ReferenceFinder] Target branch: 'main'
[ReferenceFinder] Inferred job: 'main'
[ReferenceFinder] Found reference build '#1' for target branch
[Pipeline] }
[Pipeline] // stage
[Pipeline] }
[Pipeline] // node
[Pipeline] End of Pipeline
Finished: SUCCESS

          Christopher added a comment - - edited kon I don't think any other plugins are causing the problem - or at least if they are they must be part of the standard config, since I was testing this on a clean Jenkins. I don't quite know how to answer some of your questions, since I'm still just learning my way around the pipelines and multibranch projects, but if it helps, I've got the project I used for testing this on github at https://github.com/cetlan/git-forensics-pr-test,  (although you can't really drop that into a Jenkins for testing, since the test requires the first build to be run before the last commit was pushed, but at least you can see the pipeline). The Job configuration is just "Multibranch pipeline", using a github repository, with otherwise default settings (except I turned off auto-building to make sure I build the root and PRs in the "right" order to keep the commit recorder/scan happy: first commit of main, then second commit of main, then PR based off first commit of main). If it helps, here's the build log (if anyone looked at this comment earlier I looked at the wrong log - it looks like the messages are in there): Started by user admin 01:00:00 Connecting to https://api.github.com with no credentials, anonymous access 01:00:00 Jenkins-Imposed API Limiter: Current quota for Github API usage has 41 remaining (2 over budget). Next quota of 60 in 43 min. Sleeping for 6 min 4 sec. 01:00:00 Jenkins is attempting to evenly distribute GitHub API requests. To configure a different rate limiting strategy, such as having Jenkins restrict GitHub API requests only when near or above the GitHub rate limit, go to "GitHub API usage" under "Configure System" in the Jenkins settings. 01:03:01 Jenkins-Imposed API Limiter: Still sleeping, now only 3 min 2 sec remaining. 01:06:02 Jenkins-Imposed API Limiter: Still sleeping, now only 1.2 sec remaining. Obtained Jenkinsfile from 372ed74bf3c5263a95a238cf522f88f49ee40456+a5f87cb236d06095dd9304ddc09f62158d1d7417 (c8bc8c6fa1353e90820065d22a07301cef78191c) Running in Durability level: MAX_SURVIVABILITY [Pipeline] Start of Pipeline [Pipeline] node Running on Jenkins in /var/jenkins_home/workspace/git-forensics-pr-test_PR-1 [Pipeline] { [Pipeline] stage [Pipeline] { (Detect reference branch) [Pipeline] checkout The recommended git tool is: NONE No credentials specified Cloning the remote Git repository Cloning with configured refspecs honoured and without tags Cloning repository https://github.com/cetlan/git-forensics-pr-test.git > git init /var/jenkins_home/workspace/git-forensics-pr-test_PR-1 # timeout=10 Fetching upstream changes from https://github.com/cetlan/git-forensics-pr-test.git > git --version # timeout=10 > git --version # 'git version 2.30.2' > git fetch --no-tags --force --progress -- https://github.com/cetlan/git-forensics-pr-test.git +refs/pull/1/head:refs/remotes/origin/PR-1 +refs/heads/main:refs/remotes/origin/main # timeout=10 > git config remote.origin.url https://github.com/cetlan/git-forensics-pr-test.git # timeout=10 > git config --add remote.origin.fetch +refs/pull/1/head:refs/remotes/origin/PR-1 # timeout=10 > git config --add remote.origin.fetch +refs/heads/main:refs/remotes/origin/main # timeout=10 > git config remote.origin.url https://github.com/cetlan/git-forensics-pr-test.git # timeout=10 Fetching without tags Fetching upstream changes from https://github.com/cetlan/git-forensics-pr-test.git > git fetch --no-tags --force --progress -- https://github.com/cetlan/git-forensics-pr-test.git +refs/pull/1/head:refs/remotes/origin/PR-1 +refs/heads/main:refs/remotes/origin/main # timeout=10 Merging remotes/origin/main commit a5f87cb236d06095dd9304ddc09f62158d1d7417 into PR head commit 372ed74bf3c5263a95a238cf522f88f49ee40456 > git config core.sparsecheckout # timeout=10 > git checkout -f 372ed74bf3c5263a95a238cf522f88f49ee40456 # timeout=10 > git remote # timeout=10 > git config --get remote.origin.url # timeout=10 > git merge a5f87cb236d06095dd9304ddc09f62158d1d7417 # timeout=10 > git rev-parse HEAD^{commit} # timeout=10 Merge succeeded, producing 3cefdb535e9796a46b13705581d2cf7e34c7f258 Checking out Revision 3cefdb535e9796a46b13705581d2cf7e34c7f258 (PR-1) > git config core.sparsecheckout # timeout=10 > git checkout -f 3cefdb535e9796a46b13705581d2cf7e34c7f258 # timeout=10 Commit message: "Merge commit 'a5f87cb236d06095dd9304ddc09f62158d1d7417' into HEAD" First time build. Skipping changelog. The recommended git tool is: NONE No credentials specified > git rev-parse HEAD^{commit} # timeout=10 The recommended git tool is: NONE No credentials specified [GitCheckoutListener] Recording commits of 'git https://github.com/cetlan/git-forensics-pr-test.git' [GitCheckoutListener] Found no previous build with recorded Git commits [GitCheckoutListener] -> Starting initial recording of commits [GitCheckoutListener] -> Multiple parent commits found - skipping commits of local merge '3cefdb5' [GitCheckoutListener] -> Using parent commit '372ed74' of local merge as starting point [GitCheckoutListener] -> Storing target branch head 'a5f87cb' (second parent of local merge) [GitCheckoutListener] -> Git commit decorator successfully obtained 'hudson.plugins.git.browser.GithubWeb@3d4c6f91' to render commit links [GitCheckoutListener] -> Recorded 2 new commits [Pipeline] discoverGitReferenceBuild [ReferenceFinder] Reference job inferred from toplevel project 'git-forensics-pr-test' [ReferenceFinder] Target branch: 'main' [ReferenceFinder] Inferred job: 'main' [ReferenceFinder] Found reference build '#1' for target branch [Pipeline] } [Pipeline] // stage [Pipeline] } [Pipeline] // node [Pipeline] End of Pipeline Finished: SUCCESS

          Kalle Niemitalo added a comment -

          Thank you for posting the log and publishing the repository.

          Obtained Jenkinsfile from 372ed74bf3c5263a95a238cf522f88f49ee40456+a5f87cb236d06095dd9304ddc09f62158d1d7417 (c8bc8c6fa1353e90820065d22a07301cef78191c)

          • c8bc8c6 is refs/pull/1/merge. Its parents are a5f87cb and 372ed74, in that order. Jenkins gets the Jenkinsfile from it but does not appear to use it for other purposes.
          • 372ed74 is refs/heads/initial-branch and refs/pull/1/head. Its parent is 4e002f7.
          • a5f87cb is refs/heads/main. Its parent is 4e002f7.
          • 4e002f7 is the initial commit that has no parents.

          > git checkout -f 372ed74bf3c5263a95a238cf522f88f49ee40456 # timeout=10

          > git merge a5f87cb236d06095dd9304ddc09f62158d1d7417 # timeout=10

          This makes a merge commit with parents 372ed74 (initial-branch) and a5f87cb (main) in that order, which is the opposite of what GitHub did.

          Merge succeeded, producing 3cefdb535e9796a46b13705581d2cf7e34c7f258

          The commit ID 3cefdb5 (merge by Jenkins) does not match GitHub's c8bc8c6 (refs/pull/1/merge) but the tree is probably identical.

          [GitCheckoutListener] Found no previous build with recorded Git commits

          GitCheckoutListener.getLatestCommitOfPreviousBuild will return NO_COMMIT_FOUND, and that will be stored in GitCommitsCollector.latestRecordedCommit;.

          [GitCheckoutListener] -> Multiple parent commits found - skipping commits of local merge '3cefdb5'

          This shows isMergeCommit is true in GitCommitsCollector.findHeadCommit, i.e. the problem is not in GitCheckoutListener.isMerge.

          [GitCheckoutListener] -> Using parent commit '372ed74' of local merge as starting point

          This sets BuildCommits.head. Perhaps the reference resolution would work better if this selected 3cefdb5 (merge by Jenkins) rather than 372ed74 (initial-branch).

          [GitCheckoutListener] -> Storing target branch head 'a5f87cb' (second parent of local merge)

          This sets BuildCommits.target, with the correct commit.

          [GitCheckoutListener] -> Recorded 2 new commits

          The two recorded commits must be 372ed74 and 4e002f7.

          However, because a5f87cb was merged in, perhaps it should be likewise recorded and then used for locating the reference build. This could be achieved by using 3cefdb5 (merge by Jenkins) as the BuildCommits.head. (The merge commit 3cefdb5 would not need to be recorded because it won't match anything on the target branch, but I expect it is easier to record.)

          Alternatively, GitCommitsRecord.getReferencePoint might be changed to consider BuildCommits.target as well. I am not sure exactly how that would work.

          Kalle Niemitalo added a comment - Thank you for posting the log and publishing the repository. Obtained Jenkinsfile from 372ed74bf3c5263a95a238cf522f88f49ee40456+a5f87cb236d06095dd9304ddc09f62158d1d7417 (c8bc8c6fa1353e90820065d22a07301cef78191c) c8bc8c6 is refs/pull/1/merge. Its parents are a5f87cb and 372ed74, in that order. Jenkins gets the Jenkinsfile from it but does not appear to use it for other purposes. 372ed74 is refs/heads/initial-branch and refs/pull/1/head. Its parent is 4e002f7. a5f87cb is refs/heads/main. Its parent is 4e002f7. 4e002f7 is the initial commit that has no parents. > git checkout -f 372ed74bf3c5263a95a238cf522f88f49ee40456 # timeout=10 > git merge a5f87cb236d06095dd9304ddc09f62158d1d7417 # timeout=10 This makes a merge commit with parents 372ed74 (initial-branch) and a5f87cb (main) in that order, which is the opposite of what GitHub did. Merge succeeded, producing 3cefdb535e9796a46b13705581d2cf7e34c7f258 The commit ID 3cefdb5 (merge by Jenkins) does not match GitHub's c8bc8c6 (refs/pull/1/merge) but the tree is probably identical. [GitCheckoutListener] Found no previous build with recorded Git commits GitCheckoutListener.getLatestCommitOfPreviousBuild will return NO_COMMIT_FOUND, and that will be stored in GitCommitsCollector.latestRecordedCommit;. [GitCheckoutListener] -> Multiple parent commits found - skipping commits of local merge '3cefdb5' This shows isMergeCommit is true in GitCommitsCollector.findHeadCommit, i.e. the problem is not in GitCheckoutListener.isMerge. [GitCheckoutListener] -> Using parent commit '372ed74' of local merge as starting point This sets BuildCommits.head. Perhaps the reference resolution would work better if this selected 3cefdb5 (merge by Jenkins) rather than 372ed74 (initial-branch). [GitCheckoutListener] -> Storing target branch head 'a5f87cb' (second parent of local merge) This sets BuildCommits.target, with the correct commit. [GitCheckoutListener] -> Recorded 2 new commits The two recorded commits must be 372ed74 and 4e002f7. However, because a5f87cb was merged in, perhaps it should be likewise recorded and then used for locating the reference build. This could be achieved by using 3cefdb5 (merge by Jenkins) as the BuildCommits.head. (The merge commit 3cefdb5 would not need to be recorded because it won't match anything on the target branch, but I expect it is easier to record.) Alternatively, GitCommitsRecord.getReferencePoint might be changed to consider BuildCommits.target as well. I am not sure exactly how that would work.

          Ulli Hafner added a comment -

          I see. I think re-using some information of the branch API plugin makes sense for PRs.

          Ulli Hafner added a comment - I see. I think re-using some information of the branch API plugin makes sense for PRs.

          Kalle Niemitalo added a comment -

          I should file a separate feature request for making ReferenceRecorder use the target branch of a pull request as the reference branch.

          Filed as JENKINS-66672.

          Kalle Niemitalo added a comment - I should file a separate feature request for making ReferenceRecorder use the target branch of a pull request as the reference branch. Filed as JENKINS-66672 .
          Kalle Niemitalo made changes -
          Link New: This issue is related to JENKINS-66672 [ JENKINS-66672 ]

            drulli Ulli Hafner
            cetlan Christopher
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated:
              Resolved: