Skip to content

Conversation

@aSemy
Copy link
Contributor

@aSemy aSemy commented Mar 1, 2023

  • bump Gradle Plugin Publish Plugin version
  • build script updates & improvements
  • use Java Toolchains to set Java version
  • use jvm-test-fixtures plugin for functionalTest sources

This PR contains no functional changes to the library or plugin.

Things to test

  • Is the plugin still published as expected? All the artifacts should be the same.
  • Is the Maven metadata correct?
  • Is the Gradle metadata still correct? The Java version?

- bump Gradle Plugin Publish Plugin version
- build script updates & improvements
- use Java Toolchains to set Java version
- use jvm-test-fixtures plugin for functionalTest sources
@aSemy aSemy changed the base branch from master to bcv-gradle-rework March 3, 2023 13:48
@qwwdfsad qwwdfsad self-requested a review March 3, 2023 13:52
build.gradle.kts Outdated
doLast {
outputDir.mkdirs()
file(outputDir.resolve("plugin-classpath.txt")).writeText(testPluginRuntimeConfiguration.get().joinToString("\n"))
file(outputDir.resolve("plugin-classpath.txt")).writeText(testPluginRuntimeConfiguration.joinToString("\n"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems to be the really nasty hack from the earlier Gradle era where there were no proper test kits. Could you please check if it's still needed?

Copy link
Contributor Author

@aSemy aSemy Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to do so, but I thought it would be a bit of a larger change than just bumping the Gradle version. It would require updating the test code to remove the addPluginTestRuntimeClasspath() helper function.

Additionally, it seems like this hack is hardcoding the Kotlin version to be 1.8.10, even though the test data specify Kotlin 1.3

So when I remove the hack, the Config Cache checks fail. Not because BCV is incompatible with CC, but because Kotlin 1.3 is incompatible.

I can update all of the test data to use Kotlin 1.8.10?

(btw, the replacement for this hack is this line: gradlePlugin.testSourceSets(functionalTest.sources) withPluginClasspath())

Copy link
Member

@qwwdfsad qwwdfsad Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be totally okay, thanks!

I'd say it's even more preferable to have testdata being built with fresh Kotlin

@qwwdfsad
Copy link
Member

Thanks!

Is the plugin still published as expected? All the artifacts should be the same.

It is not, as I fail to execute ./gradlew publishToMavenLocal -- it now unconditionally requires PGP signature for local publication and testing. None of the developers [usually] have that, it would be nice to preserve the previous behaviour.

The corresponding error:

* What went wrong:
Execution failed for task ':signBinary-compatibility-validatorPluginMarkerMavenPublication'.
> Cannot perform signing task ':signBinary-compatibility-validatorPluginMarkerMavenPublication' because it has no configured signatory


kotlinVersion=1.8.10
pluginPublishVersion=0.10.1
pluginPublishVersion=1.1.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: Bumping this version was required to be compatible with Gradle 8, but it probably caused the signing requirement.

@qwwdfsad
Copy link
Member

I confirmed that local publication works and that publishing works without breaking Sonatype invariants (e.g. it's releasable as is). Consumption of locally published (to mavenLocal) works as well, thanks!

It's already good to go, it's up to you whether you want to get rid of plugin-classpath.txt or proceed with other changes

@aSemy
Copy link
Contributor Author

aSemy commented Mar 21, 2023

I confirmed that local publication works and that publishing works without breaking Sonatype invariants (e.g. it's releasable as is). Consumption of locally published (to mavenLocal) works as well, thanks!

It's already good to go, it's up to you whether you want to get rid of plugin-classpath.txt or proceed with other changes

Great! Thanks for checking. I think this can be merged.

I tried replacing plugin-classpath.txt with the newer withPluginClasspath() Gradle TestKit helper, but it did not work. There were errors in the Kotlin Multiplatform test projects. What I think is happening is that the testRuntime classpath contains a version of the Kotlin plugin or library or Gradle API that clashes and overrides some JAR, the result being that BCV throws a 'class not found' error for the KotlinProjectExtension class. It was weird.

The solution would be to instead publish BCV to a local Maven directory (not necessarily Maven Local). I can do that in a separate PR.

So yes, go ahead and merge please!

@qwwdfsad qwwdfsad self-requested a review March 22, 2023 10:11
Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@qwwdfsad qwwdfsad merged commit b5e0510 into Kotlin:bcv-gradle-rework Mar 22, 2023
qwwdfsad pushed a commit that referenced this pull request May 1, 2023
- bump Gradle Plugin Publish Plugin version
- build script updates & improvements
- use Java Toolchains to set the Java version
- use jvm-test-fixtures plugin for functionalTest sources
- update build config, remove redundant Maven publication
qwwdfsad pushed a commit that referenced this pull request Sep 18, 2023
- bump Gradle Plugin Publish Plugin version
- build script updates & improvements
- use Java Toolchains to set the Java version
- use jvm-test-fixtures plugin for functionalTest sources
- update build config, remove redundant Maven publication
qwwdfsad added a commit that referenced this pull request Jan 12, 2024
* Annotate file paths with `@Language("file-reference")` (#126)
* Update to Gradle 8 (#122)
* bump Gradle Plugin Publish Plugin version
* build script updates & improvements
* use Java Toolchains to set the Java version
* use jvm-test-fixtures plugin for functionalTest sources
* update build config, remove redundant Maven publication

* Initial Gradle convention plugins (#123)
* Apply BCV, commit API dump (#131)
* The ABI shape will be reduced in the future, this is a technical change
* java-diff-utils
* Update Gradle to 8.5

---------

Co-authored-by: aSemy <897017+aSemy@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants