grpc / grpc-java

The Java gRPC implementation. HTTP/2 based RPC
https://grpc.io/docs/languages/java/
Apache License 2.0
11.48k stars 3.86k forks source link

bazel rules_kotlin visibility issues. #4258

Open hsyed opened 6 years ago

hsyed commented 6 years ago

What version of gRPC are you using?

Bazel:

com_google_protobuf_commit = "106ffc04be1abf3ff3399f54ccf149815b287dd9",
grpc_java_commit = "0a6fd9bd9c192551f1056b751750bc6e718bd20c", # 1.10.0 release.

  _grpc_java_repositories(
    omit_com_google_truth_truth=True,
    omit_com_google_protobuf=True,
    omit_junit_junit=True,
    omit_com_google_code_gson=True,
    omit_com_google_guava=True,
    omit_com_squareup_okio=True,
    omit_io_opencensus_api=True,
    omit_io_opencensus_grpc_metrics=True,
    omit_io_netty_tcnative_boringssl_static=True,
    omit_com_google_code_findbugs_jsr305=True,
    omit_com_google_errorprone_error_prone_annotations=True
  )
ERROR: /Users/hassan/newcode/src/github.com/axsy-dev/server-cloud/cloud/modules/grpc/svc/BUILD:22:1: Compiling 2 Kotlin source files to cloud/modules/grpc/svc/integration_tests.jar failed (Exit 1): builder failed: error executing command 
  (cd /private/var/tmp/_bazel_hassan/0069fedf3f99ce5fa0fd408aceaeb1f0/execroot/__main__ && \
  exec env - \
  bazel-out/host/bin/external/io_bazel_rules_kotlin/kotlin/builder/builder @bazel-out/darwin-fastbuild/bin/cloud/modules/grpc/svc/integration_tests.jar-2.params)
error: supertypes of the following classes cannot be resolved. Please make sure you have the required dependencies in the classpath:
    class io.grpc.inprocess.InProcessServerBuilder, unresolved supertypes: io.grpc.internal.AbstractServerImplBuilder

Target //cloud/modules/grpc/svc:integration_tests failed to build

What did you expect to see?

Visibility resolution not causing a problem.

Problem

I am the maintainer of the Kotlin Bazel rules and I am working on porting some of our Grpc stuff to Kotlin. The visibility of the internal targets is causing the Kotlin compiler problems. I have only hit this problem in Kotlin. The Java rules don't have this problem. The only symbol I am using is the InProcessServerBuilder in my test class.

The Kotlin compiler is probably being too eager. I hope there is a work short term work around.

If there isn't a quick fix would it be possible to open up the visibility ?

carl-mastrangelo commented 6 years ago

The visibility cannot be opened, but im curious why kotlin needs it? AbstractServerImplBuilder should be visible to the InProcessServerBuilder

ejona86 commented 6 years ago

This doesn't seem to be visibility thing (private, protected, etc). It seems AbstractServerImplBuilder could not be found. Maybe you're not including transitive dependencies in your build rule? (Or maybe Kotlin transitive dependency handling has a bug?)

ejona86 commented 6 years ago

I don't see anything more we can do here. It seems the classpath of whatever rule is having trouble needs fixing. We don't have much expertise there.

If you get more details, comment and we can re-open.

hsyed commented 6 years ago

Maybe you're not including transitive dependencies in your build rule

This is indeed the case. The Kotlin rules aren't as sophisticated as the Java rules, and the java rules "strict deps" semantics are emulated.

Changing the Kotlin rules to use the full transitive classpath without strict dep enforcement would be quite a step backward. I do have a POC implementation for strict-deps for Kotlin and this implementation uses the full transitive classpath just like the java rules. However this POC uses the command line jdk jdeps tool for strict-dep post processing. It will be a while before I can work on this again and I'd like some feedback from people who work on bazel full time before I will commit to this design -- I think a Kotlin compiler plugin is better suited.

@ejona86 / @carl-mastrangelo would it be possible to export only the internal target via another java_library or alias and give it a fairly glaring do_not_use_me_esque target (perhaps in the internal package) so I can add it directly to the classpath as a work-around this wouldn't affect the correctness of the visibility when using the dependencies correctly.

FWI for these deps

kt_jvm_library(
    name = "grpc",
    srcs = glob(["*.kt"]),
    visibility = ["//visibility:public"],
    deps = [
        "//third_party/jvm/com/typesafe:config",
        "//third_party/vendor/config4k",
        "//third_party/plugins:immutables",
         # the following two are aliases
        "//third_party:grpc-core", 
        "//third_party:grpc-netty",
        "//third_party/jvm/com/google/guava",
        "//third_party/jvm/org/slf4j:slf4j_api",
        "@io_netty_netty_handler//jar",
        "//third_party/jvm/com/google/inject:guice"
    ],
)

I get the following compile classpath from the rules currently:

bazel-out/darwin-fastbuild/genfiles/external/com_typesafe_config/jar/_ijar/jar/external/com_typesafe_config/jar/config-1.3.2-ijar.jar
bazel-out/darwin-fastbuild/bin/third_party/vendor/config4k/config4k.jar
bazel-out/darwin-fastbuild/genfiles/external/org_immutables_value/jar/_ijar/jar/external/org_immutables_value/jar/value-2.5.6-ijar.jar
bazel-out/darwin-fastbuild/bin/external/grpc_java/core/libcore-hjar.jar
bazel-out/darwin-fastbuild/bin/external/grpc_java/netty/libnetty-hjar.jar
bazel-out/darwin-fastbuild/genfiles/external/com_google_guava_guava/jar/_ijar/jar/external/com_google_guava_guava/jar/guava-24.0-jre-ijar.jar
bazel-out/darwin-fastbuild/genfiles/external/org_slf4j_slf4j_api/jar/_ijar/jar/external/org_slf4j_slf4j_api/jar/slf4j-api-1.7.25-ijar.jar
bazel-out/darwin-fastbuild/genfiles/external/io_netty_netty_handler/jar/_ijar/jar/external/io_netty_netty_handler/jar/netty-handler-4.1.17.Final-ijar.jar
bazel-out/darwin-fastbuild/genfiles/external/com_google_inject_guice/jar/_ijar/jar/external/com_google_inject_guice/jar/guice-4.2.0-ijar.jar
external/com_github_jetbrains_kotlin/lib/kotlin-stdlib.jar
external/com_github_jetbrains_kotlin/lib/kotlin-stdlib-jdk7.jar
external/com_github_jetbrains_kotlin/lib/kotlin-stdlib-jdk8.jar
ejona86 commented 6 years ago

Okay. I think I follow now. The pieces:

  1. Reference InProcessServerBuilder in Kotlin source
  2. Build with Bazel. Bazel supports strict_deps for Kotlin
  3. Strict_deps support in Bazel for Kotlin appears to expose a problem where super classes are necessary. This is different behavior that Java strict_deps
  4. It's unclear where the bug lies for (3). But a quick workaround would be to add the super class (AbstractServerImplBuilder) to the explicit deps, to avoid the strict_deps processing.

I did try briefly to see if I could see how Java works for (3). I glanced at the hjar for :inprocess. It includes a META-INF/TRANSITIVE folder with AbstractServerImplBuilder (and ServerBuilder). The class has had all its methods stripped; not just the method bodies. But there's more magic going on, as you can't reference ServerBuilder at all if you are only depending on :inprocess, while you can reference inherited methods (like executor()).

This makes me feel like (3) could at least be partially blamed on the Kotlin strict_deps implementation. Before we open up a target for working around this issue, I'd really like to see an issue filed for the Kotlin Bazel rules so we can learn if/when this issue is resolved.

hsyed commented 6 years ago

@ejona86 fyi using this as an interim fix.

def some_workspace_setup_macro( grpc_java_commit)
  _github_archive(
      name  = "io_grpc_grpc_java",
      repo = "grpc/grpc-java",
      commit = grpc_java_commit,
      # expose inprocess
      build_file_content="""
alias(
 name="core_internal_exposed",
 actual="//core:internal",
 visibility=["//visibility:public"]
)
"""
  )

def _github_archive(name, repo, commit, build_file_content=None):
  if build_file_content:
      native.new_http_archive(
          name = name,
          strip_prefix = "%s-%s" % (repo.split("/")[1], commit),
          url = "https://github.com/%s/archive/%s.zip" % (repo, commit),
          type = "zip",
          build_file_content=build_file_content
      )
  else:
      native.http_archive(
          name = name,
          strip_prefix = "%s-%s" % (repo.split("/")[1], commit),
          url = "https://github.com/%s/archive/%s.zip" % (repo, commit),
          type = "zip",
      )
ejona86 commented 6 years ago

@hsyed, why the workaround? I am fine with opening up visibility as a workaround, but I want a tracking issue for the Kotlin strict_deps so we can remove the workaround when no longer necessary.

hsyed commented 6 years ago

@ejona86 it's temporary. I am working on kotlin coroutine based grpc stub generation internally and had some mixed mode kotlin / java targets. This was making the build a bit convoluted. Will remove the workaround once you have added the changes.

MarkMarine commented 6 years ago

I'm running into this issue right now @hsyed If there is anything I can do to help out in fixing this, let me know.

mancini0 commented 5 years ago

Also bumping in to this.