-
Notifications
You must be signed in to change notification settings - Fork 3
Update to Gradle 9 and make more progress toward configuration cache compatibility #234
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
Conversation
… instead of `project.copy`
…er file configuration. Update StopLabKey properties and conditions
…hat directory to be created when not otherwise used (e.g., for TeamCity's distribution deployment
…cation.properties file and fix up executeSql method so it finds the right drivers
src/main/groovy/org/labkey/gradle/plugin/extension/DistributionExtension.groovy
Outdated
Show resolved
Hide resolved
| if (BuildUtils.embeddedProjectExists(project)) { | ||
| def embeddedProject = project.project(BuildUtils.getEmbeddedProjectPath()) |
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.
Is BuildUtils.embeddedProjectExists even relevant anymore? (probably out of scope but something to clean up later)
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.
It is still technically possible that you won't have the embedded project included in your settings.gradle file. I'm not sure why you would, but it's possible.
| static String getLabKeyServerPort(Project project) | ||
| { | ||
| return getTeamCityProperty(project, 'tomcat.port', null) | ||
| } |
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.
Putting these static TeamCity property getters in the task class seems odd. It also causes some method name overlap; there is a static getLabKeyServerPort that gets the property but there is also the implicit non-static getLabKeyServerPort for the task input.
If TeamCityExtension isn't the right place for these, maybe they could live in a new TeamCityUtils (similar to BuildUtils).
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.
The placement of these methods doesn't seem odd to me, but good point about the name overlap. I don't remember the evolution of this change, but I am generally moving away from heavy usage of Extension classes where possible because we don't seem to be using them in the way they are intended in a lot of places. No real harm for static classes, though, so I've moved these methods back (for now, at least).
Rationale
Gradle 9.0.0 has been released (and 9.1.0 is coming soon). This release has removed some deprecated features and moves even closer to making the use of the configuration cache the default behavior, so getting out build closer to compatibility will help make the transition easier whenever that happens (Probably Gradle 10, but I'm unsure on the timing of that).
Related Pull Requests
Changes
listNodeModulestask defined byNpmRunplugincleanOuttask defined byFileModuleplugincleanAndDeploytask fromServerDeploypluginCopyAndInstallRPackageTask class to use injected FileSystem instead ofproject.copyGzipActiontask to use task logger and ant builder objectsStopLabKeyandStartLabKeywith appropriate dependencies to mirror the deprecatedStopTomcatandStartTomcatgetServerDeployDirectoryfromServerDeployExtensioncreateNlpConfigtask fromTeamCityplugin since that module is no longer in usesetuptask fromServerDeployplugin.DeployDistributionandStageDistributionfor configuration cache compatibilityModuleDistributionfor better configuration cache compatibilityPropertiesUtilsto remove code for supportinglabkey.xml.restartTriggerFileto prevent creation of emptybuild/deploy/modulesdirectory for embedded distribution deploymentbootstraptask so it will updateapplication.properties