bazeltools / bazel-deps

Generate bazel dependencies for maven artifacts
MIT License
249 stars 121 forks source link

Kotlin support generates invalid kt_jvm_import rules #214

Open davidsantiago opened 5 years ago

davidsantiago commented 5 years ago

I've tried depending on some kotlin dependencies as in the following example:

  io.reactivex.rxjava2:
    rxkotlin:
      lang: kotlin
      version: 2.1.0

This generates the following in the BUILD file:

kt_jvm_import(
    name = "rxkotlin",
    jars = [
        "//external:jar/io/reactivex/rxjava2/rxkotlin"
    ],
    runtime_deps = [
       "//third_party/org/jetbrains/kotlin:kotlin_stdlib",
       ":rxjava"
    ],
    visibility = [
        "//visibility:public"
    ]
)

When I actually depend on this with a BUILD rule of my own code, as in the following:

kt_jvm_library(
    name = "common",
    srcs = glob(["src/**/*.kt"]),
    deps = [
      "//third_party/io/reactivex/rxjava2:rxkotlin"
    ],
)

It gives an error:

ERROR: /Users/davidsantiago/Documents/Work/api/third_party/io/reactivex/rxjava2/BUILD:21:1: in kt_jvm_import rule //third_party/io/reactivex/rxjava2:rxkotlin:
Traceback (most recent call last):
    File "/Users/davidsantiago/Documents/Work/api/third_party/io/reactivex/rxjava2/BUILD", line 21
        kt_jvm_import(name = 'rxkotlin')
    File "/private/var/tmp/_bazel_davidsantiago/c923a8cc58b0bcaa2b4cce0c43a9c083/external/io_bazel_rules_kotlin/kotlin/internal/jvm/impl.bzl", line 124, in kt_jvm_import_impl
        struct(kt = kt_info, providers = [Default...])
    File "/private/var/tmp/_bazel_davidsantiago/c923a8cc58b0bcaa2b4cce0c43a9c083/external/io_bazel_rules_kotlin/kotlin/internal/jvm/impl.bzl", line 128, in struct
        JavaInfo(output_jar = jars[0], compile_jar ...], <3 more arguments>)
com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget cannot be cast to com.google.devtools.build.lib.rules.java.JavaInfo

When I look at the documentation in rules_kotlin for the kt_jvm_import rule, it suggests to me that the rule should be written as

kt_jvm_import(
  name = "rxkotlin",
  jars = [
    "//external:jar/io/reactivex/rxjava2/rxkotlin"
  ],
  visibility = [
    "//visibility:public"
  ]
)

Indeed, when I manually edit this file, it does proceed to compilation instead of the bazel error. I'm trying to figure out what's gone wrong here, because I'm pretty sure this was previously working for you guys, but at the same time, I'm seeing what I'm seeing and what the rules_kotlin source says.

johnynek commented 5 years ago

I accepted the PR without tests. Maybe we can make the change you suggest and add tests. How does that sound?

davidsantiago commented 5 years ago

Sure. Should we ping the guy who originally implemented this, in case they have a use case or situation that this also affects?

johnynek commented 5 years ago

Sure.

On Mon, Oct 15, 2018 at 11:32 David Santiago notifications@github.com wrote:

Sure. Should we ping the guy who originally implemented this, in case they have a use case or situation that this also affects?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/johnynek/bazel-deps/issues/214#issuecomment-430021263, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEJdnpZCT82m7v6JGTCbkulPNc2lHxsks5ulP7igaJpZM4Xc53R .

-- P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

davidsantiago commented 5 years ago

OK. I think that looks like @wcurrie . Hello.

I've got a PR for the change and it works, but I wasn't able to figure out all the ins and outs of Scala's test framework before bed last night. Will keep working on it. It's a test that takes a Target and sees that it's correctly written to the text of a kt_jvm_import by Writer. Sound reasonable?

wcurrie commented 5 years ago

Hi @davidsantiago I didn't try a kotlin dependency with maven runtime dependencies. The only cases I tried are in the dependencies.yaml. MVP..

IIUC the suggested fix is: don't emit the runtime_deps for a kotlin library. Should work provided those dependencies are added explicitly to the project (or not actually used at runtime).

kt_jvm_import appears to have runtime_deps. I'll try to understand.

davidsantiago commented 5 years ago

Sure, and just to be clear, I'm not trying to kill the runtime_deps feature. I'm just doing what the rules_kotlin source code says to do here, where it says that "This style will pull in the transitive runtime dependencies of the targets as well." So that would seem to me to be adequate for just about any maven jar you'd depend on.

But I'm definitely not an expert on maven dependencies, just trying to get my code working.

cgruber commented 5 years ago

runtime deps on the kotlin rules kt_jvm_import are broken. kt_jvm_import shouldn't even be necessary except for bazel ijar clobbering function bodies for inline functions. That's being assessed right now at bazelbuild/bazel#4549. In the mean-time, kt_jvm_import should support runtime_deps, but this is broken at HEAD. I'm looking into it, but we're also waiting for some love from Google on the rules_kotlin project.