Skip to content
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

Refactor compiler configuration repositories #900

Closed
wants to merge 10 commits into from

Conversation

comius
Copy link
Contributor

@comius comius commented Dec 22, 2022

Add capabilities.bzl to Kotlin compiler repository. Generate kt_kotlinc_options rule from it, that is filter out options that are not available in a specific version.

This change makes a narrow interface between Kotlin compiler and Kotlin rules. It will make it possible to release and use new versions of Kotlin compiler independently from Kotlin rules.

The release of Kotlin compiler should include the new capabilities.bzl file.

The capabilities.bzl files were generated by grepping respective version of Kotlin compiler sources for @Argument annotation (some of them are not documented). The legacy file was generated from Kotlin compiler 1.3.0 additionally removing options that weren't available in src/legacy.

There are some minor differences from previous src/legacy - javac opts are the same for versions.

Works toward: #660

@comius comius changed the title Refactor rkt repostiories Refactor compiler configuration repositories Dec 22, 2022
@comius comius marked this pull request as ready for review December 22, 2022 11:25
This was referenced Dec 22, 2022
@restingbull restingbull self-assigned this Dec 22, 2022
@restingbull
Copy link
Collaborator

This makes multiple kotlin version interactions more difficult, if not impossible. This is a common usecase for burgeoning mono-repos. It's come up with java numerous times, and I've seen it a few other places.

The repository approach was chosen to allow providers (and common code) to live in the core repo, while versioned code can reference that and reuse as necessary. Flattening it means that only a single version of kotlin can be accessible at a time.

That said, the capabilities work is much better than the opts kludge.

* Convert strings to label with the correct repository.

* Convert label to string before passing to register_toolchains

* Add labels to select

* Stringify Labels to support older bazels
@restingbull
Copy link
Collaborator

I'm guessing bzlmod hates the idea of multiple-repos in a module. Of course, much of this was to work around the lack of bzlmod... erg.

@comius
Copy link
Contributor Author

comius commented Dec 23, 2022

bzlmod controls versions "transitively". In the graph of repository/module dependencies you can have several modules depend on Kotlin rules and maybe even Kotlin compiler. bzlmod will select a single version (of rules and compiler) for all of them. Top level module can choose to additional control/override the versions, but not versions that specific modules use.

I'm not sure how would multiple versions of Kotlin play together. Everything is fine until Kotlin rules don't interact / depend on each-other. If any interaction happens, let's say two Kotlin libraries depend on each other, but have a different version of Kotlin rules. The problem would be 2 different versions of providers. Starlark would say that those are not the same provider and prevent those dependencies to be made.

The interaction might be a bit better using 2 different versions of a compiler, providing they use the same conventions. But in general I think it's a bad idea to use 2 versions, either with bzlmod or in a monorepo.

In principle you can't ask bzlmod from Kotlin rules, what version of the compiler you've got and then create from it a second helper repository/extension. capabilities.bzl are close to this though. You could also put version.bzl into the compiler, that would allow you to ask.

Another thing that can happen now is, that the interface for kt_javac_options can change, because the version of the compiler changed. It might be better to keep the attributes fixed and only report a warning if something is unrecognized by the compiler.

@comius
Copy link
Contributor Author

comius commented Dec 23, 2022

There are two more things to mention.

With bzlmod toplevel module can set 2 different (or multiple) versions of Kotlin compiler for specific users - that's a feature envisioned to break diamond dependency problem.

And also toolchains could be used to support several versions of the compiler at the same time.

@restingbull
Copy link
Collaborator

There are two more things to mention.

With bzlmod toplevel module can set 2 different (or multiple) versions of Kotlin compiler for specific users - that's a feature envisioned to break diamond dependency problem.

And also toolchains could be used to support several versions of the compiler at the same time.

I had a few minutes to think this over while sitting in SLC airport.... (oi.)

This approach is fine (actually, it's substantially better), with a few tweaks. The bzlmod will actually make multi-version easier, but we don't need to worry about that at this exact moment (the rough sketch is that each legacy version can be an extension).

I have a few notes on the layout...

  • I've been moving the starlark implementation out of kotlin and into src/main/starlark. The kotlin directory should be considered the public API, while the src/main/starlark the implementation. This is long term push to improve our test coverage and make the packaging more sensible. In practice, let's not move the options implementation to the kotlin directory.
  • I loathe to give up the rkt_* directories, because it was nice to have a strict separation. However, keeping them seems more trouble that it's worth for the capabilities files. Versioning is hard, let's go shopping, etc, etc.
  • [nice to have] Can you add a script to scripts that pulls the flags from a kotlinc jar? If not, I think I know how to handle it.

@@ -1,5 +1,19 @@
load("@dev_io_bazel_rules_kotlin//src/main/starlark/core/options:derive.bzl", "derive")
load("@dev_io_bazel_rules_kotlin//src/main/starlark/core/options:convert.bzl", "convert")
# Copyright 2022 The Bazel Authors. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move to src/main/starlark and import from there.

@@ -0,0 +1,125 @@
# Copyright 2022 The Bazel Authors. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move these to a capabilities directory? It's getting crowded in here.

restingbull added a commit that referenced this pull request Apr 21, 2023
This simplifies the option resolution for head.

Multiple kotlin toolchains can be more simply handled via bzlmod.
restingbull added a commit that referenced this pull request Apr 21, 2023
This simplifies the option resolution for head.

Multiple kotlin toolchains can be more simply handled via bzlmod.
restingbull added a commit that referenced this pull request Apr 23, 2023
This simplifies the option resolution for head.

Multiple kotlin toolchains can be more simply handled via bzlmod.
@chrislovecnm
Copy link

Any update on this PR?

@restingbull
Copy link
Collaborator

Implemented in #963

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.

4 participants