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.

Sign up for GitHub

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 experimental treatInternalAsPrivate #1248

Merged
merged 7 commits into from
Jan 16, 2025
Merged

Add support for experimental treatInternalAsPrivate #1248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e03f25c
Add support for experimental treatInternalAsPrivate
rbeazleyspot Dec 18, 2024
f01ebc3
Fix ktlint violations
rbeazleyspot Dec 18, 2024
31ce64b
Add experimental_use_public_only_abi_jars docs
rbeazleyspot Dec 19, 2024
7881d02
Add option to tag rule as kt_public_only_in_abi_plugin_incompatible
rbeazleyspot Dec 19, 2024
48db5df
Regenerate docs
rbeazleyspot Dec 19, 2024
d3736d5
Split treatInternalAsPrivate and removePrivateClasses flags
rbeazleyspot Dec 23, 2024
cccb679
Add failure message
rbeazleyspot Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions CompileAvoidance.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,29 @@ kt_jvm_library(
tags = ["kt_abi_plugin_incompatible"],
)
```

Depending on your compiler version you may find non-public symbols in your ABI jar such as [internal declarations](https://youtrack.jetbrains.com/issue/KT-65690) or [effectively private classes](https://youtrack.jetbrains.com/issue/KT-64590).
Exclusion of these symbols from the ABI jar can be enabled through the `experimental_treat_internal_as_private_in_abi_jars` and `experimental_remove_private_classes_in_abi_jars` flags in addition to `experimental_use_abi_jars` flag when defining the toolchain as follows

```python
load("//kotlin:core.bzl", "define_kt_toolchain")


define_kt_toolchain(
name = "kotlin_toolchain",
experimental_use_abi_jars = True,
experimental_treat_internal_as_private_in_abi_jars = True,
experimental_remove_private_classes_in_abi_jars = True,
)
```

If you encounter bugs in older compiler versions such as [KT-71525](https://youtrack.jetbrains.com/issue/KT-71525) then ABI generation with only public symbols can be disabled on a per target basis by setting the following tags

```python
load("//kotlin:jvm.bzl", "kt_jvm_library")

kt_jvm_library(
name = "framework",
tags = ["kt_treat_internal_as_private_in_abi_plugin_incompatible", "kt_remove_private_classes_in_abi_plugin_incompatible"],
)
```
12 changes: 8 additions & 4 deletions docs/kotlin.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,10 +508,12 @@ This allows setting options and dependencies independently from the initial plug
load("@rules_kotlin//kotlin:core.bzl", "define_kt_toolchain")

define_kt_toolchain(<a href="#define_kt_toolchain-name">name</a>, <a href="#define_kt_toolchain-language_version">language_version</a>, <a href="#define_kt_toolchain-api_version">api_version</a>, <a href="#define_kt_toolchain-jvm_target">jvm_target</a>, <a href="#define_kt_toolchain-experimental_use_abi_jars">experimental_use_abi_jars</a>,
<a href="#define_kt_toolchain-experimental_strict_kotlin_deps">experimental_strict_kotlin_deps</a>, <a href="#define_kt_toolchain-experimental_report_unused_deps">experimental_report_unused_deps</a>,
<a href="#define_kt_toolchain-experimental_reduce_classpath_mode">experimental_reduce_classpath_mode</a>, <a href="#define_kt_toolchain-experimental_multiplex_workers">experimental_multiplex_workers</a>, <a href="#define_kt_toolchain-javac_options">javac_options</a>,
<a href="#define_kt_toolchain-kotlinc_options">kotlinc_options</a>, <a href="#define_kt_toolchain-jvm_stdlibs">jvm_stdlibs</a>, <a href="#define_kt_toolchain-jvm_runtime">jvm_runtime</a>, <a href="#define_kt_toolchain-jacocorunner">jacocorunner</a>, <a href="#define_kt_toolchain-exec_compatible_with">exec_compatible_with</a>,
<a href="#define_kt_toolchain-target_compatible_with">target_compatible_with</a>, <a href="#define_kt_toolchain-target_settings">target_settings</a>)
<a href="#define_kt_toolchain-experimental_treat_internal_as_private_in_abi_jars">experimental_treat_internal_as_private_in_abi_jars</a>,
<a href="#define_kt_toolchain-experimental_remove_private_classes_in_abi_jars">experimental_remove_private_classes_in_abi_jars</a>, <a href="#define_kt_toolchain-experimental_strict_kotlin_deps">experimental_strict_kotlin_deps</a>,
<a href="#define_kt_toolchain-experimental_report_unused_deps">experimental_report_unused_deps</a>, <a href="#define_kt_toolchain-experimental_reduce_classpath_mode">experimental_reduce_classpath_mode</a>,
<a href="#define_kt_toolchain-experimental_multiplex_workers">experimental_multiplex_workers</a>, <a href="#define_kt_toolchain-javac_options">javac_options</a>, <a href="#define_kt_toolchain-kotlinc_options">kotlinc_options</a>, <a href="#define_kt_toolchain-jvm_stdlibs">jvm_stdlibs</a>,
<a href="#define_kt_toolchain-jvm_runtime">jvm_runtime</a>, <a href="#define_kt_toolchain-jacocorunner">jacocorunner</a>, <a href="#define_kt_toolchain-exec_compatible_with">exec_compatible_with</a>, <a href="#define_kt_toolchain-target_compatible_with">target_compatible_with</a>,
<a href="#define_kt_toolchain-target_settings">target_settings</a>)
</pre>

Define the Kotlin toolchain.
Expand All @@ -526,6 +528,8 @@ Define the Kotlin toolchain.
| <a id="define_kt_toolchain-api_version"></a>api_version | <p align="center"> - </p> | `None` |
| <a id="define_kt_toolchain-jvm_target"></a>jvm_target | <p align="center"> - </p> | `None` |
| <a id="define_kt_toolchain-experimental_use_abi_jars"></a>experimental_use_abi_jars | <p align="center"> - </p> | `False` |
| <a id="define_kt_toolchain-experimental_treat_internal_as_private_in_abi_jars"></a>experimental_treat_internal_as_private_in_abi_jars | <p align="center"> - </p> | `False` |
| <a id="define_kt_toolchain-experimental_remove_private_classes_in_abi_jars"></a>experimental_remove_private_classes_in_abi_jars | <p align="center"> - </p> | `False` |
| <a id="define_kt_toolchain-experimental_strict_kotlin_deps"></a>experimental_strict_kotlin_deps | <p align="center"> - </p> | `None` |
| <a id="define_kt_toolchain-experimental_report_unused_deps"></a>experimental_report_unused_deps | <p align="center"> - </p> | `None` |
| <a id="define_kt_toolchain-experimental_reduce_classpath_mode"></a>experimental_reduce_classpath_mode | <p align="center"> - </p> | `None` |
Expand Down
14 changes: 14 additions & 0 deletions kotlin/internal/jvm/compile.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,20 @@ def _run_kt_builder_action(
omit_if_empty = True,
)

if not "kt_remove_private_classes_in_abi_plugin_incompatible" in ctx.attr.tags and toolchains.kt.experimental_remove_private_classes_in_abi_jars == True:
args.add("--remove_private_classes_in_abi_jar", "true")

if not "kt_treat_internal_as_private_in_abi_plugin_incompatible" in ctx.attr.tags and toolchains.kt.experimental_treat_internal_as_private_in_abi_jars == True:
if not "kt_remove_private_classes_in_abi_plugin_incompatible" in ctx.attr.tags and toolchains.kt.experimental_remove_private_classes_in_abi_jars == True:
args.add("--treat_internal_as_private_in_abi_jar", "true")
else:
fail(
"experimental_treat_internal_as_private_in_abi_jars without experimental_remove_private_classes_in_abi_jars is invalid." +
"\nTo remove internal symbols from kotlin abi jars ensure experimental_remove_private_classes_in_abi_jars " +
"and experimental_treat_internal_as_private_in_abi_jars are both enabled in define_kt_toolchain." +
"\nAdditionally ensure the target does not contain the kt_remove_private_classes_in_abi_plugin_incompatible tag.",
)

args.add("--build_kotlin", build_kotlin)

progress_message = "%s %%{label} { kt: %d, java: %d, srcjars: %d } for %s" % (
Expand Down
20 changes: 20 additions & 0 deletions kotlin/internal/toolchains.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ def _kotlin_toolchain_impl(ctx):
"supports-multiplex-workers": "1" if ctx.attr.experimental_multiplex_workers else "0",
},
experimental_use_abi_jars = ctx.attr.experimental_use_abi_jars,
experimental_treat_internal_as_private_in_abi_jars = ctx.attr.experimental_treat_internal_as_private_in_abi_jars,
experimental_remove_private_classes_in_abi_jars = ctx.attr.experimental_remove_private_classes_in_abi_jars,
experimental_strict_kotlin_deps = ctx.attr.experimental_strict_kotlin_deps,
experimental_report_unused_deps = ctx.attr.experimental_report_unused_deps,
experimental_reduce_classpath_mode = ctx.attr.experimental_reduce_classpath_mode,
Expand Down Expand Up @@ -202,6 +204,20 @@ _kt_toolchain = rule(
`kt_abi_plugin_incompatible`""",
default = False,
),
"experimental_treat_internal_as_private_in_abi_jars": attr.bool(
doc = """This applies the following compiler plugin option:
plugin:org.jetbrains.kotlin.jvm.abi:treatInternalAsPrivate=true
Copy link
Collaborator

@Bencodes Bencodes Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do these flags need to be paired together to work? If not splitting these up into two individual flags could help further ease the migration over to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in my (limited) experience yes, they only appear to work together... however i will split them, maybe there are variations in compiler versions....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe it would be better if we pivoted on this idea; if we keep down this road there will be a parallel release train with the kotlin compiler. Each option added for the abi plugin will be mirrored in a similar commit/release here. Could we instead have some experimental_abi_plugin_flags dictionary or similar that is unpacked when configuring the plugin in KotlinJvmTaskExecutor ?

On the other hand i quite like the idea of restricting what options are available such that these rules can be opinionated about how to build abi jars and increase compilation avoidance :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it looks like this conditional means both flags are needed as its gated behind removePrivateClasses...

private fun shouldRemoveFromAbi(irClass: IrClass?, removePrivateClasses: Boolean, treatInternalAsPrivate: Boolean): Boolean = when {
    irClass == null -> false
    irClass.isFileClass -> false
    removePrivateClasses -> irClass.isVisibilityStrippedFromAbi(stripInternal = treatInternalAsPrivate)
    else -> false
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

anyway, i've pushed another commit that splits these flags... its no effort to revert that commit if we so wish

Can be disabled for an individual target using the tag.
`kt_treat_internal_as_private_in_abi_plugin_incompatible`""",
default = False,
),
"experimental_remove_private_classes_in_abi_jars": attr.bool(
doc = """This applies the following compiler plugin option:
plugin:org.jetbrains.kotlin.jvm.abi:removePrivateClasses=true
Can be disabled for an individual target using the tag.
`kt_remove_private_classes_in_abi_plugin_incompatible`""",
default = False,
),
"experimental_strict_kotlin_deps": attr.string(
doc = "Report strict deps violations",
default = "off",
Expand Down Expand Up @@ -288,6 +304,8 @@ def define_kt_toolchain(
api_version = None,
jvm_target = None,
experimental_use_abi_jars = False,
experimental_treat_internal_as_private_in_abi_jars = False,
experimental_remove_private_classes_in_abi_jars = False,
experimental_strict_kotlin_deps = None,
experimental_report_unused_deps = None,
experimental_reduce_classpath_mode = None,
Expand All @@ -314,6 +332,8 @@ def define_kt_toolchain(
_NOEXPERIMENTAL_USE_ABI_JARS: False,
"//conditions:default": experimental_use_abi_jars,
}),
experimental_treat_internal_as_private_in_abi_jars = experimental_treat_internal_as_private_in_abi_jars,
experimental_remove_private_classes_in_abi_jars = experimental_remove_private_classes_in_abi_jars,
experimental_multiplex_workers = experimental_multiplex_workers,
experimental_strict_kotlin_deps = experimental_strict_kotlin_deps,
experimental_report_unused_deps = experimental_report_unused_deps,
Expand Down
8 changes: 8 additions & 0 deletions src/main/kotlin/io/bazel/kotlin/builder/tasks/KotlinBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ class KotlinBuilder
DEBUG("--kotlin_debug_tags"),
TASK_ID("--kotlin_task_id"),
ABI_JAR("--abi_jar"),
ABI_JAR_INTERNAL_AS_PRIVATE("--treat_internal_as_private_in_abi_jar"),
ABI_JAR_REMOVE_PRIVATE_CLASSES("--remove_private_classes_in_abi_jar"),
GENERATED_JAVA_SRC_JAR("--generated_java_srcjar"),
GENERATED_JAVA_STUB_JAR("--kapt_generated_stub_jar"),
GENERATED_CLASS_JAR("--kapt_generated_class_jar"),
Expand Down Expand Up @@ -161,6 +163,12 @@ class KotlinBuilder
argMap.mandatorySingle(KotlinBuilderFlags.LANGUAGE_VERSION)
strictKotlinDeps = argMap.mandatorySingle(KotlinBuilderFlags.STRICT_KOTLIN_DEPS)
reducedClasspathMode = argMap.mandatorySingle(KotlinBuilderFlags.REDUCED_CLASSPATH_MODE)
argMap.optionalSingle(KotlinBuilderFlags.ABI_JAR_INTERNAL_AS_PRIVATE)?.let {
treatInternalAsPrivateInAbiJar = it == "true"
}
argMap.optionalSingle(KotlinBuilderFlags.ABI_JAR_REMOVE_PRIVATE_CLASSES)?.let {
removePrivateClassesInAbiJar = it == "true"
}
this
}

Expand Down
6 changes: 6 additions & 0 deletions src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/KotlinJvmTaskExecutor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,12 @@ class KotlinJvmTaskExecutor
.notEmpty {
plugin(plugins.jvmAbiGen) {
flag("outputDir", directories.abiClasses)
if (info.treatInternalAsPrivateInAbiJar) {
flag("treatInternalAsPrivate", "true")
}
if (info.removePrivateClassesInAbiJar) {
flag("removePrivateClasses", "true")
}
}
given(outputs.jar).empty {
plugin(plugins.skipCodeGen)
Expand Down
4 changes: 4 additions & 0 deletions src/main/protobuf/kotlin_model.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ message CompilationTaskInfo {
string strict_kotlin_deps = 10;
// Optimize classpath by removing dependencies not required for compilation
string reduced_classpath_mode = 11;
// Internal declarations are treaded as private for abi.jar generation
bool treat_internal_as_private_in_abi_jar = 12;
// Private classes are removed in abi.jar generation
bool remove_private_classes_in_abi_jar = 13;
}

// Nested messages not marked with stable could be refactored.
Expand Down
5 changes: 5 additions & 0 deletions src/test/kotlin/io/bazel/kotlin/builder/KotlinJvmTestBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,11 @@ public TaskBuilder outputAbiJar() {
return this;
}

public TaskBuilder publicOnlyAbiJar() {
taskBuilder.getInfoBuilder().setTreatInternalAsPrivateInAbiJar(true).setRemovePrivateClassesInAbiJar(true);
return this;
}

public TaskBuilder generatedSourceJar() {
taskBuilder.getOutputsBuilder()
.setGeneratedJavaSrcJar(instanceRoot().resolve("gen-src.jar").toAbsolutePath().toString());
Expand Down
67 changes: 67 additions & 0 deletions src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/KotlinBuilderJvmAbiTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,15 @@
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

import java.io.IOException;
import java.util.zip.ZipEntry;
import java.util.zip.ZipFile;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.Is.is;
import static org.hamcrest.core.IsNot.not;
import static org.hamcrest.core.IsNull.nullValue;

@RunWith(JUnit4.class)
public class KotlinBuilderJvmAbiTest {
private static final KotlinJvmTestBuilder ctx = new KotlinJvmTestBuilder();
Expand Down Expand Up @@ -60,4 +69,62 @@ public void testGeneratesAbiJarSource() {
c.compileKotlin();
});
}

@Test
public void testGeneratesPublicAbiOnly() throws IOException {
Deps.Dep d = ctx.runCompileTask(
c -> {
c.addSource("AClass.kt", "package something;" + "class AClass{}");
c.addSource("AnotherClass.kt", "package something;", "", "class AnotherClass{}");
c.addSource("NonPublicClass.kt", "package something;", "", "internal class NonPublicClass{}");
c.outputJar();
c.outputAbiJar();
c.publicOnlyAbiJar();
c.compileKotlin();
c.outputJdeps();
});

String abiJarPath = d.compileJars()
.stream()
.filter( (name)->name.endsWith("abi.jar") )
.findFirst()
.orElse(null);

assertThat(abiJarPath, is(not(nullValue())));

ZipEntry entry = null;
try( ZipFile zipFile = new ZipFile(abiJarPath) ) {
entry = zipFile.getEntry("something/NonPublicClass.class");
}
assertThat(entry, is(nullValue()));
}


@Test
public void testGeneratesAbiIncludingInternal() throws IOException {
Deps.Dep d = ctx.runCompileTask(
c -> {
c.addSource("AClass.kt", "package something;" + "class AClass{}");
c.addSource("AnotherClass.kt", "package something;", "", "class AnotherClass{}");
c.addSource("NonPublicClass.kt", "package something;", "", "internal class NonPublicClass{}");
c.outputJar();
c.outputAbiJar();
c.compileKotlin();
c.outputJdeps();
});

String abiJarPath = d.compileJars()
.stream()
.filter( (name)->name.endsWith("abi.jar") )
.findFirst()
.orElse(null);

assertThat(abiJarPath, is(not(nullValue())));

ZipEntry entry = null;
try( ZipFile zipFile = new ZipFile(abiJarPath) ) {
entry = zipFile.getEntry("something/NonPublicClass.class");
}
assertThat(entry, is(not(nullValue())));
}
}