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

[gax-java] fix: add back javax.annotation dependency #1129

Merged
merged 1 commit into from Jun 16, 2020
Merged

[gax-java] fix: add back javax.annotation dependency #1129

merged 1 commit into from Jun 16, 2020

Conversation

miraleung
Copy link
Contributor

@miraleung miraleung commented Jun 15, 2020

This dependency was removed in PR #1000 in favor of transitively pulling it in from another project. However, it doesn't build in Kokoro when updating the WORKSPACE hash in g3/tp, which is currently set just past v1.55.0. We get errors like this:

/<path>/<project>/BUILD.bazel:71:1: no such package '@javax_annotation_javax_annotation_api//jar': The repository '@javax_annotation_javax_annotation_api' could not be resolved and referenced by  <project>:<project>_java_gapic

@googlebot googlebot added the cla: yes label Jun 15, 2020
@miraleung miraleung requested a review from vam-google Jun 15, 2020
@codecov
Copy link

@codecov codecov bot commented Jun 15, 2020

Codecov Report

Merging #1129 into master will increase coverage by 0.65%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1129      +/-   ##
============================================
+ Coverage     78.72%   79.37%   +0.65%     
- Complexity     1169     1245      +76     
============================================
  Files           204      204              
  Lines          5184     5446     +262     
  Branches        416      471      +55     
============================================
+ Hits           4081     4323     +242     
- Misses          928      931       +3     
- Partials        175      192      +17     
Impacted Files Coverage Δ Complexity Δ
...gle/api/gax/rpc/FixedTransportChannelProvider.java 96.55% <0.00%> (-3.45%) 25.00% <0.00%> (+10.00%) ⬇️
.../com/google/api/gax/rpc/FixedWatchdogProvider.java 100.00% <0.00%> (ø) 19.00% <0.00%> (+9.00%)
...gle/api/gax/rpc/InstantiatingWatchdogProvider.java 92.10% <0.00%> (+1.19%) 29.00% <0.00%> (+13.00%)
...ain/java/com/google/api/gax/rpc/ClientContext.java 82.41% <0.00%> (+1.53%) 17.00% <0.00%> (+8.00%)
...api/gax/grpc/InstantiatingGrpcChannelProvider.java 82.04% <0.00%> (+3.28%) 48.00% <0.00%> (+15.00%)
...httpjson/InstantiatingHttpJsonChannelProvider.java 77.17% <0.00%> (+6.20%) 30.00% <0.00%> (+11.00%)
...src/main/java/com/google/api/gax/rpc/Watchdog.java 87.76% <0.00%> (+7.42%) 18.00% <0.00%> (+7.00%)
...in/java/com/google/api/gax/core/GaxProperties.java 59.25% <0.00%> (+9.25%) 9.00% <0.00%> (+3.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acf1fec...8796110. Read the comment docs.

@miraleung miraleung requested a review from noahdietz Jun 15, 2020
@noahdietz noahdietz removed their request for review Jun 15, 2020
@noahdietz
Copy link
Contributor

@noahdietz noahdietz commented Jun 15, 2020

I'm not sure I know enough about why the dep was removed to approve replacing it. I'll let @vam-google review

@miraleung miraleung requested a review from noahdietz Jun 15, 2020
Copy link
Contributor

@vam-google vam-google left a comment

Looks like the dependency was removed from grpc dependencies (replaced with tomcat version) recently in grpc/grpc-java@6a50a63#diff-515bc54a0cbb4b12fb4a7c465758b011L151, so we can't consume it as a transitive anymore.

The commit above was tagged for 1.30 release. So once we updated gax to depend on grpc 1.30 it started failing. This error is not kokoro-specific and is reproducible on workstations.

The fix is LGTM. Another way to fix it would be to use @org_apache_tomcat_annotations_api dependency instead (like what gRPC started doing).

@miraleung
Copy link
Contributor Author

@miraleung miraleung commented Jun 16, 2020

Summarizing our offline discussion: We decided not to switch to the tomcat dependency, because that change will need to be propagated to all dependent clients.

@miraleung miraleung merged commit 77a4cc3 into master Jun 16, 2020
9 checks passed
@miraleung miraleung deleted the fix/deps branch Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants