Skip to content
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

CompileOnly for the KGP in wire-gradle-plugin #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lizard-boy
Copy link

No description provided.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR updates the Wire Gradle plugin configuration and test projects, focusing on dependency management and build settings. Key changes include:

  • Modified wire-gradle-plugin/build.gradle.kts to use 'compileOnly' for Kotlin Gradle Plugin dependency
  • Updated test projects to use version catalogs for plugin versions
  • Added new settings.gradle.kts files for build cache configuration in test projects
  • Removed old settings.gradle files from test projects
  • Updated WirePluginTest.kt to accommodate changes in plugin behavior and configuration

These changes aim to improve the plugin's build process, test setup, and version management across the Wire project.

30 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings

@@ -35,14 +35,15 @@ gradlePlugin {
dependencies {
compileOnly(gradleApi())
compileOnly(libs.pluginz.android)
compileOnly(libs.pluginz.kotlin)
Copy link

Choose a reason for hiding this comment

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

logic: Moving KGP to compileOnly may cause issues if the plugin relies on KGP classes at runtime

dependsOn(":wire-schema:publishAllPublicationsToLocalMavenRepository")
dependsOn(":wire-grpc-client:publishAllPublicationsToLocalMavenRepository")
dependsOn(":wire-compiler:publishAllPublicationsToLocalMavenRepository")
dependsOn(":wire-gradle-plugin:publishAllPublicationsToLocalMavenRepository")
Copy link

Choose a reason for hiding this comment

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

logic: Circular dependency: wire-gradle-plugin depends on itself for tests

Copy link

Choose a reason for hiding this comment

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

logic: Removing buildCache configuration may affect test behavior. Ensure this is intentional.

Copy link

Choose a reason for hiding this comment

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

logic: Removing the build cache configuration may affect caching behavior in tests. Ensure this is intentional and doesn't break any caching-related tests.

@@ -0,0 +1,24 @@
buildCache {
local {
directory = File(rootDir, "../.cache-include-paths-build-cache")
Copy link

Choose a reason for hiding this comment

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

style: Using a relative path for the build cache directory may cause issues if the project structure changes

it.apply {
mavenCentral()
maven {
url = uri(rootDir.resolve("../../../../../build/localMaven").absolutePath)
Copy link

Choose a reason for hiding this comment

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

style: Absolute path for local Maven repository reduces build portability

dependencyResolutionManagement {
versionCatalogs {
create("libs") {
from(files(rootDir.resolve("../../../../../gradle/libs.versions.toml").absolutePath))
Copy link

Choose a reason for hiding this comment

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

style: Absolute path for version catalog file may cause issues in different environments

Comment on lines +3 to +4
id 'org.jetbrains.kotlin.jvm' version libs.versions.kotlin
id 'com.squareup.wire' version "$wireVersion"
Copy link

Choose a reason for hiding this comment

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

style: Consider using the same version declaration style for both plugins for consistency

Comment on lines +2 to +3
id("com.squareup.wire") version("$wireVersion")
id("org.jetbrains.kotlin.jvm") version libs.versions.kotlin
Copy link

Choose a reason for hiding this comment

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

style: Consider using the same version declaration style for both plugins for consistency

Comment on lines +3 to +4
id 'org.jetbrains.kotlin.jvm' version libs.versions.kotlin
id 'com.squareup.wire' version "$wireVersion"
Copy link

Choose a reason for hiding this comment

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

style: Consider using the same version declaration style for both plugins for consistency

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