-
Notifications
You must be signed in to change notification settings - Fork 232
Add initial support for Kotlin Native toolchain and klib compilation #1351
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
base: master
Are you sure you want to change the base?
Add initial support for Kotlin Native toolchain and klib compilation #1351
Conversation
@@ -19,6 +19,8 @@ bazel_dep(name = "rules_shell", version = "0.4.1") | |||
|
|||
bazel_dep(name = "buildifier_prebuilt", version = "8.0.3", dev_dependency = True) | |||
|
|||
bazel_dep(name = "aspect_bazel_lib", version = "2.19.4") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this to leverage copy_to_directory
to expose the Kotlin native compiler distribution as a directory in the toolchain here (as it's massive and has a ton of files)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use copy_to_directory
it's basically antagonistic to RBE.
copy_to_directory uses uses a tree artifact (via declare_directory which is, under the hood, a tarball.) It doesn't save time or number of files. Why? Because bazel only works with files, rather than directories. The intent of that feature is handle actions where the outputs are unknown before execution. copy_to_directory
does know the inputs (all the files in the glob), and then applies filters etc to the them via an action... Which means we take a well known set of inputs, execute an action, place them in a tarball, and then unpack them everytime we need them (give or take -- there are some optimizations under the hood.)
For RBE, this is ugly: first, transfer all the files into a remote action; second, tar all the outputs and place them in CAS. Given that the files have not changed, this bloats the CAS size, increases transfer costs, adds additional actions... for no good reason, other than laziness on the rule writers part. After all, copy_to_directory
could be written in starlark to do the filtering... or the rules could handle the filtering during globbing, or...
You get the idea.
), | ||
] | ||
|
||
kt_klib_library = rule( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the new kt_klib_library
rule that produces klibs using the kotlin/native toolchain. Potentially we could just have it as action and have an attr on the existing rules to trigger klib compilation on them as well (I'm not sure if that's better - let me know)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kt_library
fits the naming convention: kt_js_library
, kt_jvm_library
.
kotlin seems to be moving towards using klib as the core intermediate format (js and native), so I suspect java may follow at some point.
// within the current working directory to isolate its cache (which is unique to this worker anyway) | ||
// Ideally we disable caching though and rely only on Bazel | ||
add("-Xauto-cache-dir=${autoCacheDirectory.absolutePathString()}") | ||
add("-Xauto-cache-from=${autoCacheFromDirectory.absolutePathString()}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this, the compiler tries to write within the cache in the external repository where konan.home
is and bazel errors out. Ideally we disable caching but there doesn't seem to be a flag for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've got a different effort looking at incremental caching. This is good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is odd, I've never had this problem.
I think I avoided it because instead of it being implicit, I pass -nostdlib
(or was it -no-default-libs
that does this? I pass both) and all the standard library stuff is passed as -library <path>
.
I see 2 advantages to do it this way:
- it allows to "lift" the cache generation into bazel as separate actions, and the resulting artifacts can be cached by bazel (including remotely instead of just being local)
- cross compilation is much easier for not officially supported host-target combinations (like building macos artifacts on linux). I've yet to test this but I think it should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is odd, I've never had this problem. I think I avoided it because instead of it being implicit, I pass
-nostdlib
(or was it-no-default-libs
that does this? I pass both) and all the standard library stuff is passed as-library <path>
. I see 2 advantages to do it this way:
- it allows to "lift" the cache generation into bazel as separate actions, and the resulting artifacts can be cached by bazel (including remotely instead of just being local)
- cross compilation is much easier for not officially supported host-target combinations (like building macos artifacts on linux). I've yet to test this but I think it should work.
Interesting, I'll double check again. If I remember not setting this led to the klib/cache
folder under the external konan home repository being used as the cached and Bazel errors out as it breaks the sandboxing rules (access denied basically). I'll check if it is an issue (or not) with -nostdlib
.
cross compilation is much easier for not officially supported host-target combinations (like building macos artifacts on linux). I've yet to test this but I think it should work.
Interesting, that would make sense - I haven't tested with host-target platforms being different but I'll check and follow-up what my observations are.
val konanHome = System.getenv("KONAN_HOME") | ||
requireNotNull(konanHome) {"KONAN_HOME env var must be set!"} | ||
|
||
System.setProperty("konan.home", konanHome) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed for the compiler to find the native code/distribution to produce klib.
@@ -0,0 +1,98 @@ | |||
load("//src/main/starlark/core/repositories/kotlin:templates.bzl", "TEMPLATES") | |||
|
|||
def _kotlin_capabilities_impl(repository_ctx): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was copied into a separate file to allow leveraging it with kotlin_native_compiler_repository
out = "klib/cache/macos_arm64STATIC/klib_cache_marker", | ||
content = [ | ||
"Marker file intended to create the klib system cache placeholder even if we don't use it. The native compiler errors out otherwise", | ||
"See See https://github.com/JetBrains/kotlin/blob/v2.1.21/kotlin-native/backend.native/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/KonanConfig.kt#L567" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compiler on bootstrap tries to create klib/cache/<platform>STATIC
directory in the external repo and this is non-hermetic and causes Bazel to fail. To workaround it (unfortunately there doesn't appear to be a way to disable it with a flag), so we create this marker/dummy file to avoid running into the issue (happy to consider other solutions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start. Needs some TLC around packaging konan home and configuring toolchians properly.
@@ -19,6 +19,8 @@ bazel_dep(name = "rules_shell", version = "0.4.1") | |||
|
|||
bazel_dep(name = "buildifier_prebuilt", version = "8.0.3", dev_dependency = True) | |||
|
|||
bazel_dep(name = "aspect_bazel_lib", version = "2.19.4") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use copy_to_directory
it's basically antagonistic to RBE.
copy_to_directory uses uses a tree artifact (via declare_directory which is, under the hood, a tarball.) It doesn't save time or number of files. Why? Because bazel only works with files, rather than directories. The intent of that feature is handle actions where the outputs are unknown before execution. copy_to_directory
does know the inputs (all the files in the glob), and then applies filters etc to the them via an action... Which means we take a well known set of inputs, execute an action, place them in a tarball, and then unpack them everytime we need them (give or take -- there are some optimizations under the hood.)
For RBE, this is ugly: first, transfer all the files into a remote action; second, tar all the outputs and place them in CAS. Given that the files have not changed, this bloats the CAS size, increases transfer costs, adds additional actions... for no good reason, other than laziness on the rule writers part. After all, copy_to_directory
could be written in starlark to do the filtering... or the rules could handle the filtering during globbing, or...
You get the idea.
), | ||
] | ||
|
||
kt_klib_library = rule( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kt_library
fits the naming convention: kt_js_library
, kt_jvm_library
.
kotlin seems to be moving towards using klib as the core intermediate format (js and native), so I suspect java may follow at some point.
vararg args: String, | ||
): ExitCode { | ||
System.setProperty("zip.handler.uses.crc.instead.of.timestamp", "true") | ||
val konanHome = System.getenv("KONAN_HOME") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this provided as an environment variable, rather that passed in as an argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should be a flag for consistency but I didn't want to intercept the arguments passed to the compiler and then set it. I can update it though (would be nice if this could be passed as a flag to the compiler but there doesn't seem to be from what I can tell)
_import_artifacts(KOTLIN_NATIVE_ARTIFACTS.windows_x86_64, kt_jvm_import, compiler_repo = _KT_NATIVE_COMPILER_REPO_PREFIX + "_windows_x86_64") | ||
|
||
# a convenience alias for kotlin-native to be referenced in other places | ||
native.alias( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the usecase for this? It's far preferable to use specific toolchains.
builder_args.add("--output_klib", klib.path) | ||
|
||
deps_klibs = [] | ||
for dep in ctx.attr.deps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicated from line #13?
// within the current working directory to isolate its cache (which is unique to this worker anyway) | ||
// Ideally we disable caching though and rely only on Bazel | ||
add("-Xauto-cache-dir=${autoCacheDirectory.absolutePathString()}") | ||
add("-Xauto-cache-from=${autoCacheFromDirectory.absolutePathString()}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've got a different effort looking at incremental caching. This is good enough.
], | ||
) | ||
|
||
copy_to_directory( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted above, don't do this.
In fact, this is a prefect place to just use a filegroup. We don't need to move these files, just know where they are... enough to set KONAN_HOME.
"konan/**", | ||
"klib/**", | ||
]) + [ | ||
":klib_system_cache_marker_linux_x64", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be in all platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created it for all since Konan sets it up for the target platform and we don't know in advance the target platform a user selects in the repository rule https://github.com/JetBrains/kotlin/blob/v2.1.21/kotlin-native/backend.native/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/KonanConfig.kt#L532 Probably could be a better way to do it though.
load("//kotlin/internal:defs.bzl", "KtKlibInfo") | ||
load("//kotlin/internal/klib:klib.bzl", "kt_klib_library") | ||
|
||
def _common_assertions(env, target): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test for library -> library to ensure that transitive klibs are included for compilation.
Add test for data to ensure runfiles are included.
@@ -379,6 +385,12 @@ def define_kt_toolchain( | |||
jvm_runtime = jvm_runtime if jvm_runtime != None else [ | |||
Label("//kotlin/compiler:kotlin-stdlib"), | |||
], | |||
konan_home = select({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each of these should be a separate toolchain with different platform constraints.
If nothing else, it will make debugging toolchain issues sane.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to put it into its own toolchain.
// Ideally we disable caching though and rely only on Bazel | ||
add("-Xauto-cache-dir=${autoCacheDirectory.absolutePathString()}") | ||
add("-Xauto-cache-from=${autoCacheFromDirectory.absolutePathString()}") | ||
add("-Xklib-normalize-absolute-path") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
normalize-absolute-path will only fix the slashes.
In order to support rbe and remote caching, we'll need to do something intelligent with relative paths in klibs](https://kotlinlang.org/docs/native-libraries.html#using-relative-paths-in-klibs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link only mentions -Xklib-relative-path-base
, but -Xdebug-prefix-map
is also required, otherwise the debug info will have absolute paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pointers, I will update them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As somebody who has written bazel rules for Kotlin Native, I hope you don't mind a few comments.
KOTLIN_NATIVE_CURRENT_RELEASE_LINUX_X86_64 = version( | ||
version = _DEFAULT_KOTLIN_COMPILER_RELEASE_VERSION, | ||
url_templates = [ | ||
"https://github.com/JetBrains/kotlin/releases/download/v{version}/kotlin-native-prebuilt-linux-x86_64-{version}.tar.gz", | ||
], | ||
sha256 = "42fb88529b4039b6ac1961a137ccb1c79fc80315947f3ec31b56834c7ce20d0b", | ||
strip_prefix_template = "kotlin-native-prebuilt-linux-x86_64-{version}", | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the pure klib generating mode, but this will not be enough in the long run. Kotlin Native compiler downloads some host-specific stuff (clang+llvm, libffi, lldb, sysroot on linux) on first launch at runtime which is difficult to express in bazel.
In theory, one could parse the konan.properties file and figure out the necessary artifacts but that seems like a lot of work with no guarantees of stability.
I simply include them with the specific compiler version and platform (1). There might be a better way but even rules_rust has a giant file with known artifacts and their hashes.
As a precaution against this, I also specify -Xoverride-konan-properties=airplaneMode=true
(which, annoyingly, is a flag that is not accepted by the cinterop compiler).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean - there's a lot of embedded stuff in there and I didn't put a lot of thought yet into Cinterop or other native support (I had mostly considered JVM/bS). But I do agree this would not enough especially for host/target platform not being the same. And I think the toolchain we bootstrap here should be extensible to suporot them. It'd be nice to wire up the toolchain from toolchains_llvm for this but I guess there's no way beyond parsing konan.properties and making it understand a hermetic toolchain (assuming it's possible).
I simply include them with the specific compiler version and platform (1). There might be a better way but even rules_rust has a giant file with known artifacts and their hashes.
Wow, that's a massive file. I guess that approach could be ok too if generating it could be automated.
// Ideally we disable caching though and rely only on Bazel | ||
add("-Xauto-cache-dir=${autoCacheDirectory.absolutePathString()}") | ||
add("-Xauto-cache-from=${autoCacheFromDirectory.absolutePathString()}") | ||
add("-Xklib-normalize-absolute-path") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link only mentions -Xklib-relative-path-base
, but -Xdebug-prefix-map
is also required, otherwise the debug info will have absolute paths.
// within the current working directory to isolate its cache (which is unique to this worker anyway) | ||
// Ideally we disable caching though and rely only on Bazel | ||
add("-Xauto-cache-dir=${autoCacheDirectory.absolutePathString()}") | ||
add("-Xauto-cache-from=${autoCacheFromDirectory.absolutePathString()}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is odd, I've never had this problem.
I think I avoided it because instead of it being implicit, I pass -nostdlib
(or was it -no-default-libs
that does this? I pass both) and all the standard library stuff is passed as -library <path>
.
I see 2 advantages to do it this way:
- it allows to "lift" the cache generation into bazel as separate actions, and the resulting artifacts can be cached by bazel (including remotely instead of just being local)
- cross compilation is much easier for not officially supported host-target combinations (like building macos artifacts on linux). I've yet to test this but I think it should work.
plugin = {}, | ||
compile = { | ||
"kotlin-native-linux-x86_64": "konan/lib/kotlin-native.jar", | ||
"trove4j-linux-x86_64": "konan/lib/trove4j.jar", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun fact, this file is missing in 2.2.0 version. I also had it specified explicitly, but i have since replaced it with a glob
.
In #1347 (comment), it was mentioned that it's preferred to bootstrap a Kotlin native toolchain first to leverage the new IR compiler to utilize a compilation model to output klibs in a generic way that can be leveraged for JS and potentially other platforms in the future. This PR tries to add that to the toolchain and also adds a new rule
kt_klib_library
rule which is intended to use the Kotlin-native compiler to produce klibs for platform indepedent code that can be shared between various platform specific targets (think ofcommonMain
in a gradle project) and can be in the future consumed by platform specific JS/JVM (and maybe WASM targets as well).Some notes on the implementation details:
kotlin_native_compiler_repository
to bootstrap/download the native distributions for different platforms supported along with its "capabilities" repository similar to how kotlinc is setup. I'll add some more comments inline.konan.home
or the native compiler files as a TreeArtifact/directory to avoid passing a massive number of files during compilation.produce=library
//kotlin/compiler:kotlin-native
which is supposed to alias to the native compiler jar/distribution on the execution platform.kt_klib_library
depending on another target). Add some junit test cases to assert klib has some entries we expect.A simple example shown below:
TODO: