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.

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

[WIP][POC] Starlark implementation of kt_android_* rules #868

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mauriciogg
Copy link
Contributor

@mauriciogg mauriciogg commented Nov 6, 2022

This PR is intended to be a proof of concept of integrating rules_kotlin with the Starlark version for rules_android.
This PR uses the pre-alpha version of rules_android with some additional fixes to integrate with rules_kotlin here (Note that rules_android is not officially supported and this is just a private fork).

The most interesting bits of these PR are in the android.bzl, android_impl.bzl and android_resources.bzl files. Additionally, examples/android contains an example usage of android_application with a very simple configuration.

There are several benefits of integrating with rules_android:

  • kt_android_* rules can be implemented natively in this repo without the use of sandwich rules.
    This means that ${target}_kt targets no longer exist only the original ${target}
  • Decoupled from Bazel binary. Enhancements to the resource processing (the main pice of functionality we use from rules_android here) pipeline can happen without needing to wait for Bazel updates.
  • Enable use of app bundles and dynamic features via android_application rule. This rule will not be implemented in native Bazel.
  • Future proofing rules_kotlin- native rules_android will be deprecated at some point.

This PR does the following:

  • Implements kt_android_library and kt_android_local_test by using the resource processing pipeline implemented by rules_android. This enables these targets to provide the StarlarkAndroidResourcesInfo provider which is the provider collected by the Starlark rules and need in order to use the android_application rule.
  • Toggles the usage of starlark rules_android in rules_jvm_external by setting use_starlark_android_rules = True in maven_install
  • Migrates the examples to load the pre-alpha rules_android.
  • Adds an android_application target that generates an App Bundle under examples/android

The performance of this implementation is very close to the current implementation (tested on our internal repo). The build graph is mostly the same as what currently exist. In addition multiplex workers are enabled for the resources actions (same as current implementation). We are experimentally building our Android project internally using this implementation and all our builds and tests are green, but your milage may vary (fixes were tailored to problems I encountered in our repo).

The main objective of this PR is to start the conversation towards an official migration, move the rules_android (my fork) repo (ideally bazelbuild/rules_android) to a more official home and create a shared stewardship of the repo.

Some caveats

  • This was only tested with our internal repo so YMMV. The upstream rules_android repo is not yet officially supported. See Android Rules Starlark Migration bazel#5354
  • dynamic features work only with targets that contain native libs and assets (not sources and not resources)
  • fetching sources in maven_install is not possible (this is a simple filename extension issue)
  • depending on concrete files from a external maven targets will fail since for whatever reason the visibility of those artifacts is private when using starlak rules_android`
  • Changes are required in the intellij plugin for these rules to play nice with the IDE

http_archive(
name = "build_bazel_rules_android",
name = "io_bazel_rules_android",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason for the name change here? I think that we should keep it consistent so this these repositories aren't downloaded multiple times. And a while back we intentionally switched to build_bazel_rules_android to align with the rest of the community and what rules_android recommends.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no reason I think I just copied the pattern from rules_kotlin but can switch it back to whatever

# See the License for the specific language governing permissions and
# limitations under the License.

"""Various Java utils that were copied from pre-alpha branch of rules_android repo
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason we can't just reference the ones in rules_android? Are they not public? Alternatively could we advocate to move these into https://github.com/bazelbuild/bazel-skylib?

@Bencodes
Copy link
Collaborator

Bencodes commented Nov 6, 2022

We should probably cut a new release of rules_kotlin before we land this PR so that there's something stable out there in case this has bugs.

@nkoroste nkoroste requested a review from ahumesky November 7, 2022 18:40
@nkoroste
Copy link
Collaborator

nkoroste commented Nov 7, 2022

fyi @ahumesky - it will need some of the fixes from https://github.com/bazelbuild/rules_android/pulls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants