Enables compiling against direct dependencies only - kotlin #842

nkoroste · 2022-10-07T23:37:26Z

Defines --@io_bazel_rules_kotlin//kotlin/settings:experimental_prune_transitive_deps=True that will allow you to make java and android to only use direct dependancies when compiling a given target. This significantly speeds up incremental compilations at the cost of adding explicit deps to all targets.

Relates to bazelbuild/bazel#16426

Can be tested with #840 which will produce the following expected output:

cd examples/deps && ./run.sh --@io_bazel_rules_kotlin//kotlin/settings:experimental_prune_transitive_deps=True

Rebuilt libjava targets: 4
Rebuilt libandroid targets: 4
Rebuilt libkt targets: 2
Rebuilt libktandroid targets: 2
Rebuilt libandroid res-only targets: 4
Rebuilt libktandroid res-only targets: 4

Bencodes · 2022-10-10T16:52:58Z

kotlin/internal/jvm/compile.bzl

@@ -101,16 +102,26 @@ def _jvm_deps(toolchains, associated_targets, deps, runtime_deps = []):
            ",\n ".join(["    %s" % x for x in list(diff)]),
        )
    dep_infos = [_java_info(d) for d in associated_targets + deps] + [toolchains.kt.jvm_stdlibs]
+
+    # Reduced classpath, exclude transitive deps from compilation
+    if (ctx.attr._experimental_prune_transitive_deps[BuildSettingInfo].value):


Would it be better to expose this at the toolchains.kt level so that there's a unified place to look up these high level build settings?

You're talking about kotlin/internal/toolchains.bzl right? defining this in toolchains will prevent you from passing this flag on CLI, no?
Right now you're able to do something like this:

bazel build --@io_bazel_rules_kotlin//kotlin/internal/jvm:experimental_prune_transitive_deps=True //foo

Ah yes you are actually right.

That isn't strictly true right? You can always have a config setting that enables the feature in the tool chain.

We do this for conditional multiplex workers

config_setting( name = "kotlinc_multiplex_workers", define_values = { "kotlinc_multiplex_workers": "enabled", }, ) define_kt_toolchain( name = "toolchain", api_version = "1.7", experimental_multiplex_workers = select({ ":kotlinc_multiplex_workers": True, "//conditions:default": False, }),

Then enable it via --define=kotlinc_multiplex_workers=enabled one could also do this as a proper flag. The benefits of this is that we don't need to ship skylib in the release archive and downstream users get full control over how it's toggled and set.

You're right, but that requires a lot of setup on the user side that can potentially be error prone. I personally think that having it via skylib is more user friendly and aligns with all other first class citizen flags we support. Though I don't have super strong feeling either way.

you can attach the build flag on the toolchain rule, there's no need for this build flag to be an attr for pretty much every kotlin rule...

rules_rust pulls the config settings in the toolchain, rules_go has a config rule that reads the flags and generates a config provider that is then consumed by other rules

either way the flags in these rules need to be better located and integrated, they are in an internal jvm package, they should be moved out to something like //kotlin/config:* so they easier to reason about

As Cris recommended, I've moved the flag under kotlin/settings and attached it to the toolchain as it's cleaner that way.
I'll do the same for the jdeps, kotlin_deps flag for consistency in a follow up pr.

Bencodes · 2022-10-12T15:27:52Z

kotlin/internal/jvm/BUILD

@@ -33,3 +33,13 @@ bzl_library(
        "@build_bazel_rules_android//android",
    ],
 )
+
+load("@bazel_skylib//rules:common_settings.bzl", "bool_flag")


@nkoroste I think we need to include bazel_skylib in the release archive as a release dependency.

Not sure how releases work to be honest so I would differ to your expertise

lukaciko · 2022-10-13T09:57:34Z

@nkoroste does this replace experimental_reduce_classpath_mode? What is the difference between the two?

kotlin/internal/jvm/android.bzl

rockwotj · 2022-10-19T11:19:04Z

@nkoroste does this replace experimental_reduce_classpath_mode? What is the difference between the two?

@lukaciko the difference with this PR is that the transitive jars aren't even passed into the action, so you get caching benefits instead of just giving kotlinc a smaller classpath. As far as I can tell, this PR should make experimental_reduce_classpath_mode obsolete. Please someone correct me if I'm wrong

See:

rules_kotlin/src/main/kotlin/io/bazel/kotlin/builder/tasks/jvm/CompilationTask.kt

Line 72 in 33c3165

inputs.directDependenciesList + transitiveDepsForCompile

arunkumar9t2 · 2022-10-19T11:50:50Z

I would not say this makes the reduced claspath mode redundant yet since this forces every target to list their deps which can be problematic for many @maven targets which do not define their impl vs api correctly. Eg. Room uses LiveData but does not declare it. Fixing this manually is lot of pain.

It is true that not passing jars as inputs is the best option since we get caching benefits. To do that in starlark for rules_kotlin however we need few changes from Bazel.

Bazel already has experimental class path mode called bazel which prunes transitive classpath based on jdeps before action registration. But this is not exposed to starlark yet. Rules kotlin already has jdeps and if this function is available via java_common like

jars = java_common.reduce_classpath(transitive_classpath, jdeps)

Then we can also do classpath reduction before action registration which will help with compile avoidance.

Lastly bazel has automatic recovery in default java_builder classpath mode. It attempts compilation with direct jars first and if it fails it falls back to full transitive classpath.

Side note:

What we are doing internally is we filter classpath only for project targets and for external targets like @maven we don't filter and pass as it is. This for now keeps build.bazel not too verbose. We pass this information to bazel and rules_kotlin via tags.

nkoroste · 2022-10-25T18:28:25Z

As far as I can tell, this PR should make experimental_reduce_classpath_mode obsolete. Please someone correct me if I'm wrong

I agree with @arunkumar9t2, it doesn't make it obsolete as we can do reduction on both the class level (bazelbuild/bazel#16457) and on the jar/deps level which is what this PR is trying to achieve.

It is true that not passing jars as inputs is the best option since we get caching benefits. To do that in starlark for rules_kotlin however we need few changes from Bazel.

Are you talking about this bazelbuild/bazel#16426?

Side note:
What we are doing internally is we filter classpath only for project targets and for external targets like @maven we don't filter and pass as it is. This for now keeps build.bazel not too verbose. We pass this information to bazel and rules_kotlin via tags.

Do you mind sharing what you have? I guess my PR will take care of both which does have pretty large performance gain for us

…idanceKotlin_upstream * upstream/master: (51 commits) Expose ksp_version (bazelbuild#989) Bump quick guide to use v1.8 (bazelbuild#987) Fail CI if the docs are outdated (bazelbuild#961) Rename dev_io_bazel_rules_kotlin -> io_bazel_rules_kotlin (bazelbuild#973) update stardoc to 0.5.6 (bazelbuild#986) fix readme about kotlinc_opts and javac_opts (bazelbuild#984) Update README with KSP support (bazelbuild#983) Regenerate the docs (bazelbuild#980) Update the README.md with the latest dev override config (bazelbuild#981) Remove opts.release.bzl which is no longer being used (bazelbuild#982) Remove print warning from kt_download_local_dev_dependencies (bazelbuild#974) Avoid creating duplicate android_sdk_repositories (bazelbuild#978) Normalize label (bazelbuild#968) Generate koltinc options (bazelbuild#962) Restore neverlink on compiler. (bazelbuild#977) Use the mnemonic for worker keys (bazelbuild#976) Update KtLint to 0.49.1 (bazelbuild#970) Add support for -Xuse-fir-lt (bazelbuild#972) Support -Xenable-incremental-compilation (bazelbuild#971) Update to Kotlin 1.8.21 (bazelbuild#969) ...

…s installed

…ps_incompatible tag

…idanceKotlin_upstream * upstream/master: Move the KSP and compiler target definitions into rules_kotlin (bazelbuild#1014) Avoid running java_common#compile against KSP generated outputs (bazelbuild#990) Update kt_java_stub_template to 6.2.1 (bazelbuild#992) Disable strict deps for Android example (bazelbuild#1013) Group the all_tests jobs together in presubmit.yml (bazelbuild#1008) Add rules_java 6.4.0 (bazelbuild#1010) Update Bazel to 6.3.2 (bazelbuild#1007) Update rules_jvm_external to 5.3 (bazelbuild#1009) Migrate usages of deprecated `JavaInfo` fields (bazelbuild#1005) Remove unnecessary Java runtime dependencies (bazelbuild#1000)

Bencodes · 2023-09-21T17:13:23Z

kotlin/internal/jvm/android.bzl

+            **kwargs
+        )
+        exported_target_labels.append(base_name)
+    elif resource_files:


This conditional also needs to check if there is a manifest, otherwise manifests might get left behind.

Prior to bazelbuild#842, kwargs were sent to the base android library rule. In that PR, one of the branches no longer contained it. This causes any target with resources/manifest not to send kwargs, which means that e.g. `assets` don't get passed down.

* Revert android.bzl changes introduced by #842 * Formatting

) * Revert android.bzl changes introduced by bazelbuild#842 * Formatting

nkoroste mentioned this pull request Oct 7, 2022

Enables compiling against direct dependencies only bazelbuild/bazel#16426

Open

Bencodes reviewed Oct 10, 2022

View reviewed changes

oliviernotteghem mentioned this pull request Oct 11, 2022

JVM build time improvements via tracking unused deps or classes [Java part] bazelbuild/bazel#16457

Open

Bencodes reviewed Oct 12, 2022

View reviewed changes

nkoroste marked this pull request as ready for review October 13, 2022 00:08

nkoroste requested review from cgruber, timpeut and restingbull as code owners October 13, 2022 00:08

nkoroste force-pushed the nk-compilationAvoidanceKotlin_upstream branch from e8fdb31 to 7ec08e4 Compare October 13, 2022 00:11

Bencodes reviewed Oct 14, 2022

View reviewed changes

kotlin/internal/jvm/android.bzl Outdated Show resolved Hide resolved

nkoroste commented Oct 18, 2022

View reviewed changes

kotlin/internal/jvm/android.bzl Show resolved Hide resolved

nkoroste added 4 commits November 1, 2022 12:55

Enables compiling against direct dependencies only - kotlin

3416501

optimize libktandroid targets

16fecde

add kt_prune_transitive_deps_incompatible tag support

32638bf

fix

f11d238

nkoroste force-pushed the nk-compilationAvoidanceKotlin_upstream branch from 377c9a7 to f11d238 Compare November 1, 2022 19:56

nkoroste added 3 commits November 1, 2022 12:59

fixes after rebase

a05fc41

Introduce the new flag under kotlin/settings

2aabe48

Remove traced of the old flag

2b4cbe8

nkoroste force-pushed the nk-compilationAvoidanceKotlin_upstream branch from 091dc74 to 2b4cbe8 Compare December 14, 2022 19:49

nkoroste and others added 6 commits December 14, 2022 12:03

buildifier cleanup

37af8f2

more buildifier

1558636

Merge branch 'master' into nk-compilationAvoidanceKotlin_upstream

9f14ecf

fix after git merge

3186a38

remove explicit builds tool version to fallback on whatever version i…

b2ab463

…s installed

nkoroste added 3 commits July 26, 2023 16:37

All excluding specific target via kt_experimental_prune_transitive_de…

81cce1e

…ps_incompatible tag

buildifier kotlin/internal/toolchains.bzl

ce390b2

Bencodes reviewed Sep 21, 2023

View reviewed changes

Add manifest check

d28f9fa

nkoroste requested a review from ahumesky as a code owner September 22, 2023 19:29

Bencodes approved these changes Sep 22, 2023

View reviewed changes

nkoroste merged commit 004189e into bazelbuild:master Sep 29, 2023
1 check passed

Bencodes mentioned this pull request Oct 4, 2023

Experimental strict kotlin deps should support target exclusion #1041

Open

pswaminathan mentioned this pull request Feb 13, 2024

Send **kwargs to base _android_library #1110

Closed

Bencodes added a commit that referenced this pull request Mar 1, 2024

Revert android.bzl changes introduced by #842

efcc8f8

Bencodes added a commit that referenced this pull request Mar 1, 2024

Revert android.bzl changes introduced by #842 (#1122)

0f4c0af

* Revert android.bzl changes introduced by #842 * Formatting

oliviernotteghem pushed a commit to uber-common/rules_kotlin that referenced this pull request Aug 2, 2024

Revert android.bzl changes introduced by bazelbuild#842 (bazelbuild#1122

f6c973f

) * Revert android.bzl changes introduced by bazelbuild#842 * Formatting

oliviernotteghem pushed a commit to uber-common/rules_kotlin that referenced this pull request Aug 2, 2024

Revert android.bzl changes introduced by bazelbuild#842 (bazelbuild#1122

4dea295

) * Revert android.bzl changes introduced by bazelbuild#842 * Formatting

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enables compiling against direct dependencies only - kotlin #842

Enables compiling against direct dependencies only - kotlin #842

nkoroste commented Oct 7, 2022 •

edited

Loading

Bencodes Oct 10, 2022

nkoroste Oct 12, 2022

Bencodes Oct 12, 2022

rockwotj Oct 19, 2022

nkoroste Oct 25, 2022

chancila Nov 23, 2022 •

edited

Loading

nkoroste Dec 14, 2022

Bencodes Oct 12, 2022

nkoroste Oct 13, 2022

lukaciko commented Oct 13, 2022

rockwotj commented Oct 19, 2022

arunkumar9t2 commented Oct 19, 2022 •

edited

Loading

nkoroste commented Oct 25, 2022

Bencodes Sep 21, 2023

nkoroste Sep 22, 2023

Enables compiling against direct dependencies only - kotlin #842

Enables compiling against direct dependencies only - kotlin #842

Conversation

nkoroste commented Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chancila Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaciko commented Oct 13, 2022

rockwotj commented Oct 19, 2022

arunkumar9t2 commented Oct 19, 2022 • edited Loading

nkoroste commented Oct 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkoroste commented Oct 7, 2022 •

edited

Loading

chancila Nov 23, 2022 •

edited

Loading

arunkumar9t2 commented Oct 19, 2022 •

edited

Loading