From 0fafdb7eb8b710594448d2ccf61d3896b4ea8a17 Mon Sep 17 00:00:00 2001 From: David Cole Date: Thu, 18 Dec 2008 10:43:24 -0500 Subject: [PATCH] BUG: Do not copy permissions of files when making the copy in an install rule. If the source file was read-only, this prevents the subsequent set of the destination file's modification time, making the copied file always different in time-stamp than the original and always installing a new file with a new time stamp (but the same content) causing unnecessary downstream incremental rebuilds. As part of this fix, add an optional copyPermissions parameter to the SystemTools routines CopyFileIfDifferent, CopyFileAlways, CopyAFile and CopyADirectory. The copyPermissions parameter defaults to true to preserve the behavior of these routines for existing callers. --- Source/cmFileCommand.cxx | 10 ++++++++-- Source/kwsys/SystemTools.cxx | 28 +++++++++++++++++----------- Source/kwsys/SystemTools.hxx.in | 27 ++++++++++++++++++--------- 3 files changed, 43 insertions(+), 22 deletions(-) diff --git a/Source/cmFileCommand.cxx b/Source/cmFileCommand.cxx index 032d603a8..ca8e06691 100644 --- a/Source/cmFileCommand.cxx +++ b/Source/cmFileCommand.cxx @@ -1049,7 +1049,7 @@ bool cmFileInstaller::InstallFile(const char* fromFile, const char* toFile, this->Makefile->DisplayStatus(message.c_str(), -1); // Copy the file. - if(copy && !cmSystemTools::CopyAFile(fromFile, toFile, true)) + if(copy && !cmSystemTools::CopyAFile(fromFile, toFile, true, false)) { cmOStringStream e; e << "INSTALL cannot copy file \"" << fromFile @@ -1064,7 +1064,13 @@ bool cmFileInstaller::InstallFile(const char* fromFile, const char* toFile, // Set the file modification time of the destination file. if(copy && !always) { - cmSystemTools::CopyFileTime(fromFile, toFile); + if (!cmSystemTools::CopyFileTime(fromFile, toFile)) + { + cmOStringStream e; + e << "Problem setting modification time on file \"" << toFile << "\""; + this->FileCommand->SetError(e.str().c_str()); + return false; + } } // Set permissions of the destination file. diff --git a/Source/kwsys/SystemTools.cxx b/Source/kwsys/SystemTools.cxx index 72b109cfe..65f62596b 100644 --- a/Source/kwsys/SystemTools.cxx +++ b/Source/kwsys/SystemTools.cxx @@ -1611,7 +1611,8 @@ kwsys_stl::string SystemTools::ConvertToWindowsOutputPath(const char* path) } bool SystemTools::CopyFileIfDifferent(const char* source, - const char* destination) + const char* destination, + bool copyPermissions) { // special check for a destination that is a directory // FilesDiffer does not handle file to directory compare @@ -1624,7 +1625,8 @@ bool SystemTools::CopyFileIfDifferent(const char* source, new_destination += SystemTools::GetFilenameName(source_name); if(SystemTools::FilesDiffer(source, new_destination.c_str())) { - return SystemTools::CopyFileAlways(source, destination); + return SystemTools::CopyFileAlways(source, destination, + copyPermissions); } else { @@ -1637,7 +1639,7 @@ bool SystemTools::CopyFileIfDifferent(const char* source, // are different if(SystemTools::FilesDiffer(source, destination)) { - return SystemTools::CopyFileAlways(source, destination); + return SystemTools::CopyFileAlways(source, destination, copyPermissions); } // at this point the files must be the same so return true return true; @@ -1718,10 +1720,12 @@ bool SystemTools::FilesDiffer(const char* source, } +//---------------------------------------------------------------------------- /** * Copy a file named by "source" to the file named by "destination". */ -bool SystemTools::CopyFileAlways(const char* source, const char* destination) +bool SystemTools::CopyFileAlways(const char* source, const char* destination, + bool copyPermissions) { // If files are the same do not copy if ( SystemTools::SameFile(source, destination) ) @@ -1824,7 +1828,7 @@ bool SystemTools::CopyFileAlways(const char* source, const char* destination) { return false; } - if ( perms ) + if ( copyPermissions && perms ) { if ( !SystemTools::SetPermissions(destination, perm) ) { @@ -1836,15 +1840,15 @@ bool SystemTools::CopyFileAlways(const char* source, const char* destination) //---------------------------------------------------------------------------- bool SystemTools::CopyAFile(const char* source, const char* destination, - bool always) + bool always, bool copyPermissions) { if(always) { - return SystemTools::CopyFileAlways(source, destination); + return SystemTools::CopyFileAlways(source, destination, copyPermissions); } else { - return SystemTools::CopyFileIfDifferent(source, destination); + return SystemTools::CopyFileIfDifferent(source, destination, copyPermissions); } } @@ -1853,7 +1857,7 @@ bool SystemTools::CopyAFile(const char* source, const char* destination, * "destination". */ bool SystemTools::CopyADirectory(const char* source, const char* destination, - bool always) + bool always, bool copyPermissions) { Directory dir; dir.Load(source); @@ -1877,14 +1881,16 @@ bool SystemTools::CopyADirectory(const char* source, const char* destination, fullDestPath += dir.GetFile(static_cast(fileNum)); if (!SystemTools::CopyADirectory(fullPath.c_str(), fullDestPath.c_str(), - always)) + always, + copyPermissions)) { return false; } } else { - if(!SystemTools::CopyAFile(fullPath.c_str(), destination, always)) + if(!SystemTools::CopyAFile(fullPath.c_str(), destination, always, + copyPermissions)) { return false; } diff --git a/Source/kwsys/SystemTools.hxx.in b/Source/kwsys/SystemTools.hxx.in index 1484b60b7..b09276498 100644 --- a/Source/kwsys/SystemTools.hxx.in +++ b/Source/kwsys/SystemTools.hxx.in @@ -490,33 +490,42 @@ public: /** * Copy the source file to the destination file only - * if the two files differ. + * if the two files differ. If the "copyPermissions" + * argument is true, the permissions of the copy are + * set to be the same as the permissions of the + * original. */ static bool CopyFileIfDifferent(const char* source, - const char* destination); - + const char* destination, + bool copyPermissions = true); + /** * Compare the contents of two files. Return true if different */ static bool FilesDiffer(const char* source, const char* destination); - + /** * Return true if the two files are the same file */ static bool SameFile(const char* file1, const char* file2); /** - * Copy a file + * Copy a file. If the "copyPermissions" argument is true, the + * permissions of the copy are set to be the same as the permissions + * of the original. */ - static bool CopyFileAlways(const char* source, const char* destination); + static bool CopyFileAlways(const char* source, const char* destination, + bool copyPermissions = true); /** * Copy a file. If the "always" argument is true the file is always * copied. If it is false, the file is copied only if it is new or - * has changed. + * has changed. If the "copyPermissions" argument is true, the + * permissions of the copy are set to be the same as the permissions + * of the original. */ static bool CopyAFile(const char* source, const char* destination, - bool always = true); + bool always = true, bool copyPermissions = true); /** * Copy content directory to another directory with all files and @@ -525,7 +534,7 @@ public: * are new are copied. */ static bool CopyADirectory(const char* source, const char* destination, - bool always = true); + bool always = true, bool copyPermissions = true); /** * Remove a file