[GN] Don't rewrite files with the same contents
BUG=
Review URL: https://codereview.chromium.org/1656253003
Cr-Original-Commit-Position: refs/heads/master@{#373544}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: f8ea5cceefcedd4a01935d5ac4d2ba71e23ac13e
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..e44d337 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,59 @@
#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));
+ ASSERT_TRUE(base::GetFileInfo(file_path, &file_info));
+ EXPECT_NE(last_modified, file_info.last_modified);
+}
+
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(
