Add support for Gradle to PCT#795
Add support for Gradle to PCT#795aaravmahajanofficial wants to merge 14 commits intojenkinsci:masterfrom
Conversation
oleg-nenashev
left a comment
There was a problem hiding this comment.
Looks pretty good so far!
| private final Process p; | ||
|
|
||
| @CheckForNull | ||
| private final File buildLogFile; |
There was a problem hiding this comment.
Do you also consider taking Problem Reports, Configuration Cacher reports, and other utility files?
Maybe for the next iteration, but they could be added to the aggregated PCT report
There was a problem hiding this comment.
Yeah, that would be a great addition ✅
I tried to find aggregated PCT report generation functionality, but didn't find one though. May need to implement one in future IMHO
oleg-nenashev
left a comment
There was a problem hiding this comment.
Sorry, it took me some time to return to the PR. I added some comments about the code that could be improved.
IMHO this PR could be merged after the following conditions are met:
- The PR is updated to the new location of https://github.com/aaravmahajanofficial/jenkins-gradle-convention-plugin after hosting and, possibly, renaming
- There is some documentation added on Gradle Support, e.g. to README. This documentation can indicate the experimental status of the support, and referencing to the requirements (e.g. using the convention plugin)
- NOTE: I Know that the current documentation is not very sufficient (e.g. I created #800 ). For now it should be enough to just add a "Support for Gradle Build Tool" section before useful links
- There is a review/approval from at least one @jenkinsci/plugin-compat-tester-developers except me - I have been on sabbatical and it would not be totally appropriate if I come back and start merging things
|
|
||
| public enum BuildSystem { | ||
| MAVEN("pom.xml"), | ||
| GRADLE("build.gradle", "build.gradle.kts", "settings.gradle", "settings.gradle.kts"); |
There was a problem hiding this comment.
Maybe , GRADLE_BUILD_TOOL to follow the formal name
There was a problem hiding this comment.
Thanks Oleg, fixed this in new commit 👍
|
|
||
| @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "intended behavior") | ||
| public class BuildSystemUtils { | ||
| public static BuildSystem detectBuildSystem(File cloneDir) { |
There was a problem hiding this comment.
You could implement a more universal logic that iterates though the build systems and tries to apply them. For that, Java also allows implementing methods right inside enums.
That said, none of that is strictly needed now as we do not have any other reasonably maintained build tool for Jenkins as of now.
| this.isGradlePom = !pomFile.getName().equals("pom.xml"); | ||
| } | ||
|
|
||
| private static File resolvePomFile(File baseDir, PluginCompatTesterConfig config) { |
There was a problem hiding this comment.
Does it have to happen in the constructor? Having complex logic in it, especially with a risk of exceptions, is rather an anti-pattern.
There was a problem hiding this comment.
Extracted this logic out of the constructor as a new method in the new commit.
| this.config = config; | ||
| this.runner = runner; | ||
| this.pomFile = resolvePomFile(localCheckoutDir, config); | ||
| this.isGradlePom = !pomFile.getName().equals("pom.xml"); |
There was a problem hiding this comment.
This is some dark magic. How do you determine it just by name equation?
Maybe, returning a tuple from the previous method would be more resilient
There was a problem hiding this comment.
Thanks Oleg, I have added an enum PomInfo & PomFile class {File, type of pom file}.
| File gradleWrapper = new File(baseDirectory, SystemUtils.IS_OS_WINDOWS ? "gradlew.bat" : "gradlew"); | ||
| if (externalGradle != null) { | ||
| cmd.add(externalGradle.getAbsolutePath()); | ||
| } else if (gradleWrapper.exists() && gradleWrapper.canExecute()) { |
There was a problem hiding this comment.
this conflicts with the help for --gradle-path which says the platform default will be used if the arg is not supplied.
I would argue we should not be using wrappers for executing the tools (we do not attempt to honor an mvnwrapper).
There was a problem hiding this comment.
Thanks for this suggestion, fixed this in new commit. Simplified the logic to use only the installed gradle or externally provided one.
| } | ||
| } | ||
|
|
||
| private static class GradleGobbler extends Thread { |
There was a problem hiding this comment.
this appears to be unescescary cut-and-paste of https://github.com/jenkinsci/plugin-compat-tester/pull/795/files#diff-3da4b28add832c598dc7f84a32def72cb0376b7c0b34a10edfc489a517fcff41R143
refactor the existing code so that it is shared?
There was a problem hiding this comment.
Thanks 👍, I have introduced a ProcessOutputGobbler class in new commit to reduce the duplication
| || BuildSystemUtils.detectBuildSystem(context.getCloneDirectory()) | ||
| .equals(BuildSystem.GRADLE)) { |
There was a problem hiding this comment.
how do we perform the same functionality for gradle? Additionally why is this disabled but JenkinsTestHarnessHook2 remains?
There was a problem hiding this comment.
There are three hooks that are presently not compatible with Gradle builds, ServletApiWorkaround, JenkinsTestHarnessHook2, HPIPluginHook & JenkinsTestHarnessHook. So, I have disabled all four. For the first three, I have disabled them in the base class that they extend.
There was a problem hiding this comment.
Ref to build logs, before disabling any of these
There was a problem hiding this comment.
There are three hooks that are presently not compatible with Gradle builds, ServletApiWorkaround, JenkinsTestHarnessHook2, HPIPluginHook & JenkinsTestHarnessHook
Right, but they provide functionality that is needed.
You could say that all of the Gradle plugins do not currently need these, but in the future if we need to update the HPIPlugin (which IIUC the gradle code still uses) how would we do that?
I do not see any (example or otherwise) hooks that would do things that could be needed for gradle?
Hello everyone 👋, as part of GSoC project Gradle Convention Plugin for Developing Jenkins Plugins, our aim is to recover the Gradle developer flow for Jenkins. I have been developing a Convention Plugin (which uses Gradle-JPI-Plugin under the hood, but adds new functionality and patches over it, like code-quality tools, support for Jenkins BOM, easy config. etc.).
In continuation of this, next target is to add Native Gradle support in Jenkins PCT.
Focus is to not disturb the already existing Maven logic; instead is to reuse the same logic. As valid pom.xml is required for PCT, I have added ability to make use of generated POM file by
gradle-jpi-plugin.Want to gather valuable feedback from Jenkins community & my GSoC mentors @oleg-nenashev @rahulsom @sghill.
Would be happy to discuss, ideate & improve further.
Thank you! 😄
Testing done
Build Successful when run with:
Submitter checklist