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.

Sign up for GitHub

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

Jump to bottom

Refactor Maven Runner build config #2911

Merged

IgnatBeresnev merged 8 commits into Kotlin:master from aSemy:feat/refactor_maven_runner_build_config

Mar 16, 2023

Contributor

aSemy commented Mar 8, 2023

Part of #2703

Updates the Maven runner build logic to work better with the Gradle API

Removes the explicit cross-project dependency that was used to download the Maven exe twice
Creates a convention plugin for downloading the Maven exe
Removes eager downloading of the Maven exe
Implements normalization (removes a timestamp) of a Maven property file
Some minor build config tweaks to be a little more succint/Gradley/use newer functionality.

aSemy added 2 commits

March 8, 2023 23:53


          initial refactor

4c8cfcd


          tidying up, tests working

d3c4c7c

aSemy commented

View reviewed changes

buildSrc/src/main/kotlin/org/jetbrains/SetupMaven.kt Outdated

-              import java.io.File
-              @Suppress("LeakingThis")
-              open class SetupMaven : Sync() {

Contributor Author

aSemy Mar 8, 2023

The properties in this file were moved to an extension in the new maven-cli-setup.gradle.kts convention plugin

buildSrc/src/main/kotlin/org/jetbrains/SetupMaven.kt Outdated

-                  }
-                  init {
-                      from(mavenBinaryConfiguration.map { file -> project.zipTree(file) })

Contributor Author

aSemy Mar 8, 2023

This line caused eager configuration, so the binary was always downloaded whether it was necessary or not.

integration-tests/maven/build.gradle.kts

Comment on lines -19 to -20

		val setupMavenTask = project(":runners:maven-plugin").tasks.withType<SetupMaven>().single()
		dependsOn(setupMavenTask)

Contributor Author

aSemy Mar 8, 2023

Instead of using a cross-project dependency to re-use the Maven exec, just download it again.

Gradle will cache the downloaded dependency, so it's fine to set it up twice in different subprojects.

runners/maven-plugin/build.gradle.kts

		}

		into(temporaryDir)

Contributor Author

aSemy Mar 8, 2023 •

edited

Instead of worrying about Sync/Copy tasks overwriting, and having to use preserve {}, I think it's best to always use Sync into the task's temporary dir, and use a final Sync task (prepareMavenPluginBuildDir) to sync the files from the other Sync tasks in one go.

runners/maven-plugin/build.gradle.kts Outdated

Comment on lines 66 to 81

+                  doLast("normalize maven-plugin-help.properties") {
+                      // The maven-plugin-help.properties file contains a timestamp by default.
+                      // It should be removed as it is not reproducible and impacts Gradle caching
+                      val pluginHelpProperties = workingDir.resolve("maven-plugin-help.properties")
+                      pluginHelpProperties.writeText(
+                          buildString {
+                              val lines = pluginHelpProperties.readText().lines().iterator()
+                              // the first line is a descriptive comment
+                              appendReproducibleNewLine(lines.next())
+                              // the second line is the timestamp, which should be ignored
+                              lines.next()
+                              // the remaining lines are properties
+                              lines.forEach { appendReproducibleNewLine(it) }
+                          }
+                      )
+                  }

Contributor Author

aSemy Mar 8, 2023

Unfortunately the generated maven-plugin-help.properties contains a timestamp, so Gradle will always consider the output out-of-date.

I wrote this code to fix it - but maybe there's a Maven flag that will disable the timestamp?

This is perhaps a bit OTT/pre-optimising.

Member

IgnatBeresnev Mar 9, 2023

I actually don't understand what helpMojo is supposed to achieve, and I can't google anything sensible. It looks like it should either generate the help goal (which Dokka's Maven plugin doesn't have) or maybe add some help documentation? Were you able to figure it out?

Nothing looks broken to me, so I think it's fine to ignore the timestamp. Perhaps, at some point our Maven plugin should be revisited as well 😅

Contributor Author

aSemy Mar 9, 2023

I think it auto generates a 'help' command based on the properties in the Maven plugin

The file it generates is in runners/maven-plugin/build/maven/generated-sources

But, like you, I'm not sure! I've never developed a Maven plugin.

package org.jetbrains.dokka.maven;

/**
 * Display help information on dokka-maven-plugin.<br>
 * Call <code>mvn dokka:help -Ddetail=true -Dgoal=&lt;goal-name&gt;</code> to display parameter details.
 * @author maven-plugin-tools
 * @goal help
 * @requiresProject false
 * @threadSafe
 */
public class HelpMojo
    extends AbstractMojo
{
    /**
     * If <code>true</code>, display all settable properties for each goal.
     *
     * @parameter property="detail" default-value="false"
     */
    private boolean detail;

    /**
     * The name of the goal for which to show help. If unspecified, all goals will be displayed.
     *
     * @parameter property="goal"
     */
    private java.lang.String goal;

    /**
     * The maximum length of a display line, should be positive.
     *
     * @parameter property="lineLength" default-value="80"
     */
    private int lineLength;

...

I was looking at other options, and I found this Gradle plugin: https://github.com/britter/maven-plugin-development. It might help with developing a Maven plugin, but on the other hand, the current approach seems to be working.

runners/maven-plugin/build.gradle.kts Outdated

Comment on lines 34 to 38

+                      expand(
+                          "mavenVersion" to setupMavenProperties.mavenVersion,
+                          "dokka_version" to dokka_version,
+                          "mavenPluginToolsVersion" to setupMavenProperties.mavenPluginToolsVersion,
+                      )

Contributor Author

aSemy Mar 8, 2023

using the Gradle expand () is a little nicer to use and look at than the previous manual find and replace.

To achieve this, I needed to update the properties in pom.tpl.xml to be of the format ${mavenVersion}

IgnatBeresnev self-requested a review

March 9, 2023 11:40

IgnatBeresnev approved these changes

View reviewed changes

Member

IgnatBeresnev left a comment •

edited

Looks very neat, and thank you for the explanation comments! Only have optional nitpicks

I'll start the integration tests and, if everything's good, will merge it

runners/maven-plugin/build.gradle.kts Outdated Show resolved Hide resolved

integration-tests/maven/build.gradle.kts Outdated Show resolved Hide resolved

runners/maven-plugin/build.gradle.kts Outdated

Comment on lines 66 to 81

+                  doLast("normalize maven-plugin-help.properties") {
+                      // The maven-plugin-help.properties file contains a timestamp by default.
+                      // It should be removed as it is not reproducible and impacts Gradle caching
+                      val pluginHelpProperties = workingDir.resolve("maven-plugin-help.properties")
+                      pluginHelpProperties.writeText(
+                          buildString {
+                              val lines = pluginHelpProperties.readText().lines().iterator()
+                              // the first line is a descriptive comment
+                              appendReproducibleNewLine(lines.next())
+                              // the second line is the timestamp, which should be ignored
+                              lines.next()
+                              // the remaining lines are properties
+                              lines.forEach { appendReproducibleNewLine(it) }
+                          }
+                      )
+                  }

Member

IgnatBeresnev Mar 9, 2023

I actually don't understand what helpMojo is supposed to achieve, and I can't google anything sensible. It looks like it should either generate the help goal (which Dokka's Maven plugin doesn't have) or maybe add some help documentation? Were you able to figure it out?

Nothing looks broken to me, so I think it's fine to ignore the timestamp. Perhaps, at some point our Maven plugin should be revisited as well 😅

buildSrc/src/main/kotlin/org/jetbrains/conventions/maven-cli-setup.gradle.kts Outdated

Comment on lines 14 to 15

		val mavenVersion = "3.5.0"
		val mavenPluginToolsVersion = "3.5.2"

Member

IgnatBeresnev Mar 9, 2023

Very much optional, while we're at it: I think we actually forget to check/update the Maven version because it's buried in a place we don't visit that much. Extracting it to gradle.properties (or into the toml in the other PR) would not hurt

Contributor Author

aSemy Mar 9, 2023

Good point, I think so too. For now I'll put it in gradle.properties, and I think that later they can be moved to libs.versions.toml as part of #2884

Contributor Author

aSemy Mar 9, 2023 •

edited

It was a little more work than I thought to refactor this, but I think it was worth it.

It revealed that the CrossPlatformExec task wasn't really needed, and could be replaced with a Provider that would determine the current operating system and use mvn.cmd on Windows, else use mvn.

Hopefully the GitHub Workflow tests pass 🤞

Member

IgnatBeresnev Mar 16, 2023

Oof, sorry for asking you to do that then, and thanks for doing! I do think it was worth it though!

Contributor Author

aSemy Mar 16, 2023

no apology needed :)


          rename Maven Plugin template POM

baefed3

aSemy force-pushed the feat/refactor_maven_runner_build_config branch from 120cb21 to baefed3 Compare

March 9, 2023 18:59

aSemy and others added 4 commits

March 9, 2023 20:33


          document Exec.environment Provider API workaround

bf195e8


          move Maven versions to gradle.properties, and refactor to remove Cros…

c7617d6

…sPlatformExec (replaced with a OS provider that maps Windows to `mvn.bat` otherwise to `mvn`)


          Merge branch 'master' into feat/refactor_maven_runner_build_config

f7db0d3


          Move maven-cli convention into build-logic

91f0314

Member

IgnatBeresnev commented Mar 16, 2023

After master was merged in, the maven-cli convention was needed to be moved into build-logic. I hope you don't mind I pushed it into your branch

I'll start integration tests once again, will merge it once they pass 👍 👍


          Suppress incubating API

835c6e5

IgnatBeresnev merged commit 12e2a3c into Kotlin:master

29 checks passed

Contributor Author

aSemy commented Mar 16, 2023

After master was merged in, the maven-cli convention was needed to be moved into build-logic. I hope you don't mind I pushed it into your branch

Not at all! Good job noticing it, and thanks for fixing it.

aSemy deleted the feat/refactor_maven_runner_build_config branch

March 16, 2023 11:08

This was referenced Mar 16, 2023

build config: Gradle Version Catalog + type-safe project dependencies #2884

Merged

remove old Maven CLI setup tasks #2930

Merged

aSemy mentioned this pull request

Maven dokka:help #3035

Closed

aSemy mentioned this pull request

Fix Maven plugin help task #3036

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment