Skip to content

Conversation

@hsyed
Copy link
Contributor

@hsyed hsyed commented Apr 27, 2018

This wip just includes the structural changes to get something generating for Kotlin artifacts.

To check if it's working just add this to the projects dependencies.yaml

  org.jetbrains.kotlinx:
    kotlinx-coroutines-core:
      lang: kt_jvm
      version : "0.22.5"

some issues that need consideration.

  1. The transitive dependencies of a kotlin artifact could include other kotlin artifacts, some of these will not be mentioned in dependencies.yaml. I assume the current behaviour -- which I haven't tested -- would be to treat these all as java dependencies or as kotlin dependencies. We can't detect the nature of the dep without deep inspection of the artifact.

  2. Akt_jvm_library is generated currently. This should be switched to kt_jvm_import but kt_jvm_import needs to be updated it's currently only really a simple bootstrap rule (add a single jar + -sources.jar to the project and disable ijarification). it needs to be extended to support the regular java artifact concepts such as exports, exports_plugin, deps, runtime_deps. If this is not done then we need to include two entries in the output a kt_jvm_import followed by a java_library for exporting (kt_jvm_library is invalid without sources currently). Also kotlin_jvm_import has a bug currently with triggers when -sources.jar are absent from the generated repository. This is triggered by 0.12.0. This issue will fix the bug, and I might update the rule with the other required changes

I am not in favour of this approach but we need a solution now. I feel kotlin jvm artifacts do not need to be treated specially by bazel -- the only difference with a java_library is that the ijarification process needs to be slightly altered. This issue tracks the proposal to update the core ijar tool.

In conclusion more work needs to be done and I'm hoping the ijar tool gets updated which would make bazel-deps just work without any changes.

Copy link
Collaborator

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

other than this minor request to use more similar naming, I am happy to merge this an iterate.

What do you think?

implicit val langArg: Argument[Language] = new Argument[Language] {
def defaultMetavar: String = "lang"
def read(s: String) = s match {
case "kt_jvm" => Validated.valid(Language.KotlinJvm)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather use kotlin here and try to think of a strategy to handle different backends separately. For instance, in scala, we might want scala 2.11 and 2.12, and we might want jvm and js. I imagine kotlin is similar.

It is not clear the right way to do this in bazel: should we have 1 build that can build all the targets? Or should we instead use toolchains and have parallel CI builds to build each one with a toolchain configuration? We have discussed the scala situation here:

in this PR:
bazel-contrib/rules_scala#399

and some other issues.

Copy link
Contributor Author

@hsyed hsyed May 30, 2018

Choose a reason for hiding this comment

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

I don't have any objections to kotlin although the rules were switched to kt_ for parity with Google's rule naming scheme. So we have kt_jvm_ at the moment for the jvm rules. kt_ on it's own will be used for platform neutral code in the rules. Specifically we can have a kt_library rule listing source files and a kt_jvm_library rule being an implementation of it -- a bit like the proto rules at the moment.

So just to get on the same page. If we don't add the "backend" part to the lang name or as another attribute in the dep. That would mean the backend applies to the whole repo or at least to the whole dependencies.yaml file. So I think you probably mean that the "backend" part would be another attribute on the dep ? --e.g.,

com.acme:
  foo:
    lang: "kotlin"
    platform: "jvm"  // platform instead of backend ? 

A mandatory platform is more verbose, could we default the platform to jvm ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. I was thinking we should default platform to JVM for now unless you think it boxes us in too much.

I think there is a question of: do we expect to support multiple backends in the same build? Does kotlin have a versioning scheme like scala (scala major versions are not compatible with one another, 2.11 is separate from 2.12 in maven)?

I tend to think we won’t want to do cross builds inside a single build in scala and we will instead set the one major version for the whole build. But I think we may want multiple backends to have scalajs and scala JVM inside the same build.

All that said, I really haven’t given it much thought and just would rather have kotlin work as much like java and scala in this tool for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On defaulting to jvm, if we use a short language id like kt_jvm then we don't have to worry about the boxing in part -- and it's in line with the rules.

I don't think we need to worry about Kotlin ABI versioning it is quite a bit like Java. ABI version change nightmares that occur in Scala won't exist in Kotlin. Jetbrains has backward compatibility in mind in the way they are designing the ABI contracts -- ABI metadata is communicated via annotations encoded as proto. Also, Kotlin ABI versioning isn't mangled into the deps coords and I don't think it ever will be.

Cross version builds for kotlin are configured by altering the toolchain config at the moment, and the compiler version can be changed in the workspace as it is in a seperate external workspace -- the deps shouldn't need to be changed for the cross version builds in most circumstances.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it will be pretty weird if we use scala and then introduce platforms, but have kt_jvm as the language name. Also, kt_jvm isn't really the language, is it? The language is still kotlin, no?

E.g. For scala, I can see see adding platforms such as jvm, js, native to deal with scala's use a maven for different backends. So, I could imagine:

com.foo:
  some-lib:
    lang: scala
    platforms: [js, jvm]

Keep in mind, we don't have a way to list a dependency twice. So, if we use kt_jvm we can't have that dependency for anything else, e.g. kt_js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that you put it that way I'm sold. I'll switch the language to kotlin and for now since we don't have platform it will default to "jvm" -- we can revisit the default debate later. If you want to make the change feel free, it might take me a few days since I am quite busy.

@johnynek
Copy link
Collaborator

closed in favor of #212

Happy to have more PRs to iterate.

@johnynek johnynek closed this Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants