Fix associates compile classpath

[rbeazleyspot]

This is a (surprisingly) small PR to resolve #1250.

Any friends mentioned were not added to the classpath so KotlinBuilder now adds them and it keeps the compilation units friends close by adding them to the head of the classpath

Any friends jars that were aded were effectively just the ABI jars so associates.bzl now uses the class jars (which contain any internal declarations)

---

I had a hard time (ie failed) writing a test for this, i can only be sure the existing mechanism works ok.

Locally i redefined the tool chain to enable the relevant experimental options

define_kt_toolchain(
        name = "default_toolchain",
        experimental_remove_private_classes_in_abi_jars = True,
        experimental_treat_internal_as_private_in_abi_jars = True,
        experimental_use_abi_jars = True,
        language_version = "2.1",
        api_version = "2.1",
    )

with these in place the basic associates test fails, applying these commits makes it pass

an integration test could be added once a release is cut maybe?

[rbeazleyspot]

well it looks like the same code fails on CI :)

so i guess ill look into this

-- edit
Still worth getting your feedback on this whilst i scratch my head

[rbeazleyspot]

rookie mistake, not of course adding the associates java_outputs as an input to the compilation action, thanks for the help debugging @gabrielborglund

[rbeazleyspot]

I've tried to work out why the integration tests are failing, no idea ! sorry :/

[rbeazleyspot]

FYI, i believe with this change in place and considering a build graph that looks like this:

A - deps=[B], associates=[C]
B - deps=[C, D]

the compilation class path will look like this

C-full-runtime.jar (Collected from friends)
B-abi.jar (plus stdlib etc)
C-abi.jar (Collected from the transitive deps)
D.abi.jar

which is a change from

B-abi.jar (plus stdlib etc)
C-abi.jar (Collected from the transitive deps)
D.abi.jar

.... its possible a different angle could be taken and when the java_info for the associate_deps is collected here in compile.bzl

then it could be massaged to drop any ABI jars ie set compile_jar to output_jar

[rbeazleyspot]

rebased onto master

[rbeazleyspot]

In the latest commit, any associated ABI jars are replaced by the full output class jar so that the associated internal methods can be resolved

[Bencodes]

This technically changes the default behavior for everyone, is it possible to only enable this when experimental_treat_internal_as_private_in_abi_jars or experimental_remove_private_classes_in_abi_jars is enabled?

[rbeazleyspot]

on the surface of it i agree with you, but isnt the reality that these jars are quite similar without those flags enabled?

im happy to do this tho, thought it might get a bit unwieldy

[rbeazleyspot]

further thoughts, im actually thinking that i should introduce a third branch instead behind the private_abi_jars related flags and leave the current code untouched where its non transient class jar.... something like

if (not "kt_remove_private_classes_in_abi_plugin_incompatible" in ctx.attr.tags and toolchains.kt.experimental_remove_private_classes_in_abi_jars):
      jars_depset = depset(direct = [a.class_jar for a in associate[JavaInfo].java_outputs])

elif (toolchains.kt.experimental_strict_associate_dependencies and
      "kt_experimental_strict_associate_dependencies_incompatible" not in ctx.attr.tags):
      jars_depset = depset(direct = [a.compile_jar for a in associate[JavaInfo].java_outputs])

else:
      jars_depset = depset(transitive = [associate[JavaInfo].compile_jars])

since this is all about compilation avoidance i would be happen just to have the strictest of case be true

[rbeazleyspot]

Ive tested this version (and pushed the code for) in our codebase, apart from a few "mistakes" where java modules add access to internal classes it worked fine. I was able to exclude these targets using the tags.

[restingbull]

Can we get a few starlark tests on the classpath order for runtime and compile time? The change looks good, but I can't quite reason about what will show up in the final resolution.

[rbeazleyspot]

Can we get a few starlark tests on the classpath order for runtime and compile time? The change looks good, but I can't quite reason about what will show up in the final resolution.

ill give it my best shot

[rbeazleyspot]

as discussed in slack, commit with a little refactoring to focus the unit under test to be smaller

also WorkerEnvironmentTest looks to be failing, and locally, however it appears to pass sometimes even with HEAD~1

[rbeazleyspot]

the above commit was verified with

bazel test --cache_test_results=no --runs_per_test=300 //src/test/kotlin/io/bazel/worker:WorkerEnvironmentTest

[restingbull]

😆

Bazel needs test mocks....

[rbeazleyspot]

🦆