[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(