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.

Sign up for GitHub

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

[SNAP FORK] fix aar_import rule #52

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[SNAP FORK] fix aar_import rule #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions rules/aar_import/impl.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ def _process_resources(
deps = ctx.attr.deps,
exports = ctx.attr.exports,
exports_manifest = getattr(ctx.attr, "exports_manifest", True),
propagate_resources = _acls.in_aar_propagate_resources(str(ctx.label)),
propagate_resources = True,

# Tool and Processing related inputs
aapt = _get_android_toolchain(ctx).aapt2.files_to_run,
Expand Down Expand Up @@ -351,7 +351,16 @@ def _process_jars(
# TODO(djwhang): AarImportTest is not expecting jdeps, enable or remove it completely
# jdeps = jdeps_artifact,
)
providers.append(java_info)

# Merge library java_info with the resource java_info
# Libraries that directly depend on an AAR can depend as well on the AAR resource class
# so it is not enough to simply return the resource info as a dependency.
# Note that adding r_java as an export is also not the correct behavior, since we only
# want to expose it as a direct dependency.
# see AarImport.java
# https://github.com/bazelbuild/bazel/blob/e0a9081f/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java#L166
combined_java_info = java_common.merge([java_info] + r_java_info)
providers.append(combined_java_info)
Comment on lines +362 to +363
Copy link
Contributor

Choose a reason for hiding this comment

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

This is currently breaking some assumptions in internal tests. I will try to follow up on whether we should change the assumptions or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this some more: if you replace line 349's _acls.in_aar_import_exports_r_java(str(ctx.label)) with True and undo your change from lines 354-364, it makes internal tests pass again. Does that behavior work for your usecase? Merging the r_java_info with the java_info causes internal assumptions about aar_import providers to break (i.e. internally we are expecting only one srcjar to come out of aar_import, whereas your PR seems to generate two), which makes accepting this PR much more difficult.


return struct(
java_info = java_info,
Expand Down Expand Up @@ -493,8 +502,7 @@ def impl(ctx):
r_java = resources_ctx.r_java,
exports = _utils.collect_providers(JavaInfo, ctx.attr.exports),
enable_desugar_java8 = ctx.fragments.android.desugar_java8,
enable_imports_deps_check =
_acls.in_aar_import_deps_checker(str(ctx.label)),
enable_imports_deps_check = False,
aar_embedded_jars_extractor_tool =
_get_android_toolchain(ctx).aar_embedded_jars_extractor.files_to_run,
bootclasspath =
Expand Down
5 changes: 5 additions & 0 deletions rules/aar_import/rule.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

load(":attrs.bzl", _ATTRS = "ATTRS")
load(":impl.bzl", _impl = "impl")
load(
"//rules:providers.bzl",
"StarlarkAndroidResourcesInfo",
)

RULE_DOC = """
#### Examples
Expand All @@ -42,6 +46,7 @@ aar_import = rule(
AndroidLibraryResourceClassJarProvider,
AndroidNativeLibsInfo,
JavaInfo,
StarlarkAndroidResourcesInfo,
],
toolchains = ["//toolchains/android:toolchain_type"],
)
9 changes: 5 additions & 4 deletions rules/resources.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1529,6 +1529,7 @@ def _process_starlark(
output_jar = out_class_jar,
compile_jar = out_class_jar,
source_jar = r_java,
neverlink = True,
)

packages_to_r_txts_depset.setdefault(java_package, []).append(depset([out_aapt2_r_txt]))
Expand Down Expand Up @@ -1687,13 +1688,13 @@ def _process_starlark(
resources_ctx[_R_JAVA] = None
resources_ctx[_PROVIDERS] = []

# TODO(b/69552500): In the Starlark Android Rules, the R compile time
# JavaInfo is added as a runtime dependency to the JavaInfo. Stop
# adding the R.jar as a runtime dependency.
r_java = None
if resource_files:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason why if resource_files shouldn't be if resources_ctx[_R_JAVA], which was the past behavior on line 1696?

r_java = resources_ctx[_R_JAVA]
resources_ctx[_PROVIDERS].append(
AndroidLibraryResourceClassJarProvider(
depset(
(resources_ctx[_R_JAVA].runtime_output_jars if resources_ctx[_R_JAVA] else []),
(utils.list_or_depset_to_list(r_java.compile_jars) if r_java else []),
transitive = [
p.jars
for p in utils.collect_providers(
Expand Down