-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
chore(deps): update dependency com.google.cloud:libraries-bom to v16.1.0 #4278
chore(deps): update dependency com.google.cloud:libraries-bom to v16.1.0 #4278
Conversation
Java 11@elharo FYI
|
Interesting. I can't immediately find anything that's changed w.r.t. that class lately. |
Java 8 has the same problem:
|
It seems that this PR had the unintentional side effect of causing the following dependencies to no longer be considered transitive (they are no longer included from version 3.0.4 and up):
|
Strange. I wouldn't have expected that to affect which dependencies are loaded, only which versions of dependencies declared elsewhere. |
Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, or one of your required reviews was not approved. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot. |
@olavloite Where did you get the missing artifacts? (The project and the directory of I compared 2 dependency trees of google-cloud-spanner-jdbc before and after googleapis/java-spanner-jdbc#258 (c9906c9). I couldn't find the 3 missing dependencies. https://gist.github.com/suztomo/0595022143156b3b433f6cc35ea14a7c#dependency-tree-diff |
@suztomo Sorry, the conclusion regarding where/when the diff was introduced was a little premature, and was mainly based on the fact that version 1.17.3 didn't contain much other changes than this, and the fact that it worked in previous versions. But: Versions 1.17.2 and 1.17.1 were never included in any boms, so they have never been tested by the samples. This meas that the previous version that was actually used by the samples was 1.17.0. I did a intersect of the commits between 1.17.0 and 1.173. to figure out where the change occurred, and it seems that it was this PR: googleapis/java-spanner-jdbc#223 One important change in 223 was that the version of the Spanner client library was bumped from 1.60.0 to 2.0.1. Strangely enough it seems that:
Which would have me expect that the problem should have been present in 1.60.0 and not in 2.0.1, but in reality it seems to be the other way around... |
Your investigation suggests to me that the problem is likely in the samples, not google-cloud-spanner. Something in the samples is relying on a transitive dependency that was removed between versions. The other possibility is that google-cloud-spanner is missing a runtime dependency. I don't see any way it could be missing a compile dependency. |
This is the line that's failing
|
I think the problem is in
Those dependencies should really be transitive and it should not be necessary for anyone using https://github.com/googleapis/java-spanner-jdbc/pull/278/files now removes the re-declaration of the |
"Those dependencies should really be transitive and it should not be necessary for anyone using google-cloud-spanner-jdbc to have to explicitly include them. Without grpc-google-cloud-spanner-admin-database-v1 it would for example be impossible to execute DDL statements with the JDBC driver." If that's the case, then they probably should never have been put in separate artifacts in the first place. There might also be an ill-advised interface/implementation split here as well. However assuming that ship has sailed and can't be fixed now, grpc-google-cloud-spanner-admin-database-v1 still shouldn't be a compile scoped dependency of google-cloud-spanner-jdbc (unless it's needed to compile google-cloud-spanner-jdbc which doesn't seem to be the case.) At most it should be a runtime scoped dependency, and even that is iffy. |
Sorry, I'm not sure I understand what you mean in this case:
Sorry, I'm confused, so please bare with me if I'm missing your whole point. As far as I see it, the dependency tree should be as simple as this:
(The problem that caused this build error was that the transitive nature of the dependency was somehow broken between step 2 and 3) I don't see any reason why the above should be treated any differently from:
|
I think the exclusion below is what we're looking for. The published pom.xml of google-cloud-spanner-jdbc (https://search.maven.org/artifact/com.google.cloud/google-cloud-spanner-jdbc/1.17.3/jar) excludes
(The pom.xml at v1.17.3 does not declare these exclusions) Because of this exclusion, the dependencies of google-cloud-spanner do not appear in the dependency tree of google-cloud-spanner-jdbc. Therefore classes in google-cloud-spanner would fail for the missing classes. @stephaniewang526 Would you validate my finding above? |
"google-cloud-spanner (The Spanner client library) has a compile time dependency on grpc-google-cloud-spanner-admin-database-v1. That dependency is needed to execute any 'admin' operations on Spanner, e.g. DDL statements." Is the grpc-google-cloud-spanner-admin-database-v1 dependency needed to compile google-cloud-spanner? If it isn't, it should not be a compile dependency of google-cloud-spanner. If it's needed to run google-cloud-spanner then make it a runtime dependency, not a compile scope dependency. |
Yes. Changing it to a runtime dependency would cause several compile errors, for example in https://github.com/googleapis/java-spanner/blob/master/google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseAdminClientImpl.java |
Thanks. That clears some things up. In that case, @suztomo is likely correct. There's an exclusion in play that shouldn't be there. |
The question is, why are those exclusions there? They aren't in the source pom.xml in Github. Weird. This is what we need to understand and ifx. |
Stephanie found this test-scope declaration (before flattened). |
Yes, my assumption is also that that is what is causing the problem. Although I don't quite understand why it did not cause the problem previously, but does now. googleapis/java-spanner-jdbc#278 removes the test dependencies, and I had already verified locally that it solves this problem. |
db57a03
to
e01bc6a
Compare
I'm going to close this as it looks like we'll need an updated BOM before it's fixed. |
Renovate Ignore NotificationAs this PR has been closed unmerged, Renovate will now ignore this update (16.1.0). You will still receive a PR once a newer version is released, so if you wish to permanently ignore this dependency, please add it to the If this PR was closed by mistake or you changed your mind, you can simply rename this PR and you will soon get a fresh replacement PR opened. |
This PR contains the following updates:
16.0.0
->16.1.0
15.0.0
->16.1.0
Renovate configuration
📅 Schedule: At any time (no schedule defined).
🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.
♻️ Rebasing: Never, or you tick the rebase/retry checkbox.
🔕 Ignore: Close this PR and you won't be reminded about these updates again.
This PR has been generated by WhiteSource Renovate. View repository job log here.