New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 830641 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Last visit > 30 days ago
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Android Build: untangle java_library_impl() GN template

Project Member Reported by digit@chromium.org, Apr 9 2018

Issue description

The Android build rules (build/config/android/internal_rules.gni) provide an all-mighty template called "java_library_impl" that includes a ton of functionality that go far beyond the compilation of Java libraries (for example, support for building Android APKs).

This makes it difficult to understand what's going, and more importantly, to change the way Android APKs, or other constructs, are being built.

My number one issue is that the APK's .build_config file is generated through java_library_impl() and must contain a lot of information that is not strictly related to java libraries.

I really want to be able to perform this operation in android_apk() instead (in particular to change a few things / add a few new fields, for future support of APK splits).

This bug is to track the issue and cleanup the build rules appropriately.

 
One of the reasons it is this way is that android_apk *is* a java_library (e.g. you can attach java_files to it. It somewhat has to be this way because of R.java needing to be generated per-apk. 

Perhaps the main thing that could be done is to split apks into two .build_configs. One for java_library, and the other for the apk steps.
If splitting into two, test apk build config logic would probably need to be tweaked because it has some special-case logic for adding the apk's java to its classpath.

Comment 3 by digit@chromium.org, Apr 10 2018

Yes, that's what I'm seeing, but I don't see a reason why we should not move all the Java-library related stuff into its own sub-target (with its own .build_config, and added as a dependency of the apk target/build_config itself).

It is slightly tricky, but should make things a lot easier to understand and maintain in the future, and I more or less need this for app bundles.

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/372fccea34f75bd88247bb99f562b3279297396c

commit 372fccea34f75bd88247bb99f562b3279297396c
Author: David 'Digit' Turner <digit@google.com>
Date: Thu Apr 12 14:22:30 2018

android: Document more GN build rules + minor cleanups.

This CL tries to document more internal GN build rules,
as a preliminary for future patches that will perform
refactoring of some of them. In particular to simplify
the impressive java_library_impl() rule.

Also perform a little cleanup, since this led to
discover a few obsolete things:

- Remove the 'manifest_entries' variable since it is
  never set, and its entries passed as --manifest-entry=
  arguments to javac.py, which doesn't support it!

- Remove forwarding of the optional public_deps variable,
  in cases where it would never be used by the final
  rules that receive it (it was never being set by any
  upstream callers).

- Remove un-needed passing of processors_javac which
  seems obsolete.

- Move more variables in the (type == "android_apk")
  block of java_library_impl() to make it clearer that
  these are only used for android_apk() targets.

BUG=830641
R=agrieve@chromium.org

Change-Id: I489c1921d58bc9f649a5bc02cf630c36e607b20a
Reviewed-on: https://chromium-review.googlesource.com/1007061
Commit-Queue: David Turner <digit@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550182}
[modify] https://crrev.com/372fccea34f75bd88247bb99f562b3279297396c/build/config/android/internal_rules.gni

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/372fccea34f75bd88247bb99f562b3279297396c

commit 372fccea34f75bd88247bb99f562b3279297396c
Author: David 'Digit' Turner <digit@google.com>
Date: Thu Apr 12 14:22:30 2018

android: Document more GN build rules + minor cleanups.

This CL tries to document more internal GN build rules,
as a preliminary for future patches that will perform
refactoring of some of them. In particular to simplify
the impressive java_library_impl() rule.

Also perform a little cleanup, since this led to
discover a few obsolete things:

- Remove the 'manifest_entries' variable since it is
  never set, and its entries passed as --manifest-entry=
  arguments to javac.py, which doesn't support it!

- Remove forwarding of the optional public_deps variable,
  in cases where it would never be used by the final
  rules that receive it (it was never being set by any
  upstream callers).

- Remove un-needed passing of processors_javac which
  seems obsolete.

- Move more variables in the (type == "android_apk")
  block of java_library_impl() to make it clearer that
  these are only used for android_apk() targets.

BUG=830641
R=agrieve@chromium.org

Change-Id: I489c1921d58bc9f649a5bc02cf630c36e607b20a
Reviewed-on: https://chromium-review.googlesource.com/1007061
Commit-Queue: David Turner <digit@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550182}
[modify] https://crrev.com/372fccea34f75bd88247bb99f562b3279297396c/build/config/android/internal_rules.gni

Project Member

Comment 6 by bugdroid1@chromium.org, May 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/609082c660bd5f231e592d9701e2ea3b84066fee

commit 609082c660bd5f231e592d9701e2ea3b84066fee
Author: David 'Digit' Turner <digit@google.com>
Date: Tue May 08 12:57:33 2018

android: Do not generate .apk.jar.info files in javac.py

When compiling Java sources into .jar files, the javac.py
script will also generate a .jar.info file, used to map
fully-qualified Java class names to the path of the
corresponding source or .srcjar file.

These are stored along the generated .jar file, e.g.:

  $OUT/gen/base/base_java.javac.jar
  $OUT/gen/base/base_java.javac.jar.info

In addition to this, each APK has a final .apk.jar.info
file stored under $OUT_DIR/size-info/ that corresponds to the
union of all the .jar.info files of its dependencies, e.g.:

  $OUT/size-info/ChromePublic.apk.jar.info

Before this CL, the .apk.jar.info was generated by javac.py
when compiling the APK's own source files (which include things
like the auto-generated resource-related .srcjar files), by
passing the --apk-jar-info option to the script.

Unfortunately, this made the implementation of the android_apk(),
java_library_impl() and compile_java() GN templates slightly more
complex (tightly coupled) than needed, making future changes more
difficult.

This CL tries to untangle the coupling as follows:

- Add a new script (merge_jar_info_files.py) to merge one or
  more .jar.info file into a final .apk.jar.info.

- Remove --apk-jar-info option from javac.py. The latter is now
  only responsible for generating the .jar.info of its final
  .jar file, and doesn't need to know anything about that may
  or may not embed it later.

  Similarly, remove the 'apk_name' variable from compile_java()
  and java_library_impl().

- Ensure that the android_apk() generates the final .apk.jar.info
  file by calling merge_jar_info_files.py.

+ Move common functionality to build/android/gyp/util/jar_info_utils.py

To test the result, I manually checked that all .apk.jar.info files
are identical after sorting (since they are a dump of a Python dictionary,
with no guaranteed ordering:

BUG=830641
R=agrieve@chromium.org, yfriedman@chromium.org

Change-Id: I2051bc35aba2931947d2a30db507a4d437998238
Reviewed-on: https://chromium-review.googlesource.com/1047269
Commit-Queue: David Turner <digit@chromium.org>
Reviewed-by: Peter Wen <wnwen@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556770}
[modify] https://crrev.com/609082c660bd5f231e592d9701e2ea3b84066fee/build/android/gyp/javac.py
[add] https://crrev.com/609082c660bd5f231e592d9701e2ea3b84066fee/build/android/gyp/merge_jar_info_files.py
[add] https://crrev.com/609082c660bd5f231e592d9701e2ea3b84066fee/build/android/gyp/util/jar_info_utils.py
[modify] https://crrev.com/609082c660bd5f231e592d9701e2ea3b84066fee/build/config/android/internal_rules.gni
[modify] https://crrev.com/609082c660bd5f231e592d9701e2ea3b84066fee/build/config/android/rules.gni

Blocking: -820459
We can keep this bug as is, but it is no longer blocking Android app bundles :)

Sign in to add a comment