[GN] Don't rewrite files with the same contents

[GN] Don't rewrite files with the same contents

BUG=

Committed: https://crrev.com/f8ea5cceefcedd4a01935d5ac4d2ba71e23ac13e
Cr-Commit-Position: refs/heads/master@{#373544}

[Tomasz Moniuszko, 2016-02-02 14:29:11]

tmoniuszko@opera.com changed reviewers:
+ brettw@chromium.org

[Tomasz Moniuszko, 2016-02-02 14:29:12]

This patch introduces ContentEquals() method. It's used by RunWriteFile() as
suggested in https://codereview.chromium.org/1570113002/.

Using WriteFileIfChanged() also in NinjaTargetWriter descreased ninja files
generation time by nearly 10% on my machine (from about 3750ms to 3400ms).

[brettw, 2016-02-02 20:50:23]

Thanks!

https://codereview.chromium.org/1656253003/diff/1/tools/gn/filesystem_utils.cc
File tools/gn/filesystem_utils.cc (right):

https://codereview.chromium.org/1656253003/diff/1/tools/gn/filesystem_utils.c...
tools/gn/filesystem_utils.cc:690: bool ContentsEqual(const base::FilePath&
file_path, std::stringstream* data) {
I'm thinking it might be better if this took a std::string as input so we can
avoid making a string twice out of the stringstream (these will be big strings
so we want to avoid duplicates)

https://codereview.chromium.org/1656253003/diff/1/tools/gn/function_write_fil...
File tools/gn/function_write_file.cc (right):

https://codereview.chromium.org/1656253003/diff/1/tools/gn/function_write_fil...
tools/gn/function_write_file.cc:149: // Write file, creating the directory if
necessary.
Can the rest of this function and DoWriteFile all be merged into
WriteFileIfChanged?

[Tomasz Moniuszko, 2016-02-03 10:20:30]

https://codereview.chromium.org/1656253003/diff/1/tools/gn/filesystem_utils.cc
File tools/gn/filesystem_utils.cc (right):

https://codereview.chromium.org/1656253003/diff/1/tools/gn/filesystem_utils.c...
tools/gn/filesystem_utils.cc:690: bool ContentsEqual(const base::FilePath&
file_path, std::stringstream* data) {
On 2016/02/02 20:50:23, brettw wrote:
> I'm thinking it might be better if this took a std::string as input so we can
> avoid making a string twice out of the stringstream (these will be big strings
> so we want to avoid duplicates)

Done.

https://codereview.chromium.org/1656253003/diff/1/tools/gn/function_write_fil...
File tools/gn/function_write_file.cc (right):

https://codereview.chromium.org/1656253003/diff/1/tools/gn/function_write_fil...
tools/gn/function_write_file.cc:149: // Write file, creating the directory if
necessary.
On 2016/02/02 20:50:23, brettw wrote:
> Can the rest of this function and DoWriteFile all be merged into
> WriteFileIfChanged?

Done.

[brettw, 2016-02-03 18:27:08]

lgtm

https://codereview.chromium.org/1656253003/diff/40001/tools/gn/function_write...
File tools/gn/function_write_file.cc (right):

https://codereview.chromium.org/1656253003/diff/40001/tools/gn/function_write...
tools/gn/function_write_file.cc:98: *err = Err(function->function(),
err->message());
Can you also add err->help_text() as the third parameter here so it doesn't get
lost?

[Tomasz Moniuszko, 2016-02-04 11:41:32]

https://codereview.chromium.org/1656253003/diff/40001/tools/gn/function_write...
File tools/gn/function_write_file.cc (right):

https://codereview.chromium.org/1656253003/diff/40001/tools/gn/function_write...
tools/gn/function_write_file.cc:98: *err = Err(function->function(),
err->message());
On 2016/02/03 18:27:07, brettw wrote:
> Can you also add err->help_text() as the third parameter here so it doesn't
get
> lost?

Done.

[Tomasz Moniuszko, 2016-02-04 11:41:39]

The CQ bit was checked by tmoniuszko@opera.com

[Tomasz Moniuszko, 2016-02-04 11:41:39]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1656253003/#ps60001
(title: "Fix review issue")

[commit-bot: I haz the power, 2016-02-04 11:42:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1656253003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1656253003/60001

[commit-bot: I haz the power, 2016-02-04 11:56:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-04 11:56:35]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Tomasz Moniuszko, 2016-02-04 15:16:32]

The CQ bit was checked by tmoniuszko@opera.com

[Tomasz Moniuszko, 2016-02-04 15:16:32]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1656253003/#ps80001
(title: "Fix failing test on Mac")

[commit-bot: I haz the power, 2016-02-04 15:17:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1656253003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1656253003/80001

[commit-bot: I haz the power, 2016-02-04 15:58:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-04 15:59:24]

Description was changed from

==========
[GN] Don't rewrite files with the same contents

BUG=
==========

to

==========
[GN] Don't rewrite files with the same contents

BUG=

Committed: https://crrev.com/f8ea5cceefcedd4a01935d5ac4d2ba71e23ac13e
Cr-Commit-Position: refs/heads/master@{#373544}
==========

[commit-bot: I haz the power, 2016-02-04 15:59:25]

Patchset 5 (id:??) landed as
https://crrev.com/f8ea5cceefcedd4a01935d5ac4d2ba71e23ac13e
Cr-Commit-Position: refs/heads/master@{#373544}

[please use gerrit instead, 2016-02-05 18:35:27]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1672783002/ by rouslan@chromium.org.

The reason for reverting is: Need to revert this patch according
https://sites.google.com/a/chromium.org/dev/developers/tree-sheriffs/sheriffi....
This patch added a flaky test FilesystemUtils.WriteFileIfChanged.

-----

It's a test flake:
1) Try to find the patch that caused the flake. It should be recent (e.g. last
day or two) in all likelihood.
2) If successful with finding that patch, revert the patch. This is especially
true if the flake is from a new test introduced in that patch.
3) Close the bug.

-----

The test has failed in the following builds:

http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

-----

Example failure:

[ RUN      ] FilesystemUtils.WriteFileIfChanged
../../tools/gn/filesystem_utils_unittest.cc:610: Failure
Expected: (last_modified) != (file_info.last_modified), actual: 2016-02-04
18:06:36.920 UTC vs 2016-02-04 18:06:36.920 UTC
[  FAILED  ] FilesystemUtils.WriteFileIfChanged (3 ms)
[315/315] FilesystemUtils.WriteFileIfChanged (3 ms)
Retrying 1 test (retry #2)
[ RUN      ] FilesystemUtils.WriteFileIfChanged
../../tools/gn/filesystem_utils_unittest.cc:610: Failure
Expected: (last_modified) != (file_info.last_modified), actual: 2016-02-04
18:06:36.936 UTC vs 2016-02-04 18:06:36.936 UTC
[  FAILED  ] FilesystemUtils.WriteFileIfChanged (2 ms)
[316/316] FilesystemUtils.WriteFileIfChanged (2 ms)
Retrying 1 test (retry #3)
[ RUN      ] FilesystemUtils.WriteFileIfChanged
../../tools/gn/filesystem_utils_unittest.cc:610: Failure
Expected: (last_modified) != (file_info.last_modified), actual: 2016-02-04
18:06:36.952 UTC vs 2016-02-04 18:06:36.952 UTC
[  FAILED  ] FilesystemUtils.WriteFileIfChanged (2 ms)
[317/317] FilesystemUtils.WriteFileIfChanged (2 ms)
1 test failed:
    FilesystemUtils.WriteFileIfChanged
(../../tools/gn/filesystem_utils_unittest.cc:579)

-----

More details in http://crbug.com/584548..