[GN] Don't rewrite files with the same contents
Reland of https://codereview.chromium.org/1656253003 with fix.
Reason for revert:
Need to revert this patch according https://sites.google.com/a/chromium.org/dev/developers/tree-sheriffs/sheriffing-bug-queues. 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_rel_ng/builds/176911
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/176911
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/176735
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/176715
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/176715
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/176561
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/176462
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/176375
-----
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.
Original issue's description:
> [GN] Don't rewrite files with the same contents
>
> BUG=
>
> Committed: https://crrev.com/f8ea5cceefcedd4a01935d5ac4d2ba71e23ac13e
> Cr-Commit-Position: refs/heads/master@{#373544}
BUG=584548
Review URL: https://codereview.chromium.org/1704383002
Cr-Original-Commit-Position: refs/heads/master@{#376430}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 36deb395534c77295db41cc5f3e6b9627fbc5aa8
diff --git a/tools/gn/filesystem_utils.cc b/tools/gn/filesystem_utils.cc
index 5905cdb..2ceca81 100644
--- a/tools/gn/filesystem_utils.cc
+++ b/tools/gn/filesystem_utils.cc
@@ -687,6 +687,86 @@
return toolchain_label.name() + "/";
}
+bool ContentsEqual(const base::FilePath& file_path, const std::string& data) {
+ // Compare file and stream sizes first. Quick and will save us some time if
+ // they are different sizes.
+ int64_t file_size;
+ if (!base::GetFileSize(file_path, &file_size) ||
+ static_cast<size_t>(file_size) != data.size()) {
+ return false;
+ }
+
+ std::string file_data;
+ file_data.resize(file_size);
+ if (!base::ReadFileToString(file_path, &file_data))
+ return false;
+
+ return file_data == data;
+}
+
+bool WriteFileIfChanged(const base::FilePath& file_path,
+ const std::string& data,
+ Err* err) {
+ if (ContentsEqual(file_path, data))
+ return true;
+
+ // Create the directory if necessary.
+ if (!base::CreateDirectory(file_path.DirName())) {
+ if (err) {
+ *err =
+ Err(Location(), "Unable to create directory.",
+ "I was using \"" + FilePathToUTF8(file_path.DirName()) + "\".");
+ }
+ return false;
+ }
+
+ int size = static_cast<int>(data.size());
+ bool write_success = false;
+
+#if defined(OS_WIN)
+ // On Windows, provide a custom implementation of base::WriteFile. Sometimes
+ // the base version fails, especially on the bots. The guess is that Windows
+ // Defender or other antivirus programs still have the file open (after
+ // checking for the read) when the write happens immediately after. This
+ // version opens with FILE_SHARE_READ (normally not what you want when
+ // replacing the entire contents of the file) which lets us continue even if
+ // another program has the file open for reading. See http://crbug.com/468437
+ base::win::ScopedHandle file(::CreateFile(file_path.value().c_str(),
+ GENERIC_WRITE, FILE_SHARE_READ,
+ NULL, CREATE_ALWAYS, 0, NULL));
+ if (file.IsValid()) {
+ DWORD written;
+ BOOL result = ::WriteFile(file.Get(), data.c_str(), size, &written, NULL);
+ if (result) {
+ if (static_cast<int>(written) == size) {
+ write_success = true;
+ } else {
+ // Didn't write all the bytes.
+ LOG(ERROR) << "wrote" << written << " bytes to "
+ << base::UTF16ToUTF8(file_path.value()) << " expected "
+ << size;
+ }
+ } else {
+ // WriteFile failed.
+ PLOG(ERROR) << "writing file " << base::UTF16ToUTF8(file_path.value())
+ << " failed";
+ }
+ } else {
+ PLOG(ERROR) << "CreateFile failed for path "
+ << base::UTF16ToUTF8(file_path.value());
+ }
+#else
+ write_success = base::WriteFile(file_path, data.c_str(), size) == size;
+#endif
+
+ if (!write_success && err) {
+ *err = Err(Location(), "Unable to write file.",
+ "I was writing \"" + FilePathToUTF8(file_path) + "\".");
+ }
+
+ return write_success;
+}
+
SourceDir GetToolchainOutputDir(const Settings* settings) {
return settings->toolchain_output_subdir().AsSourceDir(
settings->build_settings());
diff --git a/tools/gn/filesystem_utils.h b/tools/gn/filesystem_utils.h
index f64c00d..aaa08ba 100644
--- a/tools/gn/filesystem_utils.h
+++ b/tools/gn/filesystem_utils.h
@@ -163,6 +163,18 @@
// go in the root build directory. Otherwise, the result will end in a slash.
std::string GetOutputSubdirName(const Label& toolchain_label, bool is_default);
+// Returns true if the contents of the file and stream given are equal, false
+// otherwise.
+bool ContentsEqual(const base::FilePath& file_path, const std::string& data);
+
+// Writes given stream contents to the given file if it differs from existing
+// file contents. Returns true if new contents was successfully written or
+// existing file contents doesn't need updating, false on write error. |err| is
+// set on write error if not nullptr.
+bool WriteFileIfChanged(const base::FilePath& file_path,
+ const std::string& data,
+ Err* err);
+
// -----------------------------------------------------------------------------
// These functions return the various flavors of output and gen directories.
diff --git a/tools/gn/filesystem_utils_unittest.cc b/tools/gn/filesystem_utils_unittest.cc
index 27ccab2..d563890 100644
--- a/tools/gn/filesystem_utils_unittest.cc
+++ b/tools/gn/filesystem_utils_unittest.cc
@@ -3,8 +3,11 @@
// found in the LICENSE file.
#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
+#include "base/threading/platform_thread.h"
#include "build/build_config.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "tools/gn/filesystem_utils.h"
@@ -554,6 +557,60 @@
#endif
}
+TEST(FilesystemUtils, ContentsEqual) {
+ base::ScopedTempDir temp_dir;
+ ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+
+ std::string data = "foo";
+
+ base::FilePath file_path = temp_dir.path().AppendASCII("foo.txt");
+ base::WriteFile(file_path, data.c_str(), static_cast<int>(data.size()));
+
+ EXPECT_TRUE(ContentsEqual(file_path, data));
+
+ // Different length and contents.
+ data += "bar";
+ EXPECT_FALSE(ContentsEqual(file_path, data));
+
+ // The same length, different contents.
+ EXPECT_FALSE(ContentsEqual(file_path, "bar"));
+}
+
+TEST(FilesystemUtils, WriteFileIfChanged) {
+ base::ScopedTempDir temp_dir;
+ ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+
+ std::string data = "foo";
+
+ // Write if file doesn't exist. Create also directory.
+ base::FilePath file_path =
+ temp_dir.path().AppendASCII("bar").AppendASCII("foo.txt");
+ EXPECT_TRUE(WriteFileIfChanged(file_path, data, nullptr));
+
+ base::File::Info file_info;
+ ASSERT_TRUE(base::GetFileInfo(file_path, &file_info));
+ base::Time last_modified = file_info.last_modified;
+
+#if defined(OS_MACOSX)
+ // Modification times are in seconds in HFS on Mac.
+ base::TimeDelta sleep_time = base::TimeDelta::FromSeconds(1);
+#else
+ base::TimeDelta sleep_time = base::TimeDelta::FromMilliseconds(1);
+#endif
+ base::PlatformThread::Sleep(sleep_time);
+
+ // Don't write if contents is the same.
+ EXPECT_TRUE(WriteFileIfChanged(file_path, data, nullptr));
+ ASSERT_TRUE(base::GetFileInfo(file_path, &file_info));
+ EXPECT_EQ(last_modified, file_info.last_modified);
+
+ // Write if contents changed.
+ EXPECT_TRUE(WriteFileIfChanged(file_path, "bar", nullptr));
+ std::string file_data;
+ ASSERT_TRUE(base::ReadFileToString(file_path, &file_data));
+ EXPECT_EQ("bar", file_data);
+}
+
TEST(FilesystemUtils, GetToolchainDirs) {
BuildSettings build_settings;
build_settings.SetBuildDir(SourceDir("//out/Debug/"));
diff --git a/tools/gn/function_write_file.cc b/tools/gn/function_write_file.cc
index 9159975..feb1004 100644
--- a/tools/gn/function_write_file.cc
+++ b/tools/gn/function_write_file.cc
@@ -19,55 +19,6 @@
namespace functions {
-namespace {
-
-// On Windows, provide a custom implementation of base::WriteFile. Sometimes
-// the base version fails, especially on the bots. The guess is that Windows
-// Defender or other antivirus programs still have the file open (after
-// checking for the read) when the write happens immediately after. This
-// version opens with FILE_SHARE_READ (normally not what you want when
-// replacing the entire contents of the file) which lets us continue even if
-// another program has the file open for reading. See http://crbug.com/468437
-#if defined(OS_WIN)
-int DoWriteFile(const base::FilePath& filename, const char* data, int size) {
- base::win::ScopedHandle file(::CreateFile(
- filename.value().c_str(),
- GENERIC_WRITE,
- FILE_SHARE_READ,
- NULL,
- CREATE_ALWAYS,
- 0,
- NULL));
- if (!file.IsValid()) {
- PLOG(ERROR) << "CreateFile failed for path "
- << base::UTF16ToUTF8(filename.value());
- return -1;
- }
-
- DWORD written;
- BOOL result = ::WriteFile(file.Get(), data, size, &written, NULL);
- if (result && static_cast<int>(written) == size)
- return written;
-
- if (!result) {
- // WriteFile failed.
- PLOG(ERROR) << "writing file " << base::UTF16ToUTF8(filename.value())
- << " failed";
- } else {
- // Didn't write all the bytes.
- LOG(ERROR) << "wrote" << written << " bytes to "
- << base::UTF16ToUTF8(filename.value()) << " expected " << size;
- }
- return -1;
-}
-#else
-int DoWriteFile(const base::FilePath& filename, const char* data, int size) {
- return base::WriteFile(filename, data, size);
-}
-#endif
-
-} // namespace
-
const char kWriteFile[] = "write_file";
const char kWriteFile_HelpShort[] =
"write_file: Write a file to disk.";
@@ -138,30 +89,14 @@
} else {
contents << args[1].ToString(false);
}
- const std::string& new_contents = contents.str();
+
base::FilePath file_path =
scope->settings()->build_settings()->GetFullPath(source_file);
// Make sure we're not replacing the same contents.
- std::string existing_contents;
- if (base::ReadFileToString(file_path, &existing_contents) &&
- existing_contents == new_contents)
- return Value(); // Nothing to do.
+ if (!WriteFileIfChanged(file_path, contents.str(), err))
+ *err = Err(function->function(), err->message(), err->help_text());
- // Write file, creating the directory if necessary.
- if (!base::CreateDirectory(file_path.DirName())) {
- *err = Err(function->function(), "Unable to create directory.",
- "I was using \"" + FilePathToUTF8(file_path.DirName()) + "\".");
- return Value();
- }
-
- int int_size = static_cast<int>(new_contents.size());
- if (DoWriteFile(file_path, new_contents.c_str(), int_size)
- != int_size) {
- *err = Err(function->function(), "Unable to write file.",
- "I was writing \"" + FilePathToUTF8(file_path) + "\".");
- return Value();
- }
return Value();
}
diff --git a/tools/gn/ninja_target_writer.cc b/tools/gn/ninja_target_writer.cc
index 7d9c08a..9f19b2b 100644
--- a/tools/gn/ninja_target_writer.cc
+++ b/tools/gn/ninja_target_writer.cc
@@ -74,9 +74,7 @@
CHECK(0) << "Output type of target not handled.";
}
- std::string contents = file.str();
- base::WriteFile(ninja_file, contents.c_str(),
- static_cast<int>(contents.size()));
+ WriteFileIfChanged(ninja_file, file.str(), nullptr);
}
void NinjaTargetWriter::WriteEscapedSubstitution(SubstitutionType type) {
diff --git a/tools/gn/visual_studio_writer.cc b/tools/gn/visual_studio_writer.cc
index 44a3f11..dec0837 100644
--- a/tools/gn/visual_studio_writer.cc
+++ b/tools/gn/visual_studio_writer.cc
@@ -9,7 +9,6 @@
#include <set>
#include <string>
-#include "base/files/file_util.h"
#include "base/logging.h"
#include "base/memory/scoped_ptr.h"
#include "base/strings/string_util.h"
@@ -153,26 +152,6 @@
return base::StringPiece();
}
-bool HasSameContent(std::stringstream& data_1, const base::FilePath& data_2) {
- // Compare file sizes first. Quick and will save us some time if they are
- // different sizes.
- int64_t data_1_len = data_1.tellp();
-
- int64_t data_2_len;
- if (!base::GetFileSize(data_2, &data_2_len) || data_1_len != data_2_len)
- return false;
-
- std::string data_2_data;
- data_2_data.resize(data_2_len);
- if (!base::ReadFileToString(data_2, &data_2_data))
- return false;
-
- std::string data_1_data;
- data_1_data = data_1.str();
-
- return data_1_data == data_2_data;
-}
-
} // namespace
VisualStudioWriter::SolutionEntry::SolutionEntry(const std::string& _name,
@@ -289,32 +268,13 @@
// Only write the content to the file if it's different. That is
// both a performance optimization and more importantly, prevents
// Visual Studio from reloading the projects.
- if (!HasSameContent(vcxproj_string_out, vcxproj_path)) {
- std::string content = vcxproj_string_out.str();
- int size = static_cast<int>(content.size());
- if (base::WriteFile(vcxproj_path, content.c_str(), size) != size) {
- *err = Err(Location(), "Couldn't open " + target->label().name() +
- ".vcxproj for writing");
- return false;
- }
- }
+ if (!WriteFileIfChanged(vcxproj_path, vcxproj_string_out.str(), err))
+ return false;
base::FilePath filters_path = UTF8ToFilePath(vcxproj_path_str + ".filters");
-
std::stringstream filters_string_out;
WriteFiltersFileContents(filters_string_out, target);
-
- if (!HasSameContent(filters_string_out, filters_path)) {
- std::string content = filters_string_out.str();
- int size = static_cast<int>(content.size());
- if (base::WriteFile(filters_path, content.c_str(), size) != size) {
- *err = Err(Location(), "Couldn't open " + target->label().name() +
- ".vcxproj.filters for writing");
- return false;
- }
- }
-
- return true;
+ return WriteFileIfChanged(filters_path, filters_string_out.str(), err);
}
bool VisualStudioWriter::WriteProjectFileContents(
@@ -612,17 +572,7 @@
// Only write the content to the file if it's different. That is
// both a performance optimization and more importantly, prevents
// Visual Studio from reloading the projects.
- if (HasSameContent(string_out, sln_path))
- return true;
-
- std::string content = string_out.str();
- int size = static_cast<int>(content.size());
- if (base::WriteFile(sln_path, content.c_str(), size) != size) {
- *err = Err(Location(), "Couldn't open all.sln for writing");
- return false;
- }
-
- return true;
+ return WriteFileIfChanged(sln_path, string_out.str(), err);
}
void VisualStudioWriter::WriteSolutionFileContents(
