gn desc runtime_deps is wrong for bundled targets |
||||
Issue descriptiontools/mb/mb.py uses `gn desc [out] [label] runtime_deps` to find files to isolate for swarming. But the runtime_deps command does not properly handle create_bundle targets. There are two issues: - The bundle directory is not listed as a dependency. - The search for runtime_deps recurses into the targets that make up the bundle itself. This causes the intermediate shared_library/executable output to be listed instead. As an example: If I specify //chrome:chrome_framework as a data_deps on //chrome/test:browser_tests, the "Chromium Framework.framework" is not outputted as a runtime_deps. The only thing that is listed is the intermediate library "obj/chrome/chrome_framework_shared_library/Chromium Framework". That intermediate should not be listed. I think an appropriate fix would be to: 1) Emit the bundle's root directory as a runtime_deps (similar to what was done for outputs here https://codereview.chromium.org/1892243002/). 2) Stop traversing dependencies if the target is a create_bundle. The search for runtime_deps should not traverse into a create_bundle's deps or public_deps. I think 2) is appropriate, since if a bundle has any dependencies, it should really just be included in the bundle itself.
,
May 12 2016
,
May 13 2016
fwiw, we've had a similar issue w/ APKs on Android. I think some generic mechanism to tell GN that a target is (or will be) self-contained and doesn't need to be recursed into is a good idea. +agrieve for related thoughts ...
,
May 13 2016
Does Android use create_bundle for APKs? Should it? It seems pretty analogous to Mac bundles. CL: https://codereview.chromium.org/1982463002/
,
May 13 2016
No, it doesn't use create_bundle now. It is close, but it's not quite the same. I hope to figure out how to consolidate much of this logic (including how we handle isolates and some of the things we do for tarring up packages like for blimp), but we were going around in circles trying to work out how to do that a couple months ago, and so it's better to do independent things for the moment.
,
May 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/903e2b85c71675f83767ac9b1687087382b49f39 commit 903e2b85c71675f83767ac9b1687087382b49f39 Author: rsesek <rsesek@chromium.org> Date: Thu May 19 19:43:32 2016 [Mac/GN] Treat create_bundle as a leaf for `gn desc runtime_deps`. BUG= 611414 R=brettw@chromium.org,dpranke@chromium.org,sdefresne@chromium.org Review-Url: https://codereview.chromium.org/1982463002 Cr-Commit-Position: refs/heads/master@{#394840} [modify] https://crrev.com/903e2b85c71675f83767ac9b1687087382b49f39/tools/gn/bundle_data.cc [modify] https://crrev.com/903e2b85c71675f83767ac9b1687087382b49f39/tools/gn/bundle_data.h [modify] https://crrev.com/903e2b85c71675f83767ac9b1687087382b49f39/tools/gn/runtime_deps.cc [modify] https://crrev.com/903e2b85c71675f83767ac9b1687087382b49f39/tools/gn/runtime_deps_unittest.cc
,
May 20 2016
The android_resources() and android_assets() rules do also have the concept of leaf nodes - android_apk() being the main one. I do like the idea of having an x-platform way of specifying "I'd like these files available at runtime", but Android Assets being contained within the .apk means the C++ side would need a custom x-platform API for getting at them. I'd love to have this, but it's work no one's taken on yet.
,
May 31 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/be4da4cd67c068e11bd226979faf1f578dae5a3f commit be4da4cd67c068e11bd226979faf1f578dae5a3f Author: rsesek <rsesek@chromium.org> Date: Tue May 31 20:10:49 2016 [Mac/GN] Collect a bundle's data_deps when using `gn desc runtime_deps`. In 903e2b85c71675f83767ac9b1687087382b49f39, GN stopped recursing into bundles to collect target dependencies. But any explicit data_deps on a bundle should be included in the runtime_deps output. BUG= 611414 R=brettw@chromium.org,sdefresne@chromium.org Review-Url: https://codereview.chromium.org/2010173004 Cr-Commit-Position: refs/heads/master@{#396910} [modify] https://crrev.com/be4da4cd67c068e11bd226979faf1f578dae5a3f/tools/gn/runtime_deps.cc [modify] https://crrev.com/be4da4cd67c068e11bd226979faf1f578dae5a3f/tools/gn/runtime_deps_unittest.cc
,
Jun 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/57979207f98a847a40a8f5b57633cf7427c68c28 commit 57979207f98a847a40a8f5b57633cf7427c68c28 Author: rsesek <rsesek@chromium.org> Date: Wed Jun 01 21:40:35 2016 [Mac/iOS/GN] Document the new rules for create_bundle and runtime_deps. BUG= 611414 R=dpranke@chromium.org Review-Url: https://codereview.chromium.org/2023003006 Cr-Commit-Position: refs/heads/master@{#397258} [modify] https://crrev.com/57979207f98a847a40a8f5b57633cf7427c68c28/tools/gn/docs/reference.md [modify] https://crrev.com/57979207f98a847a40a8f5b57633cf7427c68c28/tools/gn/functions_target.cc [modify] https://crrev.com/57979207f98a847a40a8f5b57633cf7427c68c28/tools/gn/variables.cc
,
Jun 1 2016
I'm going to call this Fixed, since Android doesn't seem applicable here, given that it doesn't use create_bundle.
,
Jun 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ff583c8b9182fc8fde41d2ebed27dc49ed92745a commit ff583c8b9182fc8fde41d2ebed27dc49ed92745a Author: Robert Sesek <rsesek@chromium.org> Date: Tue Jun 13 20:15:33 2017 Remove explicit data entries for bundled targets in //chrome/test:browser_tests. Bundled targets specified as data_deps used to not be collected as runtime_deps, but that was fixed in https://crbug.com/611414 . Listing these as explicit data is no longer necessary, and the hard-coded "Chromium" names are wrong when swarming official build tests. Bug: 611414 Bug: 731290 Change-Id: I3cffbc1b13552197524d91be85b49b281116020a Reviewed-on: https://chromium-review.googlesource.com/533754 Reviewed-by: Michael Moss <mmoss@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Commit-Queue: Robert Sesek <rsesek@chromium.org> Cr-Commit-Position: refs/heads/master@{#479119} [modify] https://crrev.com/ff583c8b9182fc8fde41d2ebed27dc49ed92745a/chrome/test/BUILD.gn |
||||
►
Sign in to add a comment |
||||
Comment 1 by rsesek@chromium.org
, May 12 2016