-
Notifications
You must be signed in to change notification settings - Fork 237
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
Add support for Kotlin JARs with inlined functions, and an Android Kotlin example app #69
Conversation
5eaab01
to
9b22178
Compare
Excellent idea from @davidsantiago: perhaps we should generate, by default, |
As discussed offline, let's go with completely replacing |
@@ -382,7 +389,7 @@ def _coursier_fetch_impl(repository_ctx): | |||
cmd = _generate_coursier_command(repository_ctx) | |||
cmd.extend(["fetch"]) | |||
cmd.extend(artifact_coordinates) | |||
cmd.extend(["--artifact-type", ",".join(_COURSIER_PACKAGING_TYPES)]) | |||
cmd.extend(["--artifact-type", ",".join(_COURSIER_PACKAGING_TYPES + ["src"])]) |
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.
drive-by fix to add src
back into the fetch_sources branch.
# Then this artifact is an rdep :-) | ||
reverse_deps.append(maybe_rdep) | ||
return reverse_deps | ||
reverse_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.
drive by formatting fix.
@@ -44,4 +44,9 @@ platforms: | |||
# TODO(jin): fix long paths issue on Windows | |||
# - "//examples:app" | |||
test_targets: | |||
- "--" |
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 does the "--" do?
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.
allows using the target subtraction syntax -//:target
.bazelci/presubmit.yml
Outdated
- "//tests/unit/..." | ||
# TODO(jin): Concurrent download problems on macOS | ||
# https://github.com/bazelbuild/rules_jvm_external/issues/54 | ||
- "-//tests/unit/kotlin/..." |
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.
FYI: had to disable the test on the Mac too, due to network / race problems in #54
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.
Hmm maybe the CI can't resolve more than one maven_install repository concurrently?
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.
Yup, that's exactly it.
This PR adds support for Kotlin-built jars, tests, and an Android Kotlin example app.
I added the tests and app into the macOS and Linux CI configuration. rules_kotlin does not yet work on Windows (bazelbuild/rules_kotlin#179), so the Windows failure in the CI is expected. We exclude the Kotlin unit tests for Windows in this PR so postsubmit is expected to be green.
Kotlin-sourced jars can contain metadata of inlined functions (
inline fun ...
), and regular java_import will strip these away with ijar, causing builds to fail. See https://github.com/bazelbuild/rules_jvm_external/pull/69/files#diff-90e077b9f9533f17dcef14f92cbae44c for the error message. To get around that, we create a simpleno_ijar_java_import
rule for internal use during the BUILD generation.To detect whether a JAR is Kotlin-sourced, we run
jar tf
on jars during BUILD file generation, and check for the existence ofkotlin_module
files. If it exists, we use theno_ijar_java_import
rule class. If it does, we use the regularjava_import
, which has support for interface jars for more parallelizable downstream Java compilation.Fixes #59