Merge topic 'improve-symlink-error-reporting'

569fb1893e file(INSTALL): Report "Installing:" for a symlink to a directory
1461ae4933 file(INSTALL): Clarify symlink vs dir conflict errors
85f01a1ec2 file(INSTALL): Improve formatting of symlink creation error
aba48bd6ac cmSystemTools: Provide quiet link creation methods

Acked-by: Kitware Robot <[email protected]>
Acked-by: buildbot <[email protected]>
Merge-request: !7706

Source/cmFileCommand.cxx

@@ -2960,11 +2960,23 @@ bool HandleCreateLinkCommand(std::vector<std::string> const& args,
 
   // Check if the command requires a symbolic link.
   if (arguments.Symbolic) {
-    completed = static_cast<bool>(
-      cmSystemTools::CreateSymlink(fileName, newFileName, &result));
+    cmsys::Status linked =
+      cmSystemTools::CreateSymlinkQuietly(fileName, newFileName);
+    if (linked) {
+      completed = true;
+    } else {
+      result = cmStrCat("failed to create symbolic link '", newFileName,
+                        "': ", linked.GetString());
+    }
   } else {
-    completed = static_cast<bool>(
-      cmSystemTools::CreateLink(fileName, newFileName, &result));
+    cmsys::Status linked =
+      cmSystemTools::CreateLinkQuietly(fileName, newFileName);
+    if (linked) {
+      completed = true;
+    } else {
+      result = cmStrCat("failed to create link '", newFileName,
+                        "': ", linked.GetString());
+    }
   }
 
   // Check if copy-on-error is enabled in the arguments.

Source/cmFileCopier.cxx

@@ -15,7 +15,11 @@
 #include "cmValue.h"
 
 #ifdef _WIN32
+#  include <winerror.h>
+
 #  include "cmsys/FStream.hxx"
+#else
+#  include <cerrno>
 #endif
 
 #include <cstring>
@@ -504,11 +508,12 @@ bool cmFileCopier::InstallSymlinkChain(std::string& fromFile,
       cmSystemTools::RemoveFile(toFile);
       cmSystemTools::MakeDirectory(toFilePath);
 
-      if (!cmSystemTools::CreateSymlink(symlinkTarget, toFile)) {
-        std::ostringstream e;
-        e << this->Name << " cannot create symlink \"" << toFile
-          << "\": " << cmSystemTools::GetLastSystemError() << ".";
-        this->Status.SetError(e.str());
+      cmsys::Status status =
+        cmSystemTools::CreateSymlinkQuietly(symlinkTarget, toFile);
+      if (!status) {
+        std::string e = cmStrCat(this->Name, " cannot create symlink\n  ",
+                                 toFile, "\nbecause: ", status.GetString());
+        this->Status.SetError(e);
         return false;
       }
     }
@@ -557,12 +562,24 @@ bool cmFileCopier::InstallSymlink(const std::string& fromFile,
     cmSystemTools::MakeDirectory(cmSystemTools::GetFilenamePath(toFile));
 
     // Create the symlink.
-    if (!cmSystemTools::CreateSymlink(symlinkTarget, toFile)) {
-      std::ostringstream e;
-      e << this->Name << " cannot duplicate symlink \"" << fromFile
-        << "\" at \"" << toFile
-        << "\": " << cmSystemTools::GetLastSystemError() << ".";
-      this->Status.SetError(e.str());
+    cmsys::Status status =
+      cmSystemTools::CreateSymlinkQuietly(symlinkTarget, toFile);
+    if (!status) {
+#ifdef _WIN32
+      bool const errorFileExists = status.GetWindows() == ERROR_FILE_EXISTS;
+#else
+      bool const errorFileExists = status.GetPOSIX() == EEXIST;
+#endif
+      std::string reason;
+      if (errorFileExists && cmSystemTools::FileIsDirectory(toFile)) {
+        reason = "A directory already exists at that location";
+      } else {
+        reason = status.GetString();
+      }
+      std::string e =
+        cmStrCat(this->Name, " cannot duplicate symlink\n  ", fromFile,
+                 "\nat\n  ", toFile, "\nbecause: ", reason);
+      this->Status.SetError(e);
       return false;
     }
   }
@@ -630,7 +647,10 @@ bool cmFileCopier::InstallDirectory(const std::string& source,
 {
   // Inform the user about this directory installation.
   this->ReportCopy(destination, TypeDir,
-                   !cmSystemTools::FileIsDirectory(destination));
+                   !( // Report "Up-to-date:" for existing directories,
+                      // but not symlinks to them.
+                     cmSystemTools::FileIsDirectory(destination) &&
+                     !cmSystemTools::FileIsSymlink(destination)));
 
   // check if default dir creation permissions were set
   mode_t default_dir_mode_v = 0;

Source/cmSystemTools.cxx

@@ -3590,8 +3590,19 @@ std::string cmSystemTools::EncodeURL(std::string const& in, bool escapeSlashes)
 }
 
 cmsys::Status cmSystemTools::CreateSymlink(std::string const& origName,
-                                           std::string const& newName,
-                                           std::string* errorMessage)
+                                           std::string const& newName)
+{
+  cmsys::Status status =
+    cmSystemTools::CreateSymlinkQuietly(origName, newName);
+  if (!status) {
+    cmSystemTools::Error(cmStrCat("failed to create symbolic link '", newName,
+                                  "': ", status.GetString()));
+  }
+  return status;
+}
+
+cmsys::Status cmSystemTools::CreateSymlinkQuietly(std::string const& origName,
+                                                  std::string const& newName)
 {
   uv_fs_t req;
   int flags = 0;
@@ -3611,20 +3622,23 @@ cmsys::Status cmSystemTools::CreateSymlink(std::string const& origName,
 #else
     status = cmsys::Status::POSIX(-err);
 #endif
-    std::string e = cmStrCat("failed to create symbolic link '", newName,
-                             "': ", status.GetString());
-    if (errorMessage) {
-      *errorMessage = std::move(e);
-    } else {
-      cmSystemTools::Error(e);
-    }
   }
   return status;
 }
 
 cmsys::Status cmSystemTools::CreateLink(std::string const& origName,
-                                        std::string const& newName,
-                                        std::string* errorMessage)
+                                        std::string const& newName)
+{
+  cmsys::Status status = cmSystemTools::CreateLinkQuietly(origName, newName);
+  if (!status) {
+    cmSystemTools::Error(
+      cmStrCat("failed to create link '", newName, "': ", status.GetString()));
+  }
+  return status;
+}
+
+cmsys::Status cmSystemTools::CreateLinkQuietly(std::string const& origName,
+                                               std::string const& newName)
 {
   uv_fs_t req;
   int err =
@@ -3638,13 +3652,6 @@ cmsys::Status cmSystemTools::CreateLink(std::string const& origName,
 #else
     status = cmsys::Status::POSIX(-err);
 #endif
-    std::string e =
-      cmStrCat("failed to create link '", newName, "': ", status.GetString());
-    if (errorMessage) {
-      *errorMessage = std::move(e);
-    } else {
-      cmSystemTools::Error(e);
-    }
   }
   return status;
 }

Source/cmSystemTools.h

@@ -595,14 +595,16 @@ public:
   /** Create a symbolic link if the platform supports it.  Returns whether
       creation succeeded. */
   static cmsys::Status CreateSymlink(std::string const& origName,
-                                     std::string const& newName,
-                                     std::string* errorMessage = nullptr);
+                                     std::string const& newName);
+  static cmsys::Status CreateSymlinkQuietly(std::string const& origName,
+                                            std::string const& newName);
 
   /** Create a hard link if the platform supports it.  Returns whether
       creation succeeded. */
   static cmsys::Status CreateLink(std::string const& origName,
-                                  std::string const& newName,
-                                  std::string* errorMessage = nullptr);
+                                  std::string const& newName);
+  static cmsys::Status CreateLinkQuietly(std::string const& origName,
+                                         std::string const& newName);
 
   /** Get the system name. */
   static cm::string_view GetSystemName();

Source/cmcmd.cxx

@@ -1700,16 +1700,15 @@ cmsys::Status cmcmd::SymlinkInternal(std::string const& file,
   }
   std::string linktext = cmSystemTools::GetFilenameName(file);
 #if defined(_WIN32) && !defined(__CYGWIN__)
-  std::string errorMessage;
-  cmsys::Status status =
-    cmSystemTools::CreateSymlink(linktext, link, &errorMessage);
+  cmsys::Status status = cmSystemTools::CreateSymlinkQuietly(linktext, link);
   // Creating a symlink will fail with ERROR_PRIVILEGE_NOT_HELD if the user
   // does not have SeCreateSymbolicLinkPrivilege, or if developer mode is not
   // active. In that case, we try to copy the file.
   if (status.GetWindows() == ERROR_PRIVILEGE_NOT_HELD) {
     status = cmSystemTools::CopyFileAlways(file, link);
   } else if (!status) {
-    cmSystemTools::Error(errorMessage);
+    cmSystemTools::Error(cmStrCat("failed to create symbolic link '", link,
+                                  "': ", status.GetString()));
   }
   return status;
 #else

Tests/RunCMake/install/DIRECTORY-symlink-clobber-all-result.txt

@@ -0,0 +1 @@
+1

Tests/RunCMake/install/DIRECTORY-symlink-clobber-all-stderr.txt

@@ -0,0 +1,12 @@
+^CMake Error at cmake_install.cmake:[0-9]+ \(file\):
+  file INSTALL cannot duplicate symlink
+
+    [^
+]*/Tests/RunCMake/install/DIRECTORY-symlink-clobber-build/new/dir
+
+  at
+
+    [^
+]*/Tests/RunCMake/install/DIRECTORY-symlink-clobber-build/root-all/dest/dir
+
+  because: A directory already exists at that location

Tests/RunCMake/install/DIRECTORY-symlink-clobber-all-stdout.txt

@@ -0,0 +1,12 @@
+-- Installing: [^
+]*/Tests/RunCMake/install/DIRECTORY-symlink-clobber-build/root-all/dest/dir
+-- Installing: [^
+]*/Tests/RunCMake/install/DIRECTORY-symlink-clobber-build/root-all/dest/dir/file
+-- Installing: [^
+]*/Tests/RunCMake/install/DIRECTORY-symlink-clobber-build/root-all/dest/lnk
+-- Installing: [^
+]*/Tests/RunCMake/install/DIRECTORY-symlink-clobber-build/root-all/dest/lnk
+-- Up-to-date: [^
+]*/Tests/RunCMake/install/DIRECTORY-symlink-clobber-build/root-all/dest/lnk/file
+-- Installing: [^
+]*/Tests/RunCMake/install/DIRECTORY-symlink-clobber-build/root-all/dest/dir

Tests/RunCMake/install/DIRECTORY-symlink-clobber.cmake

@@ -0,0 +1,11 @@
+file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/old/dir)
+file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/old/dir/file "")
+file(CREATE_LINK dir ${CMAKE_CURRENT_BINARY_DIR}/old/lnk SYMBOLIC)
+install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/old/dir DESTINATION dest)
+install(FILES ${CMAKE_CURRENT_BINARY_DIR}/old/lnk     DESTINATION dest)
+
+file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/new/lnk)
+file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/new/lnk/file "")
+file(CREATE_LINK lnk ${CMAKE_CURRENT_BINARY_DIR}/new/dir SYMBOLIC)
+install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/new/lnk DESTINATION dest)
+install(FILES ${CMAKE_CURRENT_BINARY_DIR}/new/dir     DESTINATION dest)

Tests/RunCMake/install/RunCMakeTest.cmake

@@ -175,6 +175,10 @@ run_install_test(FILES-PERMISSIONS)
 run_install_test(TARGETS-RPATH)
 run_install_test(InstallRequiredSystemLibraries)
 
+if(UNIX)
+  run_install_test(DIRECTORY-symlink-clobber)
+endif()
+
 if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
   run_cmake(TARGETS-RUNTIME_DEPENDENCIES-macos-two-bundle)
   run_cmake(TARGETS-RUNTIME_DEPENDENCIES-macos-no-framework)