protocolbuffers / protobuf

Protocol Buffers - Google's data interchange format
http://protobuf.dev
Other
65.52k stars 15.47k forks source link

duration.proto missing from protobuf-javalite #7094

Closed ericgribkoff closed 4 years ago

ericgribkoff commented 4 years ago

Since the fixing of https://github.com/protocolbuffers/protobuf/issues/1889, protobuf-javalite includes the generated code for well-known types. It also includes the *.proto files for well-known types except for duration.proto. This can make the build process difficult for users (see, e.g., https://github.com/grpc/grpc-java/issues/6573).

Is there a reason that duration.proto was excluded from the protobuf-javalite release? I checked the latest version (3.11.1) but AFAIK it has not been included in any of the javalite artifacts.

Edit 5/26/2020: It looks like com.google.protobuf.util.Timestamps and com.google.protobuf.util.Durations are also missing from the javalite artifact.

gitprelimtek commented 4 years ago

I have created a work around that could hopefully help you. The gist of it is; creating a fatjar using protobuf-javalite , protolite-well-known-types and proto-google-common-protos. Then you can use the output artifact in your gradle dependency inplace of other protobuf artifacts: Refer to this git repo:

https://github.com/gitprelimtek/protobuf-javalite-firebase-wellknowntypes.git

Since the fixing of #1889, protobuf-javalite includes the generated code for well-known types. It also includes the *.proto files for well-known types except for duration.proto. This can make the build process difficult for users (see, e.g., grpc/grpc-java#6573).

Is there a reason that duration.proto was excluded from the protobuf-javalite release? I checked the latest version (3.11.1) but AFAIK it has not been included in any of the javalite artifacts.

Morteza-Rastgoo commented 4 years ago

My working build.gradle:


dependencies {

   implementation 'com.squareup.okhttp:okhttp:2.7.5'
    implementation 'javax.annotation:javax.annotation-api:1.3.2'
    implementation 'io.grpc:grpc-core:1.27.1'
    implementation 'io.grpc:grpc-stub:1.27.1'
    implementation 'io.grpc:grpc-okhttp:1.27.1'
    implementation('io.grpc:grpc-protobuf-lite:1.27.1') {
        exclude module: "protobuf-lite"
    }
}

protobuf {
    protoc {
        artifact = 'com.google.protobuf:protoc:+'
    }
    plugins {
        grpc {
            artifact = 'io.grpc:protoc-gen-grpc-java:+'
        }
        javalite {
            artifact = 'com.google.protobuf:protoc-gen-javalite:+'
        }
    }
    generateProtoTasks {
        all().each { task ->
            task.builtins {
                java {
                    option 'lite'
                }
            }

            task.plugins {
                grpc {
                    // Options added to --grpc_out
                    option 'lite'
                }
            }
        }
    }
}
UncleSamTech commented 4 years ago

Please where is this going to be placed

 protobuf {
        protoc {
            artifact = 'com.google.protobuf:protoc:+'
        }
        plugins {
            grpc {
                artifact = 'io.grpc:protoc-gen-grpc-java:+'
            }
            javalite {
                artifact = 'com.google.protobuf:protoc-gen-javalite:+'
            }
        }
        generateProtoTasks {
            all().each { task ->
                task.builtins {
                    java {
                        option 'lite'
                    }
                }

                task.plugins {
                    grpc {
                        // Options added to --grpc_out
                        option 'lite'
                    }
                }
            }
        }
    }

Is it in dependencies or app

aquadize commented 4 years ago

The workarround doesn't work if you are using protobuf-java for in-app messaging and, for example, Firestore which has nested protobuf-lite dependency.

Is there any way to use protobuf-java:3.11.4 for in-app messaging and firestore:21.4.3?

ejona86 commented 4 years ago

Alternative workaround. Make a folder in your repo (e.g., "fix-javalite") and add google/protobuf/duration.proto to it (keeping that directory structure). Then add an include on that folder:

protobuf { 
    generateProtoTasks {
        all().each { task ->
            task.addIncludeDir(files("fix-javalite/"))
        }
    }
}

The gradle-protobuf-plugin will not generate code for includes; it assumes they are already provided via Java dependencies.

Morteza-Rastgoo commented 4 years ago

@ejona86 Not working for me: image

I also tried this:

            task.addIncludeDir(files("google/api/"))
            task.addIncludeDir(files("google/rpc/"))
ejona86 commented 4 years ago

@Morteza-Rastgoo, it doesn't seem you are impacted by this issue. This issue is concerning javalite containing the generated code for duration.proto, and a few others, but it does not include the .proto itself. It seems you're just wanting to generate code for googleapis. In that case you can use dependencies { protobuf "com.google.api.grpc:proto-google-common-protos:1.18.0" }.

Based on the error message, it seems you've modified annotations.proto to use import "http.proto" instead of import "google/api/http.proto".

files() is relative to the project directory, so you'd need to use:

            task.addIncludeDir(files("src/main/proto/google/api"))
            // or
            task.addIncludeDir(files("$projectDir/src/main/proto/google/api"))

However, that's still very broken, as protobuf-gradle-plugin will run protoc against proto files in src/main/proto; your approach is no better than having src/main/proto/http.proto.

Instead, my workaround suggested having a separate folder. For example, you could use src/main/fix-javalite-proto/google/protobuf/duration.proto and then have task.addIncludeDir(files("src/main/fix-javalite-proto")).

Morteza-Rastgoo commented 4 years ago

@ejona86 I tried part of your advise and this is the result:

image I appreciate that you pay attention to this issue

ejona86 commented 4 years ago

@Morteza-Rastgoo, that is still not related to this issue. Ignoring the warnings, the error is `field_behavior.proto: File not found.". I'm not familiar with a "localization.proto" that is doing the import, so it seems likely that is a problem with your own .proto not importing the right path. If there are further issues, I don't expect this issue to be the proper place to discuss them.

bubenheimer commented 4 years ago

@ejona86 I don't think your workaround for protobufing one's own common protos works these days, that was only valid a few years back pre-R8, or when not minifying with R8. Now, when minifying, Android R8 issues errors about duplicate classes from protobuf-javalite, e.g.:

Type com.google.protobuf.ListValueOrBuilder is defined multiple times: /Users/uli/.gradle/caches/modules-2/files-2.1/com.google.protobuf/protobuf-javalite/3.12.0/8d8ca7983eae28ac654f657f6119e85a0bba9402/protobuf-javalite-3.12.0.jar:com/google/protobuf/ListValueOrBuilder.class, /Users/uli/Developer/Android/bugsNtests/protobufr8duplicates/app/build/intermediates/javac/debug/classes/com/google/protobuf/ListValueOrBuilder.class

This behavior was the last straw that led me to create https://github.com/grpc/grpc-java/issues/6573 (which in turn led to the creation of this issue), because the experience of building and maintaining an Android grpc project with a few well-known protos and Google common protos is harrowing. The only way to build my pretty simple grpc project on Android is by spending hours and days on finding and applying a bunch of hacks, and then spending more time on maintaining this mess when dependency versions and tools change.

This is not the right place for this discussion, but it appears that the problem did not become apparent from my filing of https://github.com/grpc/grpc-java/issues/6573, and the misconception that this works has influenced the handling of other issues like https://github.com/grpc/grpc-java/issues/6554.

Or is there another workaround for the duplicate class problem?

bubenheimer commented 4 years ago

I want to confirm that adding Durations and Timestamps to the protobuf-javalite artifact would indeed address part of my issue from https://github.com/grpc/grpc-java/issues/6573, so I hope this will happen.

robcos commented 4 years ago

I have a PR to add this file https://github.com/protocolbuffers/protobuf/pull/7738/files

robcos commented 4 years ago

The PR has been merged

bubenheimer commented 4 years ago

Was this issue rightfully closed? My understanding is that the issue is also about missing com.google.protobuf.util.Timestamps and com.google.protobuf.util.Durations, which do not seem to have been added by the PR.

robcos commented 4 years ago

Oh, sorry for missing that part. Durations and Timestamps are a different beast!

ejona86 commented 4 years ago

Hmm... protobuf-javalite shouldn't have anything from com.google.protobuf.util.*, since that's part of protobuf-java-util. I see com.google.protobuf.Timestamp and com.google.protobuf.Duration in javalite, so the java code looks correct. However, google/protobuf/duration.proto was missing which was why this was originally filed. So it seems this is fixed.

I think we should just ignore the com.google.protobuf.util.* part, as it is expected and probably just a small mixup when doing the verification.

janbolat commented 4 years ago

@ejona86 Having com.google.protobuf.util.Timestamps and com.google.protobuf.util.Durations in lite version would be very handy to convert Timestamps and Durations to milliseconds, etc and vice-versa.

Otherwise, adding a protobuf-java-util as a separate dependency in Android causes Duplicate class errors.

ejona86 commented 4 years ago

@janbolat, I don't disagree. But I don't think that's what this issue was about. It wasn't a feature request. It was a "you can't use lite as a dependency in .protos" bug. It came out of #1889 which was only partially fixed, which actually made things worse in some cases because it broke the workaround.

I'm going to close this. I don't disagree that having utils available to javalite would be nice. But a separate issue should exist for that feature request.

ejona86 commented 4 years ago

Otherwise, adding a protobuf-java-util as a separate dependency in Android causes Duplicate class errors.

For that, I think you just exclude the protobuf-java dependency from protobuf-java-util. I'm not saying you won't have any further issues after that, though, since I'm not sure anyone has tested that and to my knowledge it is unsupported.