[SNAP FORK] fix aar_import rule #52
Conversation
|
Hi Mauricio, Can you elaborate on why ACLs don't work with external dependencies (i.e. what failure are you seeing)?
I'll have to check with internal rule maintainers about how we can handle this. Currently this is causing an internal test to fail, so there might be some projects that rely on this behavior. |
rules/aar_import/impl.bzl
Outdated
| @@ -344,7 +344,7 @@ def _process_jars( | |||
| ), | |||
| source_jar = source_jar, | |||
| neverlink = False, | |||
| deps = r_java_info + java_infos, # TODO(djwhang): Exports are not deps. | |||
| deps = java_infos, # TODO(djwhang): Exports are not deps. | |||
There was a problem hiding this comment.
This line specifically is causing the internal test to fail. I will follow up with internal teams to chase down the root cause.
There was a problem hiding this comment.
reverted. the java_infos being added as deps were exposing r classes as runtime deps which is not correct and was causing issues in our repo. Instead I set r java infos as neverlink.
mauriciogg
force-pushed
the
pre-alpha-fix-aar
branch
4 times, most recently
from
0fedda7 to
86ed4c9
Compare
The acl code does not support filed issue here #68 |
mauriciogg
force-pushed
the
pre-alpha-fix-aar
branch
2 times, most recently
from
d855b7c to
f2285a0
Compare
mauriciogg
force-pushed
the
pre-alpha-fix-aar
branch
from
f2285a0 to
a00efd0
Compare
- acls dont work with external dependencies (i.e @foo//) - Propagate StarlarkAndroidResourcesInfo otherwise starlark rules wont be able to see those resources. - Stop adding library R classes as runtime dependencies: This behavior is not correct- the only authoritive r class should be the final merged R class. Adding library R classes as runtime dependencies creates resources erasures in android_local_test targets where each individual R class is added to the test classpath and incorrect Ids are used to query the resource table. - Merge aar 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. see AarImport.java https://github.com/bazelbuild/bazel/blob/e0a9081f/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java#L166
mauriciogg
force-pushed
the
pre-alpha-fix-aar
branch
from
a00efd0 to
36cca43
Compare
| combined_java_info = java_common.merge([java_info] + r_java_info) | ||
| providers.append(combined_java_info) |
There was a problem hiding this comment.
This is currently breaking some assumptions in internal tests. I will try to follow up on whether we should change the assumptions or not.
There was a problem hiding this comment.
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.
| combined_java_info = java_common.merge([java_info] + r_java_info) | ||
| providers.append(combined_java_info) |
There was a problem hiding this comment.
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.
|
This should fix #77 if I understand it properly. Thanks! |
| # 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: |
There was a problem hiding this comment.
Is there any reason why if resource_files shouldn't be if resources_ctx[_R_JAVA], which was the past behavior on line 1696?
bazelbuild#52) of `--android_crosstool_top` works.
acls dont work with external dependencies (i.e @foo//)
https://github.com/bazelbuild/rules_android/blob/pre-alpha/rules/acls.bzl#L293
Propagate StarlarkAndroidResourcesInfo otherwise starlark rules
wont be able to see those resources.
Stop adding library R classes as runtime dependencies:
This behavior is not correct- the only authoritive r class should be
the final merged R class. Adding library R classes as runtime
dependencies creates resources erasures in android_local_test targets
where each individual R class is added to the test classpath and
incorrect Ids are used to query the resource table.
Merge aar 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.
see AarImport.java
https://github.com/bazelbuild/bazel/blob/e0a9081f/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java#L166
I think this should fix #39 as well