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

Issue 593416 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Apr 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocked on:
issue 603143



Sign in to add a comment

Generated .isolate files for Android Swarming Contain Unused Files

Project Member Reported by agrieve@chromium.org, Mar 9 2016

Issue description

gn gen --args='target_os="android"' out/Debug
tools/mb/mb.py isolate //out/Debug android_webview_unittests
cat out/Debug/android_webview_unittests.isolate


Files that I think are not needed:
*.mojom.js
*lint/config.xml
*lint/result.xml
*.so

Files that may or may not be:
pyproto/* (included via //third_party/protobuf:py_proto. Needed for host-driven tests?)
 
Cc: brettw@chromium.org
Labels: Build-Tools-GN
the *.mojom.js files are probably getting picked up transitively from something in content, and are potentially needed for running the layout_tests on android (via content_shell, not webapk).

There's not a good way to say `dependency X needs this file but dependency Y doesn't`, so you are probably stuck with them in this case.

I have no idea about the *lint files.

Do we not need the *.so files because they are also bundled inside the apk ?

Adding the Build-Tools-GN label because we might end up needing to talk about more changes for managing data/data_deps propagation.
*lint/ files are only used at build time and should not be necessary.

re *.so files: exactly, they're in the apk.
Given that these are used largely for test binaries, it's possible this may be more trouble than it's worth to fix the .so problem ...
How do we get around this in the GYP/static isolates? Are the .so's excluded in Android component builds explicitly or something?
Another thought here:
Since the files to push to device are a strict subset of the files to push to swarming, it might make sense to have:

unittest_apk() {
  data = files_to_push_to_device
}

generate_test_script() {
  data = files_required_by_test_runner
  data_deps = unittest_apk
}

This was we can generate both .isolate files via gn desc runtime_deps.

This is assuming we can get the data of unittest_apk to not include extra things.
Cc: thakis@chromium.org sdefresne@chromium.org
In thinking about this more (given thakis's questions on the //base change in https://codereview.chromium.org/1784273003/ , and  bug 589318 ), I realized we have another problem.

In order to push the files to the device, we need the list of files in files_to_push_to_device . Currently we get that from the checked-in isolate. 

If we get rid of the checked-in isolate, we have to recreate the file. Currently the way we create the files for swarming is to generate them *after* we run the build. However, in order for the list of files to be included in the (swarming) isolate, it needs to be listed as a data dependency *in* the build, which isn't currently possible.

In addition, we need files_to_push_to_device to *always* exist in a build, else you can't run the tests even in the non-swarming case.

I talked to brettw@ about this briefly. We think we probably end up needing a dump_runtime_deps_to_file() built-in function in order to handle this, and then we can update the test scripts to take a list of files rather than parse the isolate format.

It's possible (although not entirely clear to me yet) that there's also a fair amount of overlap between this problem and the bundle_data()/create_bundle() code we just added for iOS.

We should work out the rest of the issues with Android data deps, isolates, apks, etc. and come up with a solution for them all at once, and then we can see if we can find commonality between android, ios, and desktop data_deps such that we can refactor/simplify things again.


Yet another option (which I'm now leaning towards): Maybe we should just get rid of having side-loaded files for android tests entirely, and instead package the needed files as Android assets so that they are embedded in the .apk.

This would mean needing to define android_assets() targets for all of the tests, as well as updating the code to load it in the correct way. Perhaps the android_assets could be implicitly declared by test.gni, so that for non-Android the test data can be declared as data, for android, declared as android_assets, and for ios declared as bundle_data().
FWIW, my plan for iOS is to have the "test" template create an application bundle on iOS. This will use the bundle_data/create_bundle. Those two targets are marketed as iOS/OS X generic, but unless there are files named *.xcassets/*.imagefile/* then I think you can reuse them for Android too to generate *.apk.
After even more thought (my brain hurts), I've come to the same conclusion as in #6. Additionally, I think that for now we should just work-around the primitive not existing by running gn desc at build-time. Example:
https://codereview.chromium.org/1805643002
Looked into the file having so many things that are unnecessary. They fall into 3 buckets:
1. .so files that GN implicitly decides are data
2. data files (such as icudtl.dat) that are packed into the android .apk but are declared as data. These could probably be if(android)'ed so as to not appear
3. lint resultes.

The lint results I'm not sure how to fix. They are declared data_deps() in order have them build in parallel to other targets. GN has no other way to declare this though (e.g. validation_deps)

I have an example CL which just takes in the list of runtime_deps and filters out #1, #2, and #3: https://codereview.chromium.org/1805643002/


Project Member

Comment 11 by bugdroid1@chromium.org, Mar 31 2016

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

commit 0f8ad42ac1a6b159655c51bf7095ff2d21ae540f
Author: agrieve <agrieve@chromium.org>
Date: Thu Mar 31 22:00:07 2016

GN: Add write_runtime_deps variable

Allows targets to use runtime_deps at build-time.

BUG= 593416 

Review URL: https://codereview.chromium.org/1804303004

Cr-Commit-Position: refs/heads/master@{#384411}

[modify] https://crrev.com/0f8ad42ac1a6b159655c51bf7095ff2d21ae540f/tools/gn/BUILD.gn
[add] https://crrev.com/0f8ad42ac1a6b159655c51bf7095ff2d21ae540f/tools/gn/group_target_generator_unittest.cc
[modify] https://crrev.com/0f8ad42ac1a6b159655c51bf7095ff2d21ae540f/tools/gn/runtime_deps.cc
[modify] https://crrev.com/0f8ad42ac1a6b159655c51bf7095ff2d21ae540f/tools/gn/scheduler.cc
[modify] https://crrev.com/0f8ad42ac1a6b159655c51bf7095ff2d21ae540f/tools/gn/scheduler.h
[modify] https://crrev.com/0f8ad42ac1a6b159655c51bf7095ff2d21ae540f/tools/gn/target.cc
[modify] https://crrev.com/0f8ad42ac1a6b159655c51bf7095ff2d21ae540f/tools/gn/target.h
[modify] https://crrev.com/0f8ad42ac1a6b159655c51bf7095ff2d21ae540f/tools/gn/target_generator.cc
[modify] https://crrev.com/0f8ad42ac1a6b159655c51bf7095ff2d21ae540f/tools/gn/target_generator.h
[modify] https://crrev.com/0f8ad42ac1a6b159655c51bf7095ff2d21ae540f/tools/gn/target_unittest.cc
[modify] https://crrev.com/0f8ad42ac1a6b159655c51bf7095ff2d21ae540f/tools/gn/variables.cc
[modify] https://crrev.com/0f8ad42ac1a6b159655c51bf7095ff2d21ae540f/tools/gn/variables.h

Project Member

Comment 12 by bugdroid1@chromium.org, Apr 12 2016

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

commit 9b7e031dd50a0381dc25d5c487b8631f886bf079
Author: agrieve <agrieve@chromium.org>
Date: Tue Apr 12 14:22:49 2016

Stop including android lint results in runtime_deps

BUG= 593416 

Review URL: https://codereview.chromium.org/1876823003

Cr-Commit-Position: refs/heads/master@{#386679}

[modify] https://crrev.com/9b7e031dd50a0381dc25d5c487b8631f886bf079/build/config/android/internal_rules.gni

Blockedon: 603143
Figured out that is_asan=true adds more unwanted things:

../../tools/valgrind/asan/
../../third_party/llvm-build/Release+Asserts/bin/llvm-symbolizer

And also some more:
pyproto/google/__init__.py
pyproto/google/protobuf/__init__.py
pyproto/google/protobuf/descriptor.py
pyproto/google/protobuf/descriptor_database.py
pyproto/google/protobuf/descriptor_pool.py
pyproto/google/protobuf/message.py
pyproto/google/protobuf/message_factory.py
pyproto/google/protobuf/reflection.py
pyproto/google/protobuf/service.py
pyproto/google/protobuf/service_reflection.py
pyproto/google/protobuf/text_format.py
pyproto/google/protobuf/descriptor_pb2.py
pyproto/google/protobuf/internal/__init__.py
pyproto/google/protobuf/internal/api_implementation.py
pyproto/google/protobuf/internal/containers.py
pyproto/google/protobuf/internal/cpp_message.py
pyproto/google/protobuf/internal/decoder.py
pyproto/google/protobuf/internal/encoder.py
pyproto/google/protobuf/internal/enum_type_wrapper.py
pyproto/google/protobuf/internal/generator_test.py
pyproto/google/protobuf/internal/message_listener.py
pyproto/google/protobuf/internal/python_message.py
pyproto/google/protobuf/internal/type_checkers.py
pyproto/google/protobuf/internal/wire_format.py
Components: -Test Build
Labels: -Build-Tools-GN -Proj-GN-Migration
While we should fix this, I don't think this blocks the GN migration.
The pyproto files are due to net_testsupport.

There is also now:
Mojo Applications/catalog/manifest.json,

which is being pulled in through //content:resources_grit
The pyproto files are due to net_testsupport.

There is also now:
Mojo Applications/catalog/manifest.json,

which is being pulled in through //services/catalog:lib source_set
Status: Fixed (was: Available)
This is probably "fixed enough". The generator script has some blacklists for removing most unwanted files.

Sign in to add a comment