Skip to content

Comments

[BEAM-1881] Exclude protobuf-lite and replace with protobuf-java#2433

Closed
dhalperi wants to merge 2 commits intoapache:masterfrom
dhalperi:no-protobuf-lite
Closed

[BEAM-1881] Exclude protobuf-lite and replace with protobuf-java#2433
dhalperi wants to merge 2 commits intoapache:masterfrom
dhalperi:no-protobuf-lite

Conversation

@dhalperi
Copy link
Contributor

@dhalperi dhalperi commented Apr 5, 2017

They are not compatible; protobuf-lite is only intended for use with Android.

R: @iemejia

Please take a look :)

dhalperi added 2 commits April 5, 2017 09:05
They are not compatible; protobuf-lite is only intended for use with Android.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 70.011% when pulling 9407b14 on dhalperi:no-protobuf-lite into 645d0bb on apache:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 70.014% when pulling 9407b14 on dhalperi:no-protobuf-lite into 645d0bb on apache:master.

@asfbot
Copy link

asfbot commented Apr 5, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9177/
--none--

@asfbot
Copy link

asfbot commented Apr 5, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9178/

Failed Tests: 0


--none--

@dhalperi
Copy link
Contributor Author

dhalperi commented Apr 5, 2017

retest this please

Copy link
Member

@iemejia iemejia left a comment

Choose a reason for hiding this comment

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

LGTM pending the question about the grpc-protobuf-lite dependency.

<dependency>
<groupId>org.apache.beam</groupId>
<artifactId>beam-sdks-java-core</artifactId>
<exclusions>
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch this one!

@@ -534,12 +534,12 @@
<groupId>io.grpc</groupId>
<artifactId>grpc-protobuf-lite</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

Can this dependency be completely removed too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just setting the version for it via dependency management, but AFAICT no because the grpc-protobuf library depends on it. In practice, grpc-protofbuf depends on grpc-protobuf-lite but excludes the transitive protobuf-lite dependency.

</requireMavenVersion>
<bannedDependencies>
<excludes>
<exclude>com.google.protobuf:protobuf-lite</exclude>
Copy link
Member

Choose a reason for hiding this comment

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

👍

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 69.833% when pulling 9407b14 on dhalperi:no-protobuf-lite into 645d0bb on apache:master.

@asfbot
Copy link

asfbot commented Apr 5, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9188/
--none--

@asfgit asfgit closed this in d565f12 Apr 5, 2017
@dhalperi dhalperi deleted the no-protobuf-lite branch April 5, 2017 21:46
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.

4 participants