Skip to content

Commit

Permalink
ppapi: add generated files instead of a generated directory.
Browse files Browse the repository at this point in the history
Including directories full of generated files causes incorrect incremental
builds.

But using data_deps makes it easy to copy just the generated files here,
so just do that instead.

To make sure this does the right thing, I ran

  gn gen out/gn --runtime-deps-list-file=<(echo //chrome/test:browser_tests)

with and without this patch and compared out/gn/browser_tests.runtime_deps.

The diff is

-test_case.html
-test_case.html.mock-http-headers
-test_page.css
-test_page.css.mock-http-headers
-test_url_loader_data/
+corb_eligible_resource.json
+corb_eligible_resource.json.mock-http-headers
+test_case.html
+test_case.html.mock-http-headers
+test_page.css
+test_page.css.mock-http-headers
+test_url_loader_data/hello.txt
-corb_eligible_resource.json
-corb_eligible_resource.json.mock-http-headers
-test_case.html
-test_case.html.mock-http-headers
-test_page.css
-test_page.css.mock-http-headers


Importantly, `test_url_loader_data/` no longer appears. (For some reason,
the non-directories were listed twice previously.)

We now bundle corb_eligible_resource when we weren't previously,
but that seems fine.

I then downloaded an isolate from the command printed on a browser_tests
run on swarming like so:

  tools/swarming_client/isolateserver.py download -I https://isolateserver.appspot.com \
      --namespace default-gzip -s 60fe18ca29dd47b47425717c01e67516cc26d6bf \
      --target foo

and verified that these are in fact all files:

    $ ls foo/out/Release/test_url_loader_data/
    hello.txt

Bug: 912946
Change-Id: I1ad01f8b53a1eb47c5b572ad645b1901305fcbc4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1769213
Reviewed-by: Bill Budge <bbudge@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690946}
  • Loading branch information
nico authored and Commit Bot committed Aug 28, 2019
1 parent 3da4ce0 commit 65e046d
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 9 deletions.
10 changes: 5 additions & 5 deletions chrome/test/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,11 @@ if (!is_android) {
# Runtime dependencies
data_deps = [
"//chrome/browser/resources/media/mei_preload:component",

# TODO(thakis): Why do these need copying in browser_tests?
# content_browsertests uses the non-copied files instead.
"//ppapi:copy_test_files",
"//ppapi:copy_test_files2",
"//ppapi:ppapi_tests",
"//ppapi:power_saver_test_plugin",
"//remoting/webapp:browser_test_resources",
Expand Down Expand Up @@ -761,11 +766,6 @@ if (!is_android) {
"//third_party/tlslite/",
"//ui/webui/resources/",
"$root_out_dir/browser_tests.pak",
"$root_out_dir/test_case.html",
"$root_out_dir/test_case.html.mock-http-headers",
"$root_out_dir/test_page.css",
"$root_out_dir/test_page.css.mock-http-headers",
"$root_out_dir/test_url_loader_data/",
]

sources = [
Expand Down
2 changes: 0 additions & 2 deletions ppapi/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ test_common_source_files = [
]

copy("copy_test_files") {
visibility = [ ":*" ]
sources = [
# Keep "test_case.html.mock-http-headers" with "test_case.html"
# and "corb_eligible_resource.json.mock-http-headers" with
Expand All @@ -163,7 +162,6 @@ copy("copy_test_files") {
}

copy("copy_test_files2") {
visibility = [ ":*" ]
sources = [
"tests/test_url_loader_data/hello.txt",
]
Expand Down
3 changes: 1 addition & 2 deletions tools/mb/mb.py
Original file line number Diff line number Diff line change
Expand Up @@ -1102,8 +1102,7 @@ def WriteIsolateFiles(self, build_dir, command, target, runtime_deps, vals,
f == 'mr_extension/' or # https://crbug.com/997947
f == 'locales/' or
f.startswith('nacl_test_data/') or
f.startswith('ppapi_nacl_tests_libs/') or
f == 'test_url_loader_data/'):
f.startswith('ppapi_nacl_tests_libs/')):
continue

# This runs before the build, so we can't use isdir(f). But
Expand Down

0 comments on commit 65e046d

Please sign in to comment.