Add support for experimental treatInternalAsPrivate

[rbeazleyspot]

Adds support for experimental options when producing abi.jar, specifically KT-65690 and KT-64590 as discussed in #1245

Have added a couple of integration tests to perform basic checks of flags being applied

I expect there is more to do here and happy to work on it if its considered for merging

[Bencodes]

This should probably have a tag of its own to disable this behavior at the target level so that adoption across larger projects is easier.

[Bencodes]

@rbeazleyspot PR looks good to me, can you take a look at the failing CI checks and regenerate the docs?

[rbeazleyspot]

@rbeazleyspot PR looks good to me, can you take a look at the failing CI checks and regenerate the docs?

will do, i'm going to make the change you suggested as integrating into our codebase showed up a couple of targets with interesting (TM) issues

[rbeazleyspot]

Ive pushed an update to the rule to use its own tag as suggested, regenerated the docs (there were some reformatting changes that i unstaged, not sure if that was expected or not), and updated the compilation avoidance advice.

Finally Ive since been testing this further in our app and finding i need to apply kt_public_only_in_abi_plugin_incompatible to some associate targets, so im a little concerned here.

I wondered if something in associates.bzl needs to change, but I havnt looked into this at all yet. It could be our macros are breaking things a little but the effect is certainly that internal access in an associate is not possible ie its linking against the abi jar (first?) or something.

I'll continue to investigate this.

And finally finally, due to the bug mentioned in the compilation avoidance doc i've been testing this locally with compiler 2.1 (after a few tweaks to the rules codebase)... so just a heads up 🖖

[rbeazleyspot]

oh, finally finally v3 final..... i'm not sure whats going on with the CI failure, any pointers?

and FWIW this command passes locally on my machine

bazel test --cache_test_results=no -- //src/test/kotlin/io/bazel/kotlin/builder:builder_tests //src/test/kotlin/io/bazel/kotlin:KotlinJvmAssociatesBasicVisibilityTest //src/test/kotlin/io/bazel/kotlin:KotlinJvmBasicAssertionTest

[rbeazleyspot]

Update on associates

KotlinJvmAssociatesBasicVisibilityTest fails when i enable these experimental flags, so something is certainly afoot... ill continue to investigate

[rbeazleyspot]

So i thought the issue would be in associates.bzl where it pulls the friendly jars from the associated target and changed this to use the output jar rather than the module jar ... alas this didnt work

I was able to validate (what i thought was) the correct jar [bazel-out/darwin_arm64-fastbuild/bin/src/test/data/jvm/basic/test_associates_library.jar] was passed over as the value for --kotlin_friend_paths

so im rather stuck atm

Update:

I have something that works by pre-pending the associate targets class_jar to the compile_jars depset produced by the _jvm_deps macro... something along these lines

    associated_java_infos = [_java_info(d) for d in associated_targets]
    associated_output_jars = [info.outputs.jars for info in associated_java_infos]
    associated_targets_depset = depset([
        s[0].class_jar
        for s in associated_output_jars
    ])

and then added to the start of the transitive list

transitive = [
            associated_targets_depset,
        ] + [
            d.compile_jars
            for d in dep_infos
        ] + [
            d.transitive_compile_time_jars
            for d in dep_infos
        ]

if you think this is vaguly going down the right route I can rework this so its not using deprecated methods

[rbeazleyspot]

It occurred to me last night that the problem with associates this change has shown is maybe an issue in its own right.... should I open one for this @Bencodes ? ie compilation with associates use compile_jars rather than output jars

I guess this only worked as at the time there were no ABI jars generated so it defaulted to output jars?
Then when the ABI jars were generated they were (incorrectly) including more than just public api so things still just worked ™️

[Bencodes]

@rbeazleyspot can you rebase this branch against HEAD, I believe the current failing REB CI job has already been fixed.

[Bencodes]

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.

[rbeazleyspot]

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

[rbeazleyspot]

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

[rbeazleyspot]

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
}

[rbeazleyspot]

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

[Bencodes]

It occurred to me last night that the problem with associates this change has shown is maybe an issue in its own right....

A new issue would be good for this just to track it. There are a few known edge cases with associates, some of which you've called out here. The ones that I'm aware of are:

1. Associate dep collection brings the entire classpath along with each associate target. This unintentionally leaks some transitive classpath internals beyond the direct associate dependency. I filed an issue for this one a while back Associates or friends leak the internals of their compile classpath #1021
2. As you mentioned associates uses the compile jar instead of the output/runtime jar. This one is worth fixing inside the master branch.
3. There are some additional edge cases that will need to be looked at around ensuring that if we collect an output jar for an associate dependency, it's ABI doesn't make it into the list of standard classpath jars. It's been a while since i've looked into this, but I remember finding that if you pass both the ABI and runtime jars to kotlinc you get into some weird territory where one may get picked over the other, produced an unexpected error (maybe this has improved in 2.x).

I hacked together a not-so-amazing solution to all of these issues a while back since we had been testing out these abi-gen-compiler changes internally to see what kind incremental compilation improvements we could expect here. To do this I had to chop up associates.bzl quite heavily so that I could build a relationship between the compile and output jars up front, and then swap jars in and out accordingly when constructing that final classpath for the KotlinCompile action.

[rbeazleyspot]

It occurred to me last night that the problem with associates this change has shown is maybe an issue in its own right....

A new issue would be good for this just to track it. There are a few known edge cases with associates, some of which you've called out here. The ones that I'm aware of are:

#1250