[JENKINS-58716] Support checking out plugins with Incremental versions

[basil]

See JENKINS-58716. Observed in jenkinsci/bom#53. The problem and solution are described well in the bug. The rest is just a small matter of programming.

[basil]

Looks like this wasn't just a small matter of programming after all! When testing out this fix over at jenkinsci/bom#53, I got this error:

org.jenkins.tools.test.exception.PluginSourcesUnavailableException: Problem while checking out plugin sources!
	at org.jenkins.tools.test.PluginCompatTester.testPluginAgainst(PluginCompatTester.java:476)
	at org.jenkins.tools.test.PluginCompatTester.testPlugins(PluginCompatTester.java:299)
	at org.jenkins.tools.test.PluginCompatTesterCli.main(PluginCompatTesterCli.java:164)
Caused by: java.lang.RuntimeException: The git-checkout command failed. || Cloning into '/home/basil/work/third-party/jenkinsci/bom/target/local-test/pct-work/git-server'...
From git://github.com/jenkinsci/git-server-plugin
 * branch            HEAD       -> FETCH_HEAD
fatal: reference is not a tree: 303f29958f6165e96fe689bd85fea2b2db457100

	at org.jenkins.tools.test.PluginCompatTester.cloneFromSCM(PluginCompatTester.java:560)
	at org.jenkins.tools.test.PluginCompatTester.testPluginAgainst(PluginCompatTester.java:462)
	... 2 more

Evaluating this further, the problem is that the tag 303f29958f6165e96fe689bd85fea2b2db457100 exists in my fork basil/git-server-plugin but not the upstream repository jenkinsci/git-server-plugin. However, the POM in the incremental build has a <connection> value of jenkinsci/git-server-plugin, not my fork:

  <scm>
    <connection>scm:git:git://github.com/jenkinsci/git-server-plugin.git</connection>
    <developerConnection>scm:git:git@github.com:jenkinsci/git-server-plugin.git</developerConnection>
    <tag>303f29958f6165e96fe689bd85fea2b2db457100</tag>
    <url>https://github.com/jenkinsci/git-server-plugin</url>
  </scm>

I reproduced the error calling cloneFromSCM in my IDE and found that changing the value of <connection> from scm:git:git://github.com/jenkinsci/git-server-plugin.git to scm:git:git://github.com/basil/git-server-plugin.git fixed the problem.

Based on this evaluation, I think the change in this PR is makes the situation no worse for non-incremental builds and is necessary but not sufficient for fixing JENKINS-58716 for incremental builds. Therefore I think we should go ahead with this PR. Once this PR has been merged, I think the next step would be to fix the incrementals subsystem to generate a POM with a correct <connection> value, referencing the repository from which the incremental was built (which could be a fork). @jglick, what do you think?

[basil]

I sketched out a possible solution to the second half of this problem in JENKINS-58716 (comment).

[oleg-nenashev]

Looks like this wasn't just a small matter of programming after all!

The "small matter of programming" idiom exists specially for these cases

[jglick]

Last I remember, this was still a WiP pending integration with CHANGE_FORK, right?

[basil]

Last I remember, this was still a WiP pending integration with CHANGE_FORK, right?

No, I don't view this as a WiP. This PR is necessary but not sufficient to fix JENKINS-58716. However, I believe it is ready to merge as-is today. From my previous comment:

Based on this evaluation, I think the change in this PR is makes the situation no worse for non-incremental builds and is necessary but not sufficient for fixing JENKINS-58716 for incremental builds. Therefore I think we should go ahead with this PR.

Since writing that, I found another use case where this PR would have an immediate benefit as-is. When trying to add Swarm to the BOM's managed, set I got this error:

Caused by: java.lang.RuntimeException: The git-checkout command failed. || Cloning into '/home/basil/work/third-party/jenkinsci/bom/target/local-test/pct-work/swarm'...
From git://github.com/jenkinsci/swarm-plugin
 * branch            HEAD       -> FETCH_HEAD
error: pathspec 'swarm-3.17' did not match any file(s) known to git

        at org.jenkins.tools.test.PluginCompatTester.cloneFromSCM(PluginCompatTester.java:559)
        at org.jenkins.tools.test.PluginCompatTester.testPluginAgainst(PluginCompatTester.java:462)
        ... 2 more

The cause of this issue is that Swarm uses tags of the form swarm-plugin-3.17 rather than swarm-3.17. This information is present in Swarm's POM, though. Merging this PR should fix this problem. Therefore this PR has independent value even though the additional CHANGE_FORK work is still pending.

[oleg-nenashev]

Merge conflict after #185

[basil]

Merge conflict after #185

I resolved the merge conflict. I also added links to two test runs that demonstrate the code is correctly able to parse SCM tags from the POM and check out the correct tag. The point is that scmTag is a more reliable way of determining what to check out than trying to manually construct a tag name.

[oleg-nenashev]

LGTM

[jglick]

This PR is necessary but not sufficient to fix JENKINS-58716.

Have you been able to confirm yet that it works for that purpose—that if given a POM which contains a commit hash rather than a tag as the scmTag, the PCT checkout will succeed?

[basil]

Have you been able to confirm yet that it works for that purpose—that if given a POM which contains a commit hash rather than a tag as the scmTag, the PCT checkout will succeed?

Yes, I did confirm that, as I wrote in my previous comment:

I reproduced the error calling cloneFromSCM in my IDE and found that changing the value of <connection> from scm:git:git://github.com/jenkinsci/git-server-plugin.git to scm:git:git://github.com/basil/git-server-plugin.git fixed the problem.

[basil]

Perhaps you could take a look at my latest revision again. I made two small improvements:

• Added log statements to make it clear whether the tag was being taken from the POM or inferred.
• Made the behavior of AbstractMultiParentHook consistent with that of PluginCompatTester. Now both of them attempt to use the POM's SCM tag before trying to infer the tag.

[oleg-nenashev]

Would be nice to avoid code duplication, but the rest LGTM

[basil]

Would be nice to avoid code duplication, but the rest LGTM

Agreed. I only just started working with this codebase, but I already see many possible areas for improvement, this being one of them. Maybe as I get time I'll attempt some further cleanup, once I get more familiarity with the codebase and can gain more confidence that I won't be breaking things.

[oleg-nenashev]

Yes, just small steps. I have been doing some refactorings since I started maintaining PCT few years ago, but it is a pretty big codebase. Many issues to fix going forward, just not a top priority. I fix something when I change the code around, but that;s it. Should become better in next releases once we remove Internal Maven and GAE from the codebase

…

On Thu, Sep 12, 2019 at 4:32 PM Basil Crow ***@***.***> wrote: Would be nice to avoid code duplication, but the rest LGTM Agreed. I only just started working with this codebase, but I already see many possible areas for improvement, this being one of them. Maybe as I get time I'll attempt some further cleanup, once I get more familiarity with the codebase and can gain more confidence that I won't be breaking things. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#181?email_source=notifications&email_token=AAW4RIEM6HPITFX7F4JV5S3QJJHI7A5CNFSM4IIWSDW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6SDBFA#issuecomment-530854036>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAW4RIAGG3NJBFVWFUIFQ3TQJJHI7ANCNFSM4IIWSDWQ> .

[basil]

Test failure doesn't appear related to this change:

[2019-09-16T08:17:32.286Z] Exception in thread "main" java.io.IOException: Unable to open file /home/jenkins/workspace/ries_plugin-compat-tester_PR-181/out/pct-report.xsl for writing.
[2019-09-16T08:17:32.286Z] 	at org.codehaus.plexus.util.FileUtils.checkCanWrite(FileUtils.java:1240)
[2019-09-16T08:17:32.286Z] 	at org.codehaus.plexus.util.FileUtils.copyStreamToFile(FileUtils.java:1212)
[2019-09-16T08:17:32.286Z] 	at org.jenkins.tools.test.PluginCompatTester.testPlugins(PluginCompatTester.java:161)
[2019-09-16T08:17:32.286Z] 	at org.jenkins.tools.test.PluginCompatTesterCli.main(PluginCompatTesterCli.java:163)
script returned exit code 1

Closing and re-opening to trigger a new build.

[basil]

I think we should be good to move forward with this. Let me know if there's any other changes you want me to make.

[oleg-nenashev]

After some consideration I will merge it as an enhancement, not as a bugfix. PCT has never really worked with Incrementals, but with this patch we are getting closer to it.

[oleg-nenashev]

@basil would you like to have a new release of PCT? Or are you fine with taking it from the branch?

[basil]

@basil would you like to have a new release of PCT? Or are you fine with taking it from the branch?

Thanks for checking. I think I'm fine without a release for now. There are a few other fixes I want to make in PCT so I think it makes sense to bunch them together before shipping a release.