-
Notifications
You must be signed in to change notification settings - Fork 56
Add support for Gradle to PCT #795
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add support for Gradle to PCT #795
Conversation
oleg-nenashev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good so far!
src/main/java/org/jenkins/tools/test/PluginCompatTesterCli.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkins/tools/test/gradle/ExternalGradleRunner.java
Outdated
Show resolved
Hide resolved
| private final Process p; | ||
|
|
||
| @CheckForNull | ||
| private final File buildLogFile; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe , GRADLE_BUILD_TOOL to follow the formal name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do we perform the same functionality for gradle? Additionally why is this disabled but JenkinsTestHarnessHook2 remains?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ref to build logs, before disabling any of these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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