Linkage Monitor detects a genuine error between proto-google-common-protos and appengine-api-1.0-sdk

[suztomo]

From googleapis/java-common-protos#148 (comment) , where Neenu told me that the Linkage Monitor check is blocking the pull request.

Upon checking I think It's a genuine linkage error but how can we bring the check back to green?

The Linkage Monitor says:

May 05, 2021 4:03:30 PM com.google.cloud.tools.dependencies.linkagemonitor.LinkageMonitor run
INFO: BOM Coordinates: com.google.cloud:libraries-bom:20.2.0
May 05, 2021 4:06:57 PM com.google.cloud.tools.dependencies.linkagemonitor.LinkageMonitor run
INFO: The following problems in the baseline no longer appear in the snapshot:
  Class com.google.api.VisibilityProto is not found
  Class com.google.api.VisibilityProto is not found
  Class com.google.api.VisibilityProto is not found
  Class com.google.api.VisibilityProto is not found
  Class com.google.api.VisibilityProto is not found
  Class com.google.api.VisibilityProto is not found

May 05, 2021 4:06:57 PM com.google.cloud.tools.dependencies.linkagemonitor.LinkageMonitor run
SEVERE: Newly introduced problem:
(com.google.api.grpc:proto-google-common-protos:2.1.1-SNAPSHOT) com.google.api.VisibilityProto's method getDescriptor() is expected to return com.google.appengine.repackaged.com.google.protobuf.Descriptors$FileDescriptor but instead returns com.google.protobuf.Descriptors$FileDescriptor
  referenced from com.google.appengine.repackaged.com.google.api.HttpProto (com.google.appengine:appengine-api-1.0-sdk:1.9.71)
  referenced from com.google.appengine.repackaged.com.google.rpc.context.AbuseContextProto (com.google.appengine:appengine-api-1.0-sdk:1.9.71)
  referenced from com.google.appengine.repackaged.com.google.rpc.context.ConditionContextProto (com.google.appengine:appengine-api-1.0-sdk:1.9.71)
  referenced from com.google.appengine.repackaged.com.google.rpc.context.ContextExtensionsProto (com.google.appengine:appengine-api-1.0-sdk:1.9.71)
  referenced from com.google.appengine.repackaged.com.google.rpc.context.QosContextProto (com.google.appengine:appengine-api-1.0-sdk:1.9.71)
  referenced from com.google.appengine.repackaged.com.google.rpc.context.VisibilityContextProto (com.google.appengine:appengine-api-1.0-sdk:1.9.71)

com.google.api.grpc:proto-google-common-protos:2.1.1-SNAPSHOT is at:
  com.google.api.grpc:proto-google-common-protos:2.1.1-SNAPSHOT (compile)
  and 1 dependency path.
com.google.appengine:appengine-api-1.0-sdk:1.9.71 is at:
  com.google.http-client:google-http-client-appengine:1.39.2 (compile) / com.google.appengine:appengine-api-1.0-sdk:1.9.71 (provided)
  and 2 other dependency paths.

The error comes from the discrepancy between repackaged method reference (in appengine-api-1.0-sdk-1.9.71.jar) and non-repackaged implementation (in proto-google-common-protos).

appengine-api-1.0-sdk-1.9.71.jar

$  javap -verbose -cp ~/.m2/repository/com/google/appengine/appengine-api-1.0-sdk/1.9.71/appengine-api-1.0-sdk-1.9.71.jar com/google/appengine/repackaged/com/google/api/HttpProto |less
...
  #149 = Methodref          #130.#54      // com/google/api/VisibilityProto.getDescriptor:()Lcom/google/appengine/repackaged/com/google/protobuf/Descriptors$FileDescriptor;

VisibilityProto.java in this PR

The method in this PR returns non-repackaged version of FileDescriptors.FileDescriptor

  public static com.google.protobuf.Descriptors.FileDescriptor getDescriptor() {
    return descriptor;
  }

In general, when one of the classes marked "not found" in appengine-api-1.0-sdk JAR file (dashboard) is introduced to proto-google-common-protos artifact, it may have such linkage errors on method references due to repackaged classes.

Solutions?

Ideal solution

If we can make the appengine-api-1.0-sdk artifact stop repackaging classes, then that's would be great.

Temporary Solution

We add the method reference to the default exclusion rule.

Others?

Other solution to unblock the pull request (googleapis/java-common-protos#148 (comment))?

[elharo]

what if we update the appengine-api-1.0-sdk? is the problem still present?

[suztomo]

Yes, the problem is still there. The latest JAR (appengine-api-1.0-sdk-1.9.88.jar ) also has the reference to the repackaged class in VisibilityProto.getDescriptor()'s return value:

suztomo-macbookpro44% javap -verbose -cp ~/.m2/repository/com/google/appengine/appengine-api-1.0-sdk/1.9.88/appengine-api-1.0-sdk-1.9.88.jar com/google/appengine/repackaged/com/google/api/HttpProto |less
...
  #157 = Methodref          #138.#60      // com/google/api/VisibilityProto.getDescriptor:()Lcom/google/appengine/repackaged/com/google/protobuf/Descriptors$FileDescriptor;

[elharo]

I don't yet understand why or how this bug is caused.

[elharo]

Is the problem that appengine-api-sdk provides a modified version of the non-repackaged class that uses repackaged classes in its signatures? If so, that's pretty bad.

What I'm trying to figure out is how the use of non-repackaged classes in the proto-common-protos PR leads to a problem with the repackaged classes from the appengine-api-sdk jar.

[suztomo]

In the Past

The class VisibilityProto has been in the "class not found" linkage error in appengine-api-1.0-sdk's linkage check (see dashboard for the details).

Class com.google.api.VisibilityProto is not found, referenced from 6 classes
com.google.appengine.repackaged.com.google.api.HttpProto
com.google.appengine.repackaged.com.google.rpc.context.AbuseContextProto
com.google.appengine.repackaged.com.google.rpc.context.ConditionContextProto
com.google.appengine.repackaged.com.google.rpc.context.ContextExtensionsProto
com.google.appengine.repackaged.com.google.rpc.context.QosContextProto
com.google.appengine.repackaged.com.google.rpc.context.VisibilityContextProto

Now

Now the PR (googleapis/java-common-protos#148 (comment) ) is adding VisibilityProto.java into proto-google-common-protos artifact. This is going to fix the "class not found" error. This is a good thing.

However, there is a discrepancy in getDescriptor method signature between the VisibilityProto class in the PR and appengine-api-1.0-sdk's reference. The pull request has VisibilityProto's getDescriptor returning com.google.protobuf.Descriptors$FileDescriptor, while some repackaged classes in appengine-api-1.0-sdk have the references for the method returning com.google.appengine.repackaged.com.google.protobuf.Descriptors$FileDescriptor. The Linkage Monitor detected the discrepancy:

(com.google.api.grpc:proto-google-common-protos:2.1.1-SNAPSHOT) com.google.api.VisibilityProto's method getDescriptor() is expected to return com.google.appengine.repackaged.com.google.protobuf.Descriptors$FileDescriptor but instead returns com.google.protobuf.Descriptors$FileDescriptor
referenced from com.google.appengine.repackaged.com.google.api.HttpProto (com.google.appengine:appengine-api-1.0-sdk:1.9.71)
referenced from com.google.appengine.repackaged.com.google.rpc.context.AbuseContextProto (com.google.appengine:appengine-api-1.0-sdk:1.9.71)
referenced from com.google.appengine.repackaged.com.google.rpc.context.ConditionContextProto (com.google.appengine:appengine-api-1.0-sdk:1.9.71)
referenced from com.google.appengine.repackaged.com.google.rpc.context.ContextExtensionsProto (com.google.appengine:appengine-api-1.0-sdk:1.9.71)
referenced from com.google.appengine.repackaged.com.google.rpc.context.QosContextProto (com.google.appengine:appengine-api-1.0-sdk:1.9.71)
referenced from com.google.appengine.repackaged.com.google.rpc.context.VisibilityContextProto (com.google.appengine:appengine-api-1.0-sdk:1.9.71)

[elharo]

Thanks for the explanation. That makes sense. My followup question is whether this is going to break anything? I guess not, or at least not anything that wasn't already broken since previously VisibilityProto wasn't found at all? But then why have the repackaged classes bothered to change this at all?

When the linkage checker says, "com.google.api.VisibilityProto's method getDescriptor() is expected to return ..." what is it looking at? That is, why does it think getDescriptor() returns `com.google.appengine.repackaged.com.google.protobuf.Descriptors$FileDescriptor? Are there two versions f this class in the classpath now, or is it looking at what the return value is assigned to, or what? This is very confusing.

[suztomo]

whether this is going to break anything?

No, because (as you wrote) the missing VisibilityProto would have failed already.

But then why have the repackaged classes bothered to change this at all?

(I don't know the exact reason) Usually people shade dependencies to avoid dependency conflicts. I guess the change to VisibilityProto's getDescriptor method was side-effect of it. It hasn't been a problem because appengine-api-1.0-sdk has no code path to touch the method.

"com.google.api.VisibilityProto's method getDescriptor() is expected to return ..." means the 6 classes (HttpProto...) have references to the method with the method signature (in their class files' constant pool section) that returns com.google.appengine.repackaged.com.google.protobuf.Descriptors$FileDescriptor. Here is an example class file content output by javap:

suztomo-macbookpro44% javap -verbose -cp ~/.m2/repository/com/google/appengine/appengine-api-1.0-sdk/1.9.88/appengine-api-1.0-sdk-1.9.88.jar com/google/appengine/repackaged/com/google/api/HttpProto |less
...
  #157 = Methodref          #138.#60      // com/google/api/VisibilityProto.getDescriptor:()Lcom/google/appengine/repackaged/com/google/protobuf/Descriptors$FileDescriptor;