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

Use http_jar instead of rules_jvm_external to download bundletool #81

Open
wants to merge 1 commit into
base: pre-alpha
Choose a base branch
from

Conversation

Bencodes
Copy link
Contributor

No description provided.

@ahumesky
Copy link
Collaborator

So coincidentally I'm about to add another dependency to maven_install (com.android.tools.build:gradle:8.0.1 for R8 integration) which has a number of transitive dependencies, so in the end I think we'll need to keep maven_install.

I also noticed that bundletool has a number of dependencies too (https://maven.google.com/web/index.html?q=bundle#com.android.tools.build:bundletool:1.6.1), e.g. dagger, autovalue, guava, proto, etc -- are these not needed to run the tool?

@Bencodes
Copy link
Contributor Author

So coincidentally I'm about to add another dependency to maven_install (com.android.tools.build:gradle:8.0.1 for R8 integration) which has a number of transitive dependencies, so in the end I think we'll need to keep maven_install.

Would be great to avoid using it if we can at least for externally facing tools. We've found that it's generally less performant than just using an http_jar, especially as you add more maven dependencies. You can mitigate this by generating lock files, but this isn't user-friendly outside of the rules development context.

It's also a lot easier to do overrides with --override_repository when these dependencies are pulled as a maybe of http_jar than having to override a complex repository made by rules_jvm_external. This has been especially useful for us in the android_tools where we can upgrade r8/d8 independently of what's bundled into Bazel inside our own WORKSPACE file. Ex:

http_jar(
    name = "android_gmaven_r8",
    sha256 = "sha goes here",
    url = "http url to the jar goes here",
)

Being able to do this for tools like bundletool would be especially useful and would put less burden on rules_android to track updates of https://github.com/google/bundletool, or have to roll rules_android releases with minor app bundle updates.

http_jar(
    name = "bundletool",
	...
)

I also noticed that bundletool has a number of dependencies too (maven.google.com/web/index.html?q=bundle#com.android.tools.build:bundletool:1.6.1), e.g. dagger, autovalue, guava, proto, etc -- are these not needed to run the tool?

The all-jars downloaded from Github have everything compiled in that's needed to be able to execute as a CLI tool. And it's what we've been using in our builds for the past year to support app bundles.

MODULE.bazel Outdated
)

rules_android_extensions = use_extension("//:prereqs.bzl", "rules_android_extensions")
use_repo(rules_android_extensions, "bundletool_all", "py_absl")
Copy link
Contributor

Choose a reason for hiding this comment

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

We just merged this commit that uses the BCR version of py_absl.

@Bencodes Bencodes force-pushed the http-jar-bundletool branch from 0dab1e0 to 9791453 Compare May 24, 2023 17:12
Copy link
Contributor

@ted-xie ted-xie left a comment

Choose a reason for hiding this comment

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

As discussed on Slack, would you mind making a few changes?

  1. Rebase this PR on top of the HEAD of main branch
  2. Make the PR itself a pull request into main through the github UI.
  3. Revert changes that deleted Maven deps. As discussed, we want to keep Maven for internal-facing repository deps, and use http_jar() whenever possible for external-facing repo deps.

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