streem / pbandk

Kotlin Code Generator and Runtime for Protocol Buffers
MIT License
264 stars 38 forks source link

Build conflict between pbandk-protos and protobuf-java(lite) #157

Open JeroenMols opened 3 years ago

JeroenMols commented 3 years ago

Since the wkt .proto files got added to a separate artifact in https://github.com/streem/pbandk/pull/151, this now creates a build error when both pbandk and a version of protobuf-java(lite) are added to a project:

* What went wrong:
Execution failed for task ':app:mergeDebugJavaResource'.
> A failure occurred while executing com.android.build.gradle.internal.tasks.Workers$ActionFacade
   > More than one file was found with OS independent path 'google/protobuf/field_mask.proto'.

An example to reproduce this would be to add the following dependencies to an Android project:

dependencies {
    implementation "pro.streem.pbandk:pbandk-runtime:0.10.0"
    implementation "com.google.firebase:firebase-perf-ktx:20.0.0" // transitively depends on `protobuf-javalite`
}

The root cause for this is that both pbandk-protos and protobuf-javalite contain the same wtk .proto files:

Screen Shot 2021-06-03 at 9 44 27 AM Screen Shot 2021-06-03 at 9 38 49 AM

Short term

A (quick and dirty?) fix would be to add the following to the build.gradle file of the project using these libraries:

android {
    packagingOptions {
        pickFirst 'google/protobuf/*.proto'
    }
}

Is it safe to assume that the .proto files are only required for code generation? And hence they can be safely stripped out during runtime of the application? (e.g. we could even use exlude instead of pickFirst)

Long term

Beyond the quick workaround, we should consider a more structural solution to this. Conflicts with the different protobuf-java are quite tedious and put a burden on customers using pbandk transitively through an SDK.

Would it be possible to only add pbandk-protos as a build dependency for the prototype generation module?

garyp commented 3 years ago

We just keep running into one conflict after another with the protobuf-java libraries 😞

Is it safe to assume that the .proto files are only required for code generation? And hence they can be safely stripped out during runtime of the application? (e.g. we could even use exlude instead of pickFirst)

Yeah that's a safe assumption. We use the following in Streem's Android SDK, since we don't want to ship our .proto files to our SDK's consumers:

android {
    packagingOptions {
        exclude("**/*.proto")
    }
}

I guess this is also why we hadn't run into this bug ourselves. If you want a short-term solution, I'd recommend doing the same in the Plaid SDK.

Beyond the quick workaround, we should consider a more structural solution to this. Conflicts with the different protobuf-java are quite tedious and put a burden on customers using pbandk transitively through an SDK.

Agreed. Customers with a transitive pbandk dependency shouldn't have to jump through any hoops when depending on both pbandk and protobuf-java(lite).

Would it be possible to only add pbandk-protos as a build dependency for the prototype generation module?

The reason I had added pbandk-protos as a dependency to pbandk-runtime-(jvm|android) was to preserve backwards-compatibility for users who were using pbandk together with the Protobuf Gradle Plugin. When pbandk got rid of its dependency on protobuf-java(lite), it also broke projects using the Protobuf Gradle Plugin because the plugin requires one of the project's dependencies to provide the WKT .proto files that are needed during code generation. So I added pbandk-protos so that things would keep working. Obviously that solution fails in the case that multiple dependencies provide the WKT .proto files.

I think the easiest solution might be to remove the explicit dependency on pbandk-protos from pbandk-runtime-(jvm|android). And instead add documentation to say that projects that use the Protobuf Gradle Plugin together with pbandk have to explicitly depend on both pbandk-runtime and pbandk-protos. For example, the dependencies block in pbandk/examples/gradle-and-jvm/build.gradle.kts would include:

dependencies {
    implementation("pro.streem.pbandk:pbandk-runtime:$pbandkVersion")
    implementation("pro.streem.pbandk:pbandk-protos:$pbandkVersion")
}

That'll require a bit of testing but I think it should work.

JeroenMols commented 3 years ago

Would it be possible to use a Maven pom.xml dependency exclusion (similar to this https://stackoverflow.com/questions/9119055/excluding-maven-dependencies) to exclude 'pbandk-protos' as a transitive dependency for 0.10.0?

On Sun, Jun 6, 2021, 6:42 AM Gary Peck @.***> wrote:

We just keep running into one conflict after another with the protobuf-java libraries 😞

Is it safe to assume that the .proto files are only required for code generation? And hence they can be safely stripped out during runtime of the application? (e.g. we could even use exlude instead of pickFirst)

Yeah that's a safe assumption. We use the following in Streem's Android SDK, since we don't want to ship our .proto files to our SDK's consumers:

android {

packagingOptions {

    exclude("**/*.proto")

}

}

I guess this is also why we hadn't run into this bug ourselves. If you want a short-term solution, I'd recommend doing the same in the Plaid SDK.

Beyond the quick workaround, we should consider a more structural solution to this. Conflicts with the different protobuf-java are quite tedious and put a burden on customers using pbandk transitively through an SDK.

Agreed. Customers with a transitive pbandk dependency shouldn't have to jump through any hoops when depending on both pbandk and protobuf-java(lite).

Would it be possible to only add pbandk-protos as a build dependency for the prototype generation module?

The reason I had added pbandk-protos as a dependency to pbandk-runtime-(jvm|android) was to preserve backwards-compatibility for users who were using pbandk together with the Protobuf Gradle Plugin. When pbandk got rid of its dependency on protobuf-java(lite), it also broke projects using the Protobuf Gradle Plugin because the plugin requires one of the project's dependencies to provide the WKT .proto files that are needed during code generation. So I added pbandk-protos so that things would keep working. Obviously that solution fails in the case that multiple dependencies provide the WKT .proto files.

I think the easiest solution might be to remove the explicit dependency on pbandk-protos from pbandk-runtime-(jvm|android). And instead add documentation to say that projects that use the Protobuf Gradle Plugin together with pbandk have to explicitly depend on both pbandk-runtime and pbandk-protos. For example, the dependencies block in pbandk/examples/gradle-and-jvm/build.gradle.kts would include:

dependencies {

implementation("pro.streem.pbandk:pbandk-runtime:$pbandkVersion")

implementation("pro.streem.pbandk:pbandk-protos:$pbandkVersion")

}

That'll require a bit of testing but I think it should work.

β€” You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/streem/pbandk/issues/157#issuecomment-855336373, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQG4FLMWXRARYRROV3WUNTTRL4AFANCNFSM46AJ5UTQ .

garyp commented 3 years ago

@JeroenMols I'm not very familiar with pom.xml syntax, aside from the basics, and I can't figure out from that Stack Overflow post exactly what you have in mind. Can you give an example of how you think pbandk's pom.xml should look? And what effect that change will have?

JeroenMols commented 3 years ago

Sorry, terse reply from phone.

So if I understand this correctly, pbandk-protos is only required for code generation and isn't required at runtime to make the generated code work? In that case, pbandk should indeed best remove the explicit dependency as you suggest.

For people that currently ship pbandk as part of an SDK however, there is a short term fix available by updating the SDK pom.xml to exclude the transitive dependency:

 <dependency>
      <groupId>pro.streem.pbandk</groupId>
      <artifactId>pbandk</artifactId>
      <version>0.10.0</version>
      <exclusions>
        <exclusion>
          <groupId>pro.streem.pbandk</groupId>
          <artifactId>pbandk-protos</artifactId>
        </exclusion>
      </exclusions>
</dependency>

We've used it before to exclude the transitive protobuf-java dependency from pbandk:0.9.0.rc.1: https://search.maven.org/artifact/com.plaid.link/sdk-core/3.1.0/aar

ninovanhooff commented 1 year ago

Any update on this one?

sebaslogen commented 6 months ago

So the easiest solution from the consumer is to use exclude block when you import the runtime. Like this:

implementation("pro.streem.pbandk:pbandk-runtime:$pbandkVersion") {
    // Exclude pbandk-protos to avoid conflicts with proto files of other libs
    exclude(group = "pro.streem.pbandk", module = "pbandk-protos")
}

Note: AFAIK at the time of writing this there is no support in gradle for using exclude block in combination with a libs.toml dependency catalog, so you need to use the string name in the implementation/api call.

garyp commented 6 months ago

@sebaslogen Yeah the exclude block should work. It's the gradle equivalent of the pom.xml solution mentioned earlier. The best way to fix this is still my suggestion from above to remove the dependency:

I think the easiest solution might be to remove the explicit dependency on pbandk-protos from pbandk-runtime-(jvm|android). And instead add documentation to say that projects that use the Protobuf Gradle Plugin together with pbandk have to explicitly depend on both pbandk-runtime and pbandk-protos. For example, the dependencies block in pbandk/examples/gradle-and-jvm/build.gradle.kts would include:

dependencies {
    implementation("pro.streem.pbandk:pbandk-runtime:$pbandkVersion")
    implementation("pro.streem.pbandk:pbandk-protos:$pbandkVersion")
}

That'll require a bit of testing but I think it should work.

I'm happy to accept a PR that makes this change with corresponding updates to the docs and example project. (I realize there are open PRs in this project that haven't been accepted in a long time. I plan to start actively maintaining pbandk again in the next few weeks, so I promise the PR won't sit around ignored.)

Maragues commented 3 months ago

I made it work with this configuration in my SDK (KMP project)

commonMain.dependencies {
  api("pro.streem.pbandk:pbandk-runtime:0.14.3") {
    exclude(group = "pro.streem.pbandk", module = "pbandk-protos")
  }
  compileOnly("pro.streem.pbandk:pbandk-protos:0.14.3")
}

Now my projects can safely depend on javalite and my SDK.

Thanks for this discussion!