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

Issue 843562 link

Starred by 3 users
Status: Fixed
Owner:
█ digit@chromium.org
Last visit > 30 days ago
Closed: Aug 7
Cc:
█ wnwen@chromium.org
estevenson@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocking:
issue 861762



Sign in to add a comment

Python scripts used in action()s should list all inputs directly rather than use depfiles

Project Member Reported by agrieve@chromium.org, May 16 2018 
We've always used depfiles to make ninja aware of dependent .py files for build actions. However, this can sometimes lead to trybots skipping builds because "gn analyze" does not know about depfile deps ( bug 827197 ).

We should change all scripts from using depfiles to listing inputs directly. Options include:

1) Use pydeps files for them
2) Use exec_script("//build/print_python_deps.py") in actions
3) Hand-code them.

Many scripts could probably use a combination of 2+3 by having build_utils.py deps stored in a variable, then just adding the one or two extra deps onto that.

Comment 1 by agrieve@chromium.org, May 23 2018 

Another option here is to do 2) only when gn_gen_analyze_mode=true is set. E.g. similar to how grit works here:
https://cs.chromium.org/chromium/src/tools/grit/grit_rule.gni?type=cs&q=compute_grit_inputs_for_analyze&sq=package:chromium&g=0&l=106
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 27 2018 

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

commit 0bb79bb7dba7b5f4b1837b3f579b19858974a3fb
Author: Andrew Grieve <agrieve@chromium.org>
Date: Wed Jun 27 03:14:09 2018

Make "gn analyze" know about all inputs for jinja_template()

* Renames compute_grit_inputs_for_analyze -> compute_inputs_for_analyze
* Computes .py inputs via exec_script (when compute_inputs_for_analyze=true)
* Enforce that sub-included .jinja files are listed in targets.

Bug:  843562 
Change-Id: I3698c7846375530a86474bd5960bb002ac45f71a
Reviewed-on: https://chromium-review.googlesource.com/1112867
Reviewed-by: Eric Stevenson <estevenson@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570650}
[modify] https://crrev.com/0bb79bb7dba7b5f4b1837b3f579b19858974a3fb/build/android/gyp/jinja_template.py
[modify] https://crrev.com/0bb79bb7dba7b5f4b1837b3f579b19858974a3fb/build/config/android/rules.gni
[add] https://crrev.com/0bb79bb7dba7b5f4b1837b3f579b19858974a3fb/build/config/compute_inputs_for_analyze.gni
[modify] https://crrev.com/0bb79bb7dba7b5f4b1837b3f579b19858974a3fb/build/print_python_deps.py
[modify] https://crrev.com/0bb79bb7dba7b5f4b1837b3f579b19858974a3fb/chrome/android/BUILD.gn
[modify] https://crrev.com/0bb79bb7dba7b5f4b1837b3f579b19858974a3fb/tools/grit/grit_rule.gni
[modify] https://crrev.com/0bb79bb7dba7b5f4b1837b3f579b19858974a3fb/tools/mb/mb.py
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 27 2018 

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

commit ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c
Author: Andrew Grieve <agrieve@chromium.org>
Date: Wed Jun 27 16:52:05 2018

Android: Properly track GN .py inputs for actions in internal_rules.gni

* Most templates use a hard-coded list of build_utils.py deps.
* A few use exec_script() when compute_inputs_for_analyze = true.

Bug:  843562 
Change-Id: I0bb1ca7927f7366d23c23b8beb56d39a22208a06
Reviewed-on: https://chromium-review.googlesource.com/1114122
Commit-Queue: agrieve <agrieve@chromium.org>
Reviewed-by: Eric Stevenson <estevenson@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570811}
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/apkbuilder.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/bytecode_processor.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/copy_ex.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/create_java_binary_script.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/create_stack_script.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/create_test_runner_script.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/desugar.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/dex.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/emma_instr.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/lint.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/main_dex_list.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/merge_jar_info_files.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/merge_manifest.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/proguard.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/util/build_utils.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/android/gyp/write_build_config.py
[modify] https://crrev.com/ce84d1c65eee9c5eaaff45db0a4ee0642c2f613c/build/config/android/internal_rules.gni

Comment 4 by agrieve@chromium.org, Jul 5

Owner: estevenson@chromium.org
This is now done for actions in internal_rules.gni. Still need to audit rules.gni

Comment 5 by agrieve@chromium.org, Jul 10

Cc: digit@chromium.org

Comment 6 by wnwen@chromium.org, Jul 12

Blocking: 861762
Cc: estevenson@chromium.org
Owner: wnwen@chromium.org
Status: Assigned (was: Available)
Will take this one

Comment 7 by wnwen@chromium.org, Jul 12 

Just saw digit@ is already doing this with one approach: https://crrev.com/c/1131190

Comment 8 by wnwen@chromium.org, Jul 12

Cc: -digit@chromium.org wnwen@chromium.org
Owner: digit@chromium.org
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 12 

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

commit 001fc64fc2945bb7b18e6ffb20b6048c007ac6b8
Author: Peter Wen <wnwen@chromium.org>
Date: Thu Jul 12 17:05:09 2018

Android: Fix jinja template deps

Temporarily add missing deps to the depfile. Will no longer be necessary
once all downstream targets are fixed.

Bug:  843562 
Change-Id: Id797ea032a6a3818dac6013dcacb6f68baef2b6b
Reviewed-on: https://chromium-review.googlesource.com/1134884
Commit-Queue: agrieve <agrieve@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574607}
[modify] https://crrev.com/001fc64fc2945bb7b18e6ffb20b6048c007ac6b8/build/android/gyp/jinja_template.py
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 13 

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

commit 4b401aa986f4c8cda198ad6f04afee4bac64d5b4
Author: Peter Wen <wnwen@chromium.org>
Date: Fri Jul 13 15:01:53 2018

Android: Permanent fix for jinja templates

Follow-up to https://crrev.com/c/1134884 now that all jinja_template
targets have proper includes.

Bug:  843562 
Change-Id: Ibff629fe42644a21a4c9b1fe94a0e5e822168936
Reviewed-on: https://chromium-review.googlesource.com/1135711
Commit-Queue: agrieve <agrieve@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574918}
[modify] https://crrev.com/4b401aa986f4c8cda198ad6f04afee4bac64d5b4/build/android/gyp/jinja_template.py
[modify] https://crrev.com/4b401aa986f4c8cda198ad6f04afee4bac64d5b4/chrome/android/BUILD.gn
Project Member

Comment 11 by bugdroid1@chromium.org, Aug 7 

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

commit 0006f4737647ad0790431737586fd700085f8b0e
Author: David 'Digit' Turner <digit@google.com>
Date: Tue Aug 07 07:12:36 2018

android: build: Use .pydeps file to store Python dependencies.

NOTE: This is a reland of the following CL which was reverted:

  https://chromium-review.googlesource.com/c/chromium/src/+/1131190

The revert was due to a problem with an internal clank/DEPS runhooks
step that calls jinja_template.py (see BUG entry below for details).
The new CL was checking inputs by defaults, which would raise an
error during gclient sync.

To ease review, the first CL on gerrit is the original one,
unmodified, and the second fixes the issue.

---- end of NOTE ------

ProTip: start by looking at the action_with_pydefps() definition
in internal_rules.gni to review changes in this CL.

This CL tries to make "gn analyze" smarter by ensuring that
it knows about Python module dependencies, for any python
script under //build.

At the moment, these dependencies are computed by
build_utils.WriteDepFile() automatically (unless the
add_pydeps=False argument is passed). This adds all imported
module paths to the generated .d file written by the script.

Unfortunately, because this .d file is generated by the
actions, the corresponding Python sources are not part of
the target's inputs, known by GN at parse time, and thus do
not appear in "gn analyze" properly.

This CL tries to solve the problem by adding a new template
named "action_with_pydeps", which acts as "action", but also
expects a foo.pydeps file for every foo.py invoked through
the 'script' scope variable.

This '.pydeps' file is read directly at GN parse time and
added to the action's target inputs.

These .pydeps contain the path of each imported module, one
per line, and are generated with build/print_python_deps.py

Benchmarking shows no significant difference in the time
taken to perform a "gn gen out/Release" on a full Chrome
for Android checkout.

This also needs a PRESUBMIT.py step that ensures that all .pydeps
files are up-to-date with regards to their corresponding .py source
files (this can be done with 'gen_pydeps.py --check .../foo.py')

+ Remove some --depfile options on a few Python scripts that
  don't need it anymore (e.g. when their inputs and outputs
  are now fully known by GN at parse time).

+ Remove uses of compute_inputs_for_analyze in rules.gni and
  internal_rules.gni (as well as exec_script() calls to
  print_python_deps.py). Note that the variable itself is still
  needed for grit inputs, see tools/grit/grit_rule.gni for
  details).

BUG=870845, 843562 
R=agrieve@chromium.org, estevenson@chromium.org

Change-Id: Id1f606a0c9df9e4e8971fd885ac0394103ca7b03
Reviewed-on: https://chromium-review.googlesource.com/1163512
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Eric Stevenson <estevenson@chromium.org>
Commit-Queue: David Turner <digit@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581158}
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/PRESUBMIT.py
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/base/android/jni_generator/jni_generator.py
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/base/android/jni_generator/jni_generator.pydeps
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/base/android/jni_generator/jni_registration_generator.py
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/base/android/jni_generator/jni_registration_generator.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/aar.pydeps
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/aidl.py
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/aidl.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/apkbuilder.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/app_bundle_to_apks.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/bytecode_processor.pydeps
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/compile_resources.py
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/compile_resources.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/copy_ex.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/create_apk_operations_script.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/create_app_bundle.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/create_bundle_wrapper_script.pydeps
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/create_dist_jar.py
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/create_dist_jar.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/create_java_binary_script.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/create_stack_script.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/create_test_runner_script.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/create_tool_wrapper.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/desugar.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/dex.pydeps
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/dist_aar.py
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/dist_aar.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/emma_instr.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/filter_zip.pydeps
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/gcc_preprocess.py
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/gcc_preprocess.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/generate_proguarded_module_jar.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/ijar.pydeps
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/java_cpp_enum.py
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/java_cpp_enum.pydeps
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/javac.py
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/javac.pydeps
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/jinja_template.py
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/jinja_template.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/lint.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/main_dex_list.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/merge_jar_info_files.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/merge_manifest.pydeps
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/prepare_resources.py
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/prepare_resources.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/proguard.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/write_build_config.pydeps
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/write_ordered_libraries.py
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/gyp/write_ordered_libraries.pydeps
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/incremental_install/generate_android_manifest.pydeps
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/incremental_install/write_installer_json.py
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/android/incremental_install/write_installer_json.pydeps
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/config/android/internal_rules.gni
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/config/android/rules.gni
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/config/python.gni
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/print_python_deps.py
[modify] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/protoc_java.py
[add] https://crrev.com/0006f4737647ad0790431737586fd700085f8b0e/build/protoc_java.pydeps

Comment 12 by digit@google.com, Aug 7

Status: Fixed (was: Assigned)
►

Sign in to add a comment