Skip to content

Presubmit upper-bound-check with Google Libraries BOM #6606

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.

Sign up for GitHub

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

Jump to bottom
Merged
chingor13 merged 6 commits into from
Oct 29, 2019
Merged

Presubmit upper-bound-check with Google Libraries BOM #6606

chingor13 merged 6 commits into from
Oct 29, 2019

Conversation

@suztomo
Copy link
Member

@suztomo suztomo commented Oct 28, 2019 •
edited
Loading

Fixes GoogleCloudPlatform/cloud-opensource-java#971 .

@chingor13 , @kolea2 , @BenWhitehead I think this presubmit will help google-cloud-java to find forgotten dependency updates. Does this look good to you?

Background

Elliotte and I are thinking an additional presubmit to ensure upper bounds check to the libraries in Google Libraries BOM. This presubmit ensures Google Libraries BOM will pick up the highest versions among transitive dependencies.

Example failure

This presubmit fails with master (721d749):

+ mvn validate -Dgoogle.cloud.java.version=0.116.1-alpha-SNAPSHOT
[INFO] Scanning for projects...
[INFO] 
[INFO] --------< com.google.cloud.tools.opensource:upper-bounds-check >--------
[INFO] Building Cloud Platform Supported Libraries Upper Bounds Check 2.0.0-SNAPSHOT
[INFO] --------------------------------[ pom ]---------------------------------
[INFO] 
[INFO] --- maven-enforcer-plugin:3.0.0-M2:enforce (enforce) @ upper-bounds-check ---
[WARNING] Rule 0: org.apache.maven.plugins.enforcer.RequireUpperBoundDeps failed with message:
Failed while enforcing RequireUpperBoundDeps. The error(s) are [
Require upper bound dependencies error for com.google.auth:google-auth-library-oauth2-http:0.17.2 paths to dependency are:
+-com.google.cloud.tools.opensource:upper-bounds-check:2.0.0-SNAPSHOT
  +-com.google.cloud:google-cloud-core:1.91.2
    +-com.google.auth:google-auth-library-oauth2-http:0.17.2
and
+-com.google.cloud.tools.opensource:upper-bounds-check:2.0.0-SNAPSHOT
  +-com.google.cloud:google-cloud-core:1.91.2
    +-com.google.api:gax:1.49.1 (managed) <-- com.google.api:gax:1.49.0
      +-com.google.auth:google-auth-library-oauth2-http:0.18.0
and
+-com.google.cloud.tools.opensource:upper-bounds-check:2.0.0-SNAPSHOT
  +-com.google.cloud:google-cloud-storage:1.98.1-SNAPSHOT
    +-com.google.cloud:google-cloud-core-http:1.91.2
      +-com.google.auth:google-auth-library-oauth2-http:0.17.2
and
+-com.google.cloud.tools.opensource:upper-bounds-check:2.0.0-SNAPSHOT
  +-com.google.cloud:google-cloud-storage:1.98.1-SNAPSHOT
    +-com.google.cloud:google-cloud-core-http:1.91.2
      +-com.google.api:gax-httpjson:0.66.1 (managed) <-- com.google.api:gax-httpjson:0.66.0
        +-com.google.auth:google-auth-library-oauth2-http:0.18.0
, 
Require upper bound dependencies error for com.google.auth:google-auth-library-credentials:0.17.2 paths to dependency are:
+-com.google.cloud.tools.opensource:upper-bounds-check:2.0.0-SNAPSHOT
  +-com.google.cloud:google-cloud-core:1.91.2
    +-com.google.auth:google-auth-library-credentials:0.17.2
and
+-com.google.cloud.tools.opensource:upper-bounds-check:2.0.0-SNAPSHOT
  +-com.google.cloud:google-cloud-core:1.91.2
    +-com.google.auth:google-auth-library-oauth2-http:0.17.2
      +-com.google.auth:google-auth-library-credentials:0.17.2
and
+-com.google.cloud.tools.opensource:upper-bounds-check:2.0.0-SNAPSHOT
  +-com.google.cloud:google-cloud-storage:1.98.1-SNAPSHOT
    +-com.google.cloud:google-cloud-core-http:1.91.2
      +-com.google.auth:google-auth-library-credentials:0.17.2
and
+-com.google.cloud.tools.opensource:upper-bounds-check:2.0.0-SNAPSHOT
  +-com.google.cloud:google-cloud-storage:1.98.1-SNAPSHOT
    +-com.google.cloud:google-cloud-core-http:1.91.2
      +-com.google.api:gax-httpjson:0.66.1 (managed) <-- com.google.api:gax-httpjson:0.66.0
        +-com.google.auth:google-auth-library-credentials:0.18.0
]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------

What does that mean?

This output shows 2 errors:

  • com.google.auth:google-auth-library-oauth2-http: 0.18.0 is the highest but 0.17.2 is picked up (because it's closer to the root).
  • com.google.auth:google-auth-library-credentials: 0.18.0 is the highest but 0.17.2 is picked up.

What can we do?

Update the version of the two libraries.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 28, 2019
@suztomo suztomo changed the title [Not for merge yet] Draft of bom-upper-bounds-check.sh Presubmit upper-bound-check with Google Libraries BOM Oct 28, 2019
@suztomo suztomo mentioned this pull request Oct 28, 2019
Closed
@suztomo
Copy link
Member Author

suztomo commented Oct 28, 2019

The errors seem valid. I added "What can we do?" section.

@elharo Do they (this PR and the diagnosis) look good to you?

@elharo
Copy link
Contributor

elharo commented Oct 28, 2019

Sounds accurate. I agree that google-cloud-bom should update google-cloud-core dependency to 1.91.3 ASAP and absolutely before the next release. This is why we need this check. The PR that made this change shouldn't have gone in.

@suztomo suztomo marked this pull request as ready for review October 28, 2019 15:28
@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 28, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 28, 2019
@codecov
Copy link

codecov bot commented Oct 28, 2019 •
edited
Loading

Codecov Report

Merging #6606 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6606   +/-   ##
=========================================
  Coverage     46.35%   46.35%           
  Complexity    28001    28001           
=========================================
  Files          2613     2613           
  Lines        288075   288075           
  Branches      33779    33779           
=========================================
  Hits         133526   133526           
  Misses       144309   144309           
  Partials      10240    10240

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 721d749...b66e454. Read the comment docs.

@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 28, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 28, 2019
BenWhitehead
BenWhitehead reviewed Oct 28, 2019
View reviewed changes
@chingor13 chingor13 added the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 28, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 28, 2019
suztomo
suztomo commented Oct 28, 2019
View reviewed changes
@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 29, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 29, 2019
elharo
elharo reviewed Oct 29, 2019
View reviewed changes
.kokoro/bom-upper-bounds-check.sh
# Read the version of POM. Example version: '0.116.1-alpha-SNAPSHOT'
VERSION_POM=google-cloud-bom/pom.xml
# Namespace (xmlns) prevents xmllint from specifying tag names in XPath
VERSION=`sed -e 's/xmlns=".*"//' $VERSION_POM | xmllint --xpath '/project/version/text()' -`
Copy link
Contributor

@elharo elharo Oct 29, 2019

Choose a reason for hiding this comment

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

why sed -e 's/xmlns=".*"?

Copy link
Member Author

@suztomo suztomo Oct 29, 2019

Choose a reason for hiding this comment

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

Because Namespace (xmlns) prevents xmllint from specifying tag names in XPath.
https://stackoverflow.com/questions/41114695/get-pom-xml-version-with-xmllint

@suztomo
Copy link
Member Author

suztomo commented Oct 29, 2019

xmllint works nicely:

+ VERSION_POM=google-cloud-bom/pom.xml
++ sed -e 's/xmlns=".*"//' google-cloud-bom/pom.xml
++ xmllint --xpath '/project/version/text()' -
+ VERSION=0.116.1-alpha-SNAPSHOT

@chingor13 chingor13 mentioned this pull request Oct 29, 2019
Closed
@BenWhitehead
Copy link
Contributor

BenWhitehead commented Oct 29, 2019

Nice solution with xmllint!

@chingor13
Copy link
Contributor

chingor13 commented Oct 29, 2019

#6650 merged #6649 and this PR and the upper bounds tests passes

@suztomo
Copy link
Member Author

suztomo commented Oct 29, 2019 •
edited
Loading

@chingor13 Thank you for confirmation in 6650. Would you merge this PR? (Only those with write access to this repository can merge pull requests.)

@chingor13 chingor13 merged commit 597f52e into googleapis:master Oct 29, 2019
@suztomo suztomo deleted the bom-upper-bounds branch October 29, 2019 20:25
@chingor13 chingor13 mentioned this pull request Oct 30, 2019
Merged
@codyoss codyoss mentioned this pull request Nov 6, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chingor13 chingor13 chingor13 approved these changes

+2 more reviewers

@elharo elharo elharo left review comments

@BenWhitehead BenWhitehead BenWhitehead left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Can upperBoundCheck run as part of a presubmit in libraries' repositories?

6 participants

@suztomo @elharo @BenWhitehead @chingor13 @googlebot @yoshi-kokoro