Merge topic 'cmFileCopier-error-loss'

a820877d03 errors: avoid constructing a stream before getting the last error
5cf7018af6 cmFileCopier: remember error statuses and get their strings
0639a32d3a cmFileTimes: return status codes from APIs

Acked-by: Kitware Robot <kwrobot@kitware.com>
Tested-by: buildbot <buildbot@kitware.com>
Merge-request: !9023

Source/cmCoreTryCompile.cxx

@@ -629,13 +629,11 @@ cm::optional<cmTryCompileResult> cmCoreTryCompile::TryCompileCode(
     // now create a CMakeLists.txt file in that directory
     FILE* fout = cmsys::SystemTools::Fopen(outFileName, "w");
     if (!fout) {
-      std::ostringstream e;
-      /* clang-format off */
-      e << "Failed to open\n"
-           "  " << outFileName << "\n"
-        << cmSystemTools::GetLastSystemError();
-      /* clang-format on */
-      this->Makefile->IssueMessage(MessageType::FATAL_ERROR, e.str());
+      this->Makefile->IssueMessage(
+        MessageType::FATAL_ERROR,
+        cmStrCat("Failed to open\n"
+                 "  ",
+                 outFileName, '\n', cmSystemTools::GetLastSystemError()));
       return cm::nullopt;
     }
 

Source/cmExportCommand.cxx

@@ -485,13 +485,11 @@ static void StorePackageRegistry(cmMakefile& mf, std::string const& package,
     if (entry) {
       entry << content << "\n";
     } else {
-      std::ostringstream e;
-      /* clang-format off */
-      e << "Cannot create package registry file:\n"
-        << "  " << fname << "\n"
-        << cmSystemTools::GetLastSystemError() << "\n";
-      /* clang-format on */
-      mf.IssueMessage(MessageType::WARNING, e.str());
+      mf.IssueMessage(MessageType::WARNING,
+                      cmStrCat("Cannot create package registry file:\n"
+                               "  ",
+                               fname, '\n',
+                               cmSystemTools::GetLastSystemError(), '\n'));
     }
   }
 }

Source/cmFileCommand.cxx

@@ -3024,16 +3024,15 @@ bool HandleCreateLinkCommand(std::vector<std::string> const& args,
   // Check if the new file already exists and remove it.
   if (cmSystemTools::PathExists(newFileName) &&
       !cmSystemTools::RemoveFile(newFileName)) {
-    std::ostringstream e;
-    e << "Failed to create link '" << newFileName
-      << "' because existing path cannot be removed: "
-      << cmSystemTools::GetLastSystemError() << "\n";
+    auto err = cmStrCat("Failed to create link '", newFileName,
+                        "' because existing path cannot be removed: ",
+                        cmSystemTools::GetLastSystemError(), '\n');
 
     if (!arguments.Result.empty()) {
-      status.GetMakefile().AddDefinition(arguments.Result, e.str());
+      status.GetMakefile().AddDefinition(arguments.Result, err);
       return true;
     }
-    status.SetError(e.str());
+    status.SetError(err);
     return false;
   }
 

Source/cmFileCopier.cxx

@@ -86,10 +86,11 @@ bool cmFileCopier::SetPermissions(const std::string& toFile,
     }
 #endif
 
-    if (!cmSystemTools::SetPermissions(toFile, permissions)) {
+    auto perm_status = cmSystemTools::SetPermissions(toFile, permissions);
+    if (!perm_status) {
       std::ostringstream e;
       e << this->Name << " cannot set permissions on \"" << toFile
-        << "\": " << cmSystemTools::GetLastSystemError() << ".";
+        << "\": " << perm_status.GetString() << ".";
       this->Status.SetError(e.str());
       return false;
     }
@@ -118,10 +119,9 @@ std::string const& cmFileCopier::ToName(std::string const& fromName)
 bool cmFileCopier::ReportMissing(const std::string& fromFile)
 {
   // The input file does not exist and installation is not optional.
-  std::ostringstream e;
-  e << this->Name << " cannot find \"" << fromFile
-    << "\": " << cmSystemTools::GetLastSystemError() << ".";
-  this->Status.SetError(e.str());
+  this->Status.SetError(cmStrCat(this->Name, " cannot find \"", fromFile,
+                                 "\": ", cmSystemTools::GetLastSystemError(),
+                                 '.'));
   return false;
 }
 
@@ -530,11 +530,13 @@ bool cmFileCopier::InstallSymlink(const std::string& fromFile,
 {
   // Read the original symlink.
   std::string symlinkTarget;
-  if (!cmSystemTools::ReadSymlink(fromFile, symlinkTarget)) {
+  auto read_symlink_status =
+    cmSystemTools::ReadSymlink(fromFile, symlinkTarget);
+  if (!read_symlink_status) {
     std::ostringstream e;
     e << this->Name << " cannot read symlink \"" << fromFile
       << "\" to duplicate at \"" << toFile
-      << "\": " << cmSystemTools::GetLastSystemError() << ".";
+      << "\": " << read_symlink_status.GetString() << ".";
     this->Status.SetError(e.str());
     return false;
   }
@@ -604,12 +606,15 @@ bool cmFileCopier::InstallFile(const std::string& fromFile,
   this->ReportCopy(toFile, TypeFile, copy);
 
   // Copy the file.
-  if (copy && !cmSystemTools::CopyAFile(fromFile, toFile, true)) {
-    std::ostringstream e;
-    e << this->Name << " cannot copy file \"" << fromFile << "\" to \""
-      << toFile << "\": " << cmSystemTools::GetLastSystemError() << ".";
-    this->Status.SetError(e.str());
-    return false;
+  if (copy) {
+    auto copy_status = cmSystemTools::CopyAFile(fromFile, toFile, true);
+    if (!copy_status) {
+      std::ostringstream e;
+      e << this->Name << " cannot copy file \"" << fromFile << "\" to \""
+        << toFile << "\": " << copy_status.GetString() << ".";
+      this->Status.SetError(e.str());
+      return false;
+    }
   }
 
   // Set the file modification time of the destination file.
@@ -620,10 +625,11 @@ bool cmFileCopier::InstallFile(const std::string& fromFile,
     if (cmSystemTools::GetPermissions(toFile, perm)) {
       cmSystemTools::SetPermissions(toFile, perm | mode_owner_write);
     }
-    if (!cmFileTimes::Copy(fromFile, toFile)) {
+    auto copy_status = cmFileTimes::Copy(fromFile, toFile);
+    if (!copy_status) {
       std::ostringstream e;
       e << this->Name << " cannot set modification time on \"" << toFile
-        << "\": " << cmSystemTools::GetLastSystemError() << ".";
+        << "\": " << copy_status.GetString() << ".";
       this->Status.SetError(e.str());
       return false;
     }
@@ -660,10 +666,12 @@ bool cmFileCopier::InstallDirectory(const std::string& source,
   }
 
   // Make sure the destination directory exists.
-  if (!cmSystemTools::MakeDirectory(destination, default_dir_mode)) {
+  auto makedir_status =
+    cmSystemTools::MakeDirectory(destination, default_dir_mode);
+  if (!makedir_status) {
     std::ostringstream e;
     e << this->Name << " cannot make directory \"" << destination
-      << "\": " << cmSystemTools::GetLastSystemError() << ".";
+      << "\": " << makedir_status.GetString() << ".";
     this->Status.SetError(e.str());
     return false;
   }

Source/cmFileTimes.cxx

@@ -6,6 +6,8 @@
 
 #include <cm/memory>
 
+#include "cmsys/Status.hxx"
+
 #include "cm_sys_stat.h"
 
 #if defined(_WIN32)
@@ -13,6 +15,8 @@
 
 #  include "cmSystemTools.h"
 #else
+#  include <cerrno>
+
 #  include <utime.h>
 #endif
 
@@ -66,7 +70,7 @@ cmFileTimes::cmFileTimes(std::string const& fileName)
 }
 cmFileTimes::~cmFileTimes() = default;
 
-bool cmFileTimes::Load(std::string const& fileName)
+cmsys::Status cmFileTimes::Load(std::string const& fileName)
 {
   std::unique_ptr<Times> ptr;
   if (this->IsValid()) {
@@ -82,29 +86,29 @@ bool cmFileTimes::Load(std::string const& fileName)
                 GENERIC_READ, FILE_SHARE_READ, 0, OPEN_EXISTING,
                 FILE_FLAG_BACKUP_SEMANTICS, 0);
   if (!handle) {
-    return false;
+    return cmsys::Status::Windows_GetLastError();
   }
   if (!GetFileTime(handle, &ptr->timeCreation, &ptr->timeLastAccess,
                    &ptr->timeLastWrite)) {
-    return false;
+    return cmsys::Status::Windows_GetLastError();
   }
 #else
   struct stat st;
   if (stat(fileName.c_str(), &st) < 0) {
-    return false;
+    return cmsys::Status::POSIX_errno();
   }
   ptr->timeBuf.actime = st.st_atime;
   ptr->timeBuf.modtime = st.st_mtime;
 #endif
   // Accept times
   this->times = std::move(ptr);
-  return true;
+  return cmsys::Status::Success();
 }
 
-bool cmFileTimes::Store(std::string const& fileName) const
+cmsys::Status cmFileTimes::Store(std::string const& fileName) const
 {
   if (!this->IsValid()) {
-    return false;
+    return cmsys::Status::POSIX(EINVAL);
   }
 
 #if defined(_WIN32) && !defined(__CYGWIN__)
@@ -112,18 +116,28 @@ bool cmFileTimes::Store(std::string const& fileName) const
     cmSystemTools::ConvertToWindowsExtendedPath(fileName).c_str(),
     FILE_WRITE_ATTRIBUTES, 0, 0, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, 0);
   if (!handle) {
-    return false;
+    return cmsys::Status::Windows_GetLastError();
+  }
+  if (SetFileTime(handle, &this->times->timeCreation,
+                  &this->times->timeLastAccess,
+                  &this->times->timeLastWrite) == 0) {
+    return cmsys::Status::Windows_GetLastError();
   }
-  return SetFileTime(handle, &this->times->timeCreation,
-                     &this->times->timeLastAccess,
-                     &this->times->timeLastWrite) != 0;
 #else
-  return utime(fileName.c_str(), &this->times->timeBuf) >= 0;
+  if (utime(fileName.c_str(), &this->times->timeBuf) < 0) {
+    return cmsys::Status::POSIX_errno();
+  }
 #endif
+  return cmsys::Status::Success();
 }
 
-bool cmFileTimes::Copy(std::string const& fromFile, std::string const& toFile)
+cmsys::Status cmFileTimes::Copy(std::string const& fromFile,
+                                std::string const& toFile)
 {
   cmFileTimes fileTimes;
-  return (fileTimes.Load(fromFile) && fileTimes.Store(toFile));
+  cmsys::Status load_status = fileTimes.Load(fromFile);
+  if (!load_status) {
+    return load_status;
+  }
+  return fileTimes.Store(toFile);
 }

Source/cmFileTimes.h

@@ -7,6 +7,8 @@
 #include <memory>
 #include <string>
 
+#include "cmsys/Status.hxx"
+
 /** \class cmFileTimes
  * \brief Loads and stores file times.
  */
@@ -21,12 +23,13 @@ public:
   //! @return true, if file times were loaded successfully
   bool IsValid() const { return (this->times != nullptr); }
   //! Try to load the file times from @a fileName and @return IsValid()
-  bool Load(std::string const& fileName);
+  cmsys::Status Load(std::string const& fileName);
   //! Stores the file times at @a fileName (if IsValid())
-  bool Store(std::string const& fileName) const;
+  cmsys::Status Store(std::string const& fileName) const;
 
   //! Copies the file times of @a fromFile to @a toFile
-  static bool Copy(std::string const& fromFile, std::string const& toFile);
+  static cmsys::Status Copy(std::string const& fromFile,
+                            std::string const& toFile);
 
 private:
 #ifdef _WIN32

Source/cmake.cxx

@@ -1707,10 +1707,8 @@ void cmake::SetTraceFile(const std::string& file)
   this->TraceFile.close();
   this->TraceFile.open(file.c_str());
   if (!this->TraceFile) {
-    std::stringstream ss;
-    ss << "Error opening trace file " << file << ": "
-       << cmSystemTools::GetLastSystemError();
-    cmSystemTools::Error(ss.str());
+    cmSystemTools::Error(cmStrCat("Error opening trace file ", file, ": ",
+                                  cmSystemTools::GetLastSystemError()));
     return;
   }
   std::cout << "Trace will be written to " << file << '\n';