New issue
Advanced search Search tips

Issue 912946 link

Starred by 2 users
Status: Assigned
Owner:
thakis@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 911761
issue 919437
Blocking:
issue 869348
issue 899438



Sign in to add a comment

Don't allow data and data_deps listing generated directories

Project Member Reported by thakis@chromium.org, Dec 7 
data and data_deps allow listing directories. That's so far ok.

However, if a directory is in the build out dir, then that's a problem: Consider a process that copies a bunch of files into the build out dir. Later, it copies fewer files. Now, incremental bots will still have the now-removed file sitting around, and due to the listed directory, the stale file will be zipped up and sent to swarming.

That's bad for correctness, for build determinism, and for swarming cache rate (since otherwise identical builds now sometimes include that stale file and sometimes not).

Here's a list of all the places where we put generated dirs in data_deps: https://cs.chromium.org/search/?q=%5Cbdata%5B%5E%5D%5D*%5C%22%5C$%5B%5E%22%5D*%5C/%5C%22+file:%5C.gn+pcre:yes&sq=package:chromium&type=cs

It's not that many places. It looks feasible to make all those just explicitly list files and then forbid having dirs below generated dirs in data_deps.
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 7 

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

commit d68711c24f85ede732f91d640064f1156c84ca94
Author: Nico Weber <thakis@chromium.org>
Date: Fri Dec 07 16:40:56 2018

Remove a redundant data entry.

This entry is already present further down for !is_android && !is_mac,
so no need to add it in a is_chromeos-specific block.

No intended behavior change.

Bug: 912946
Change-Id: I232b056a7e72271e6ecb81243dc79193cfbb751d
Reviewed-on: https://chromium-review.googlesource.com/c/1368045
Reviewed-by: Hans Wennborg <hans@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614723}
[modify] https://crrev.com/d68711c24f85ede732f91d640064f1156c84ca94/chrome/test/BUILD.gn
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 11 

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

commit bd9b00b3d439110133d7d434f81504ef8c7eb61d
Author: Nico Weber <thakis@chromium.org>
Date: Tue Dec 11 18:47:28 2018

Remove explicit data dep on $root_out_dir/remoting/unittests/ directory.

Listing a generated directory in the isolate means that incremental
builders (and devs doing incremental builds) can have stale files in
their generated isolated files.

Since we already have data_deps on all these copy steps, the isolate
already contains an explicit list of all files in that directory, and
having a data line for the directory is not necessary. Just remove it.

Bug: 912946
Change-Id: Id7232369905fb651227d5e913247271dd39a4211
Reviewed-on: https://chromium-review.googlesource.com/c/1368584
Reviewed-by: Joe Downing <joedow@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615606}
[modify] https://crrev.com/bd9b00b3d439110133d7d434f81504ef8c7eb61d/chrome/test/BUILD.gn
[modify] https://crrev.com/bd9b00b3d439110133d7d434f81504ef8c7eb61d/remoting/webapp/files.gni

Comment 3 by thakis@chromium.org, Jan 7

Blockedon: 919437

Comment 4 by thakis@chromium.org, Jan 7

Blocking: 899438
►

Sign in to add a comment