GN: change the way we specify dependencies for the patched_sdk target.

GN: change the way we specify dependencies for the patched_sdk target.

Instead of listing all files as inputs in GN script using Python helper we make
patch_sdk.dart generate a depfile that Ninja can use:

            https://ninja-build.org/manual.html#_depfile

This prevents breakages when some dependencies are deleted and GN is not rerun.

R=ahe@google.com, kustermann@google.com
BUG=

Committed: https://github.com/dart-lang/sdk/commit/c0c87b0afdad33b5fcb5a9543f7ba6db0ab27da1

[kustermann, 2017-03-03 13:23:08]

LGTM

https://codereview.chromium.org/2729503006/diff/1/tools/patch_sdk.dart
File tools/patch_sdk.dart (right):

https://codereview.chromium.org/2729503006/diff/1/tools/patch_sdk.dart#newcode21
tools/patch_sdk.dart:21: import
'package:front_end/src/fasta/compiler_command_line.dart' show
CompilerCommandLine;
nit: long line

https://codereview.chromium.org/2729503006/diff/1/tools/patch_sdk.dart#newcode44
tools/patch_sdk.dart:44: deps.add(new File(path).absolute.path);
deps.add(file.absolute.path);

https://codereview.chromium.org/2729503006/diff/1/tools/patch_sdk.dart#newcode89
tools/patch_sdk.dart:89: libContents = libContents.replaceAll(
readInputFile has canBeMissing: true so we could do a null dereference. Maybe
set it to `true`.

https://codereview.chromium.org/2729503006/diff/1/tools/patch_sdk.dart#newcod...
tools/patch_sdk.dart:148: var partFile =
getInputFile(path.join(path.dirname(libraryIn), partPath));
run dartfmt?

https://codereview.chromium.org/2729503006/diff/1/tools/patch_sdk.dart#newcod...
tools/patch_sdk.dart:257: deps.addAll(list[1].split(' ').where((str) =>
str.isNotEmpty));
maybe assert list[0] == 'platform.dill'

Could you ensure we run this file in checked mode?

[Vyacheslav Egorov (Google), 2017-03-03 13:49:14]

vegorov@google.com changed reviewers:
+ ahe@google.com

[Vyacheslav Egorov (Google), 2017-03-03 13:49:15]

Hi Peter,

Could you take a look at a change to outline.dart I made: I need outline to
respect --packages options for patch_sdk to be able to run.

Cheers,

https://codereview.chromium.org/2729503006/diff/1/tools/patch_sdk.dart
File tools/patch_sdk.dart (right):

https://codereview.chromium.org/2729503006/diff/1/tools/patch_sdk.dart#newcode21
tools/patch_sdk.dart:21: import
'package:front_end/src/fasta/compiler_command_line.dart' show
CompilerCommandLine;
On 2017/03/03 13:23:08, kustermann wrote:
> nit: long line

Done.

https://codereview.chromium.org/2729503006/diff/1/tools/patch_sdk.dart#newcode44
tools/patch_sdk.dart:44: deps.add(new File(path).absolute.path);
On 2017/03/03 13:23:08, kustermann wrote:
> deps.add(file.absolute.path);

Done.

https://codereview.chromium.org/2729503006/diff/1/tools/patch_sdk.dart#newcode89
tools/patch_sdk.dart:89: libContents = libContents.replaceAll(
On 2017/03/03 13:23:08, kustermann wrote:
> readInputFile has canBeMissing: true so we could do a null dereference. Maybe
> set it to `true`.

canBeMissing should be false by default

https://codereview.chromium.org/2729503006/diff/1/tools/patch_sdk.dart#newcod...
tools/patch_sdk.dart:148: var partFile =
getInputFile(path.join(path.dirname(libraryIn), partPath));
On 2017/03/03 13:23:08, kustermann wrote:
> run dartfmt?

Done.

[ahe, 2017-03-03 13:51:40]

Changes to outline.dart, LGTM!

Thank you.

[Vyacheslav Egorov (Google), 2017-03-03 13:52:54]

Description was changed from

==========
GN: change the way we specify dependencies for the patched_sdk target.

Instead of listing all files as inputs in GN script using Python helper we make
patch_sdk.dart generate a depfile that Ninja can use:

            https://ninja-build.org/manual.html#_depfile

This prevents breakages when some dependencies are deleted and GN is not rerun.

R=kustermann@google.com
BUG=
==========

to

==========
GN: change the way we specify dependencies for the patched_sdk target.

Instead of listing all files as inputs in GN script using Python helper we make
patch_sdk.dart generate a depfile that Ninja can use:

            https://ninja-build.org/manual.html#_depfile

This prevents breakages when some dependencies are deleted and GN is not rerun.

R=ahe@google.com, kustermann@google.com
BUG=

Committed:
https://github.com/dart-lang/sdk/commit/c0c87b0afdad33b5fcb5a9543f7ba6db0ab27da1
==========

[Vyacheslav Egorov (Google), 2017-03-03 13:52:55]

Committed patchset #2 (id:20001) manually as
c0c87b0afdad33b5fcb5a9543f7ba6db0ab27da1 (presubmit successful).

[jamesr1, 2017-03-25 01:14:40]

jamesr@google.com changed reviewers:
+ jamesr@google.com

[jamesr1, 2017-03-25 01:14:41]

This appears to be slightly wrong when cross-compiling.  In the Fuchsia build,
which runs this step for both the target and host, the depsfile emitted for the
host claims that it is producing the output for the target build.  In the
Fuchsia build the root of the host (on an x86-64 platform) is at $out/host_x64/
and the depsfile at $out/host_x64/patched_sdk.d starts with:

patched_sdk/platform.dill: /ssd/fuchsia/dart/tools/patch_sdk.dart
/ssd/fuchsia/dart/pkg/analyzer/lib/analyzer.dart ...

This means ninja thinks this edge is dirty on every build and it thus re-runs
the step.  The output of ninja -d explain looks like:

ninja explain: expected depfile 'host_x64/patched_sdk.d' to mention
'host_x64/patched_sdk/platform.dill', got 'patched_sdk/platform.dill'

To fix this, patched_sdk.dart should take into account the fact that the path in
depsfiles is relative to the absolute root of the build and not the root of the
current toolchain's build (i.e. the difference between root_out_dir vs
root_build_dir).

[jamesr1, 2017-03-27 21:26:50]

Looks like the logic in
https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/src/fasta/kern...
is wrong - it assumes the first entry (the 'output') in a depsfile should be
specified relative to the depsfile but ninja interprets this entry relative to
the CWD of the build, i.e. the root of the whole build. These differ when doing
cross-compilation.

[jamesr1, 2017-03-27 21:41:23]

Patch uploaded: https://codereview.chromium.org/2776323002