jenkinsci / gradle-jpi-plugin

Build Jenkins Plugins with Gradle
79 stars 50 forks source link

Generate git based version #218

Closed alextu closed 1 year ago

alextu commented 1 year ago

This fixes https://github.com/jenkinsci/gradle-jpi-plugin/issues/214, some notes about the suggested implementation we can discuss:

sghill commented 1 year ago

Thanks for getting started on this and kicking off the discussion @alextu!

I have a few questions:

  1. I don't see anything here that is dependent on this plugin - what do you think about making this versioning scheme its own separately published plugin?
  2. Have you considered setting the version to an instance of GitVersion and having its #toString() lazily calculate (and cache) the version? Gradle will accept any object as the version value.
  3. JGit is a great library, but I prefer to avoid dependencies to the buildscript classpath when possible by using the worker api's classloader isolation mode. This would mean jgit is isolated on its own configuration and there is a generateVersion task that writes to a file. Artifact generation tasks would then need a dependency to generateVersion. Definitely some ceremony, but the benefit is no one has to deal with dependency conflict issues on the classpath - what do you think of this?

To make sure it's set as early as possible, we could use a settings plugin instead of this project plugin.

I don't think it's necessary to make this a settings plugin. It's reasonable to expect anyone consuming the version is doing so as late as possible, as that aligns with several versioning plugins already available.

my impl only checks for commits in other branches that could have the same abbreviated hash + commit depth.

I prefer this is removed in favor of org.eclipse.jgit.lib.ObjectReader#abbreviate. As a user, the only recourse here is to extend the length of the abbreviation. This JGit method will do that automatically, which I prefer over making another request of the user.

It's also unusual to be producing artifacts from multiple branches without a prefix, simulating a different major version. I think it'd be good to include prefix support to align with the maven-hpi-plugin.

Some tests are based on some git sample repos, for simplicity sake there are stored as is

I love seeing the tests. Over the long haul I think these kinds of tests are simpler to maintain when they are generated from within the test itself. Generation to the build directory also avoids the issue of the IDEs and tooling picking up the .git directory. Since we have JGit here already, that seems achievable?

alextu commented 1 year ago

Thanks for getting started on this and kicking off the discussion @alextu!

Thanks for reviewing this draft!

I have a few questions:

1. I don't see anything here that is dependent on this plugin - what do you think about making this versioning scheme its own separately published plugin?

I think that's a good idea, if it's ok to assume users would have to modify their build scripts and apply 2 plugins.

2. Have you considered setting the version to an instance of `GitVersion` and having its `#toString()` lazily calculate (and cache) the version? Gradle will accept [any object](https://docs.gradle.org/current/javadoc/org/gradle/api/Project.html#getVersion--) as the version value.

I can, yes but might not be necessary given the next point

3. JGit is a great library, but I prefer to avoid dependencies to the buildscript classpath when possible by using the [worker api's classloader isolation mode](https://docs.gradle.org/current/userguide/worker_api.html#changing_the_isolation_mode). This would mean jgit is isolated on its own configuration and there is a `generateVersion` task that writes to a file. Artifact generation tasks would then need a dependency to `generateVersion`. Definitely some ceremony, but the benefit is no one has to deal with dependency conflict issues on the classpath - what do you think of this?

I've been wondering about the 2 options:

  • setting the version at configuration time, as early possible a bit like the Nebula release plugin. It's quite straightforward but no classpath isolation and also it might not be configuration cache compatible by default (depending on when called and implemented)
  • setting the version at execution time with a task. It definitly adds some ceremony on the user side and they'll have to know when to execute this task, how to wire it up. For example, the user could set it for the publications:
    publishing {
    publications {
    create<MavenPublication>("mavenJpi") {
    // generate is the task generating the version file
    version = generate.outputFile.get().getAsFile().readText(Charsets.UTF_8)
    }
    }
    }

    Which seems quite elegant, but then the other tasks like jar generating the manifest file will not have the same version. Then instead the user could set the project version with the proper wiring, for example:

val generate by tasks.register<GenerateVersionTask>("generateVersion") {
    doLast {
        project.version = outputFile.get().getAsFile().readText(Charsets.UTF_8)
    }
}

tasks.jar {
    dependsOn(generate)
}

To make sure it's set as early as possible, we could use a settings plugin instead of this project plugin.

I don't think it's necessary to make this a settings plugin. It's reasonable to expect anyone consuming the version is doing so as late as possible, as that aligns with several versioning plugins already available.

my impl only checks for commits in other branches that could have the same abbreviated hash + commit depth.

I prefer this is removed in favor of org.eclipse.jgit.lib.ObjectReader#abbreviate. As a user, the only recourse here is to extend the length of the abbreviation. This JGit method will do that automatically, which I prefer over making another request of the user.

Nice! It's also a very very unlikely situation to occur for the default abbrev length of 12 😄

It's also unusual to be producing artifacts from multiple branches without a prefix, simulating a different major version. I think it'd be good to include prefix support to align with the maven-hpi-plugin. Totally, it could be configured as part of the plugin extension, something like

gitVersion {
    format = "rc-%d.%s"
}

Some tests are based on some git sample repos, for simplicity sake there are stored as is

I love seeing the tests. Over the long haul I think these kinds of tests are simpler to maintain when they are generated from within the test itself. Generation to the build directory also avoids the issue of the IDEs and tooling picking up the .git directory. Since we have JGit here already, that seems achievable? Yes, some tests already use Jgit to create sample Git repos, but testing collisions is done with an embedded sample repo already containing collisions. Anyways, with the previous comments, those tests will probably be refactored.

I'll discuss all of this with my team at Gradle and I'll keep you posted

alextu commented 1 year ago

@sghill after discussing internally we think it's best to have this logic live in the jpi plugin. I've applied your other suggestions: I've created a generateGitVersion task that generates a git based version according to the specified optional parameters in a new extension:

jenkinsPlugin {
  gitVersion {
     versionFormat = '....'
     allowDirty = true
     abbrevLength = 10
     gitRoot = ....
  }
}

The task internally uses the Worker api with classpath isolation. Let me know what you think before I polish a bit and convert to a proper PR

sghill commented 1 year ago

This looks good to me @alextu! I fixed the merge conflict - was there anything you wanted to polish before merging?

By the way thanks very much for the contributions - awesome to hear you discussed the changes internally at Gradle. Gradle is definitely one of my favorite tools. It can do just about anything, has saved me tons of time over the years, and just keeps getting better.

alextu commented 1 year ago

Thanks for the feedback @sghill and the kind words! I'll ask some of my teamates to review this PR and I'll let you know when it's ok to merge (wanted to have your validation first)

alextu commented 1 year ago

I've also added a bit of documentation to the README regarding the previously merged PRs. @sghill I'll let you check and merge

sghill commented 1 year ago

Thanks for doc updates and reviews! Will get an rc out this week so we can try it out.

sghill commented 1 year ago

This is now available in 0.48.0-rc.2

sghill commented 1 year ago

0.48.0 was released today with this change