Check R8 usage before grabbing proguard outputs from ctx.outputs

[pswaminathan]

Fixes #343

[ahumesky]

Thanks for the fix, but I wonder why this is not getting caught by this test?

rules_android/test/rules/android_binary/r8_integration/BUILD

Lines 5 to 15 in 75a50ae

	py_test(
	name = "r8_integration_test",
	srcs = ["r8_integration_test.py"],
	args = ["$(location @androidsdk//:dexdump)"],
	data = [
	"//test/rules/android_binary/r8_integration/java/com/basicapp:basic_app_R8_no_shrink",
	"//test/rules/android_binary/r8_integration/java/com/basicapp:basic_app_R8_shrink",
	"//test/rules/android_binary/r8_integration/java/com/basicapp:basic_app_no_R8",
	"@androidsdk//:dexdump",
	],
	)

which depends on (builds) an android app with R8 and proguard_specs set:

rules_android/test/rules/android_binary/r8_integration/java/com/basicapp/BUILD

Lines 25 to 37 in 75a50ae

	for name, specs, shrink in [
	(
	"basic_app_R8_shrink",
	["proguard.cfg"],
	True,
	),
	(
	"basic_app_R8_no_shrink",
	["proguard.cfg"],
	False,
	),
	("basic_app_no_R8", [], False),
	]

[pswaminathan]

@ahumesky that is indeed a very good question. I don't have a good answer for you at the moment, as I didn't see that test when I was working on this. I do know that I was able to reproduce that issue consistently with the example app, so it wasn't just limited to our environment. I haven't looked at this logic in a while, and I won't be able to look at it for the next week or so at least, but after that I should be able to take a look and at least see what I find with this test.

[pswaminathan]

@ahumesky I believe I have this figured out, and it results from using selects. Let's start with the following:

    proguard_specs = select({
        ":is_c_opt": ["some_config.pro"],
        "//conditions:default": [],
    }),
    proguard_generate_mapping = True,

Here, we look for the ACL (which is true), but also for ctx.files.proguard_specs. In the above case, if this is not an optimized build, this resolves to an empty list and so validation_ctx.use_r8 == False. But here, generate_proguard_outputs is True, because proguard_generate_mapping is True. Thus, this fails. We can't set proguard_generate_mapping as a select, though, because that yields an error. I'm not sure why actually—I've never seen this error before and I can't really find much about it when Googling:

Attribute 'proguard_generate_mapping' either doesn't exist or uses a select() (i.e. could have multiple values)
Available attributes: _action_listener, _add_g3itr_xslt, _allowlist_function_transition, _android_manifest_merge_tool, _android_sdk_toolchain_type, _bytecode_optimizer, _cc_toolchain_split, _config_dependencies, _databinding_use_androidx, _defined_resource_files, _desugared_java8_legacy_apis, _desugared_lib_config, _dummy_android_sdk, _enable_manifest_merging, _generate_proguard_outputs, _host_javabase, _java_toolchain, _legacy_main_dex_list_generator, _manifest_merge_order, _optimizing_dexer, _package_name, _rewrite_resources_through_optimizer, _use_auto_exec_groups, _whitelist_config_feature_flag, _whitelist_config_feature_flag_setter, _xsltproc_tool, applicable_licenses, aspect_hints, assets, assets_dir, compatible_with, custom_package, data, debug_key, debug_signing_keys, debug_signing_lineage_file, densities, deprecation, deps, dexopts, enable_data_binding, exec_compatible_with, exec_properties, expect_failure, feature_flags, features, generate_art_profile, generator_function, generator_location, generator_name, incremental_dexing, instruments, javacopts, key_rotation_min_sdk, main_dex_list, main_dex_list_opts, main_dex_proguard_specs, manifest, manifest_merger, min_sdk_version, multidex, name, native_target, nocompress_extensions, plugins, proguard_apply_mapping, proguard_optimization_passes, resource_apks, resource_configuration_filters, resource_files, restricted_to, srcs, startup_profiles, tags, target_compatible_with, testonly, toolchains, transitive_configs, use_r_package, visibility

I'm not entirely sure what the best path forward here is. One of:

1. Merge this PR as-is. This makes the usage symmetrical with the initial implementation of those outputs, which is at least internally consistent
2. Remove the not acls.use_r8(label) condition on the other side. I would say this is viable, except I don't know why that condition was there to begin with.
3. Make proguard_generate_mapping configurable. I don't quite know how to approach this.

[ted-xie]

For option 1, I'd want to have an integration test (see //test/rules/android_binary[...]) before we merge this.

For option 2, we need the R8 ACL for the forseeable future because we have internal desugar/dex/minification flows that don't use R8 and have a separate proguarding step.

For option 3, I'm not 100% sure either, but I think this is related to some of the special glue logic we have in the android_binary wrapper. Remember that when you load android_binary(), you're actually loading the macro wrapper around the rule itself, and there's some extra massaging that happens in the linked file. That massaging happens to ocur on generate_proguard_outputs, which has logic that's significantly intertwined with proguard_generate_mapping. Just spit-balling here, but we may need to refine this logic further or possibly add another one of these hidden implicit attributes (i.e. a possible new attr named $generate_proguard_mapping).

[pswaminathan]

@ted-xie @ahumesky I added a test here that passes in the current branch. If you revert the change to rules/android_binary/impl.bzl, it fails:

$ bazel test //test/rules/android_binary/r8_integration:all
...
ERROR: /Users/p/Code/bazel/rules_android/test/rules/android_binary/r8_integration/java/com/basicapp/BUILD:4:19: in android_binary rule //test/rules/android_binary/r8_integration/java/com/basicapp:basic_app_select_R8_genmap: 
Traceback (most recent call last):
	File "/Users/p/Code/bazel/rules_android/rules/android_binary/impl.bzl", line 1118, column 35, in impl
		return processing_pipeline.run(ctx, java_package, _PROCESSING_PIPELINE)
	File "/Users/p/Code/bazel/rules_android/rules/processing_pipeline.bzl", line 85, column 23, in _run
		info = execute(ctx, **target_ctx)
	File "/Users/p/Code/bazel/rules_android/rules/android_binary/impl.bzl", line 803, column 42, in _process_optimize
		proguard_output_jar = ctx.outputs.proguard_jar
Error: No attribute 'proguard_jar' in outputs. Make sure you declared a rule output with this name.
ERROR: /Users/p/Code/bazel/rules_android/test/rules/android_binary/r8_integration/java/com/basicapp/BUILD:4:19: Analysis of target '//test/rules/android_binary/r8_integration/java/com/basicapp:basic_app_select_R8_genmap' failed
Use --verbose_failures to see the command lines of failed build steps.
ERROR: Analysis of target '//test/rules/android_binary/r8_integration:r8_integration_test' failed; build aborted: Analysis failed
INFO: Elapsed time: 0.600s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
ERROR: Build did NOT complete successfully
//test/rules/android_binary/r8_integration:android_binary_with_neverlink_deps_build_test NO STATUS
//test/rules/android_binary/r8_integration:r8_neverlink_deps_test     NO STATUS

Executed 0 out of 2 tests: 2 were skipped.

Then if you apply the change to that file, it succeeds:

$ bazel test //test/rules/android_binary/r8_integration:all
...
INFO: Found 3 test targets...
INFO: Elapsed time: 108.044s, Critical Path: 13.83s
INFO: 328 processes: 6 internal, 290 darwin-sandbox, 32 worker.
INFO: Build completed successfully, 328 total actions
//test/rules/android_binary/r8_integration:android_binary_with_neverlink_deps_build_test PASSED in 0.1s
//test/rules/android_binary/r8_integration:r8_integration_test           PASSED in 0.2s
//test/rules/android_binary/r8_integration:r8_neverlink_deps_test        PASSED in 0.1s

Executed 3 out of 3 tests: 3 tests pass.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.

[ted-xie]

I merged this with some small edits for clarity.