Address comments in #16709 (second iteration)

Skip symlinks on Windows.

Author: Roman Inflianskas

changelog.md

@@ -114,12 +114,14 @@ with other backends. see #9125. Use `-d:nimLegacyJsRound` for previous behavior.
 
 
 - Added optional `options` argument to `copyFile`, `copyFileToDir`, and
-  `copyFileWithPermissions`. By default, symlinks are followed (copy files
-  symlinks point to).
-- `copyDir` and `copyDirWithPermissions` copy symlinks as symlinks (instead of
-  skipping them as it was before).
-- `moveFile` and `moveDir` move symlinks as symlinks (instead of skipping them
-  sometimes as it was before).
+  `copyFileWithPermissions`. By default, on non-Windows OSes, symlinks are
+  followed (copy files symlinks point to); on Windows, `options` argument is
+  ignored and symlinks are skipped.
+- On non-Windows OSes, `copyDir` and `copyDirWithPermissions` copy symlinks as
+  symlinks (instead of skipping them as it was before); on Windows symlinks are
+  skipped.
+- On non-Windows OSes, `moveFile` and `moveDir` move symlinks as symlinks
+  (instead of skipping them sometimes as it was before).
 - Added optional `followSymlinks` argument to `setFilePermissions`.
 
 ## Language changes

lib/pure/os.nim

@@ -1725,8 +1725,9 @@ proc copyFile*(source, dest: string, options = {cfSymlinkFollow}) {.rtl,
   noWeirdTarget.} =
   ## Copies a file from `source` to `dest`, where `dest.parentDir` must exist.
   ##
-  ## `options` specify the way file is copied; by default, if `source` is a
-  ## symlink, copies the file symlink points to.
+  ## On non-Windows OSes, `options` specify the way file is copied; by default,
+  ## if `source` is a symlink, copies the file symlink points to. `options` is
+  ## ignored on Windows: symlinks are skipped.
   ##
   ## If this fails, `OSError` is raised.
   ##
@@ -1759,30 +1760,19 @@ proc copyFile*(source, dest: string, options = {cfSymlinkFollow}) {.rtl,
   doAssert card(copyFlagSymlink * options) == 1, "There should be exactly " &
                                                  "one cfSymlink* in options"
   let isSymlink = source.symlinkExists
-  if isSymlink and cfSymlinkIgnore in options:
+  if isSymlink and (cfSymlinkIgnore in options or defined(windows)):
     return
   when defined(Windows):
-    proc handleOSError =
-      const ERROR_PRIVILEGE_NOT_HELD = 1314
-      let errCode = osLastError()
-      let context = $(source, dest, options)
-      if isSymlink and errCode.int32 == ERROR_PRIVILEGE_NOT_HELD:
-        stderr.write("Failed copy the symlink: error " & $errCode & ": " &
-                     osErrorMsg(errCode) & "Additional info: " & context &
-                     "\n")
-      else:
-        raiseOSError(errCode, context)
-
     let dwCopyFlags = if cfSymlinkAsIs in options: COPY_FILE_COPY_SYMLINK else: 0'i32
     var pbCancel = 0'i32
     when useWinUnicode:
       let s = newWideCString(source)
       let d = newWideCString(dest)
       if copyFileExW(s, d, nil, nil, addr pbCancel, dwCopyFlags) == 0'i32:
-        handleOSError()
+        raiseOSError(osLastError(), $(source, dest))
     else:
       if copyFileExA(source, dest, nil, nil, addr pbCancel, dwCopyFlags) == 0'i32:
-        handleOSError()
+        raiseOSError(osLastError(), $(source, dest))
   elif hasCCopyfile:
     let state = copyfile_state_alloc()
     # xxx `COPYFILE_STAT` could be used for one-shot `copyFileWithPermissions`.
@@ -1824,8 +1814,9 @@ proc copyFileToDir*(source, dir: string, options = {cfSymlinkFollow})
   {.noWeirdTarget, since: (1,3,7).} =
   ## Copies a file `source` into directory `dir`, which must exist.
   ##
-  ## `options` specify the way file is copied; by default, if `source` is a
-  ## symlink, copies the file symlink points to.
+  ## On non-Windows OSes, `options` specify the way file is copied; by default,
+  ## if `source` is a symlink, copies the file symlink points to. `options` is
+  ## ignored on Windows: symlinks are skipped.
   ##
   ## See also:
   ## * `CopyFlag enum <#CopyFlag>`_
@@ -2460,7 +2451,8 @@ proc copyDir*(source, dest: string) {.rtl, extern: "nos$1",
   tags: [ReadDirEffect, WriteIOEffect, ReadIOEffect], benign, noWeirdTarget.} =
   ## Copies a directory from `source` to `dest`.
   ##
-  ## Symlinks are copied as symlinks.
+  ## On non-Windows OSes, symlinks are copied as symlinks. On Windows, symlinks
+  ## are skipped.
   ##
   ## If this fails, `OSError` is raised.
   ##
@@ -2491,8 +2483,8 @@ proc copyDir*(source, dest: string) {.rtl, extern: "nos$1",
 proc moveDir*(source, dest: string) {.tags: [ReadIOEffect, WriteIOEffect], noWeirdTarget.} =
   ## Moves a directory from `source` to `dest`.
   ##
-  ## Symlinks are not followed: if `source` contains symlinks, they itself are
-  ## moved, not theirs target.
+  ## Symlinks are not followed: if `source` contains symlinks, they themself are
+  ## moved, not their target.
   ##
   ## If this fails, `OSError` is raised.
   ##
@@ -2536,8 +2528,9 @@ proc copyFileWithPermissions*(source, dest: string,
                               options = {cfSymlinkFollow}) {.noWeirdTarget.} =
   ## Copies a file from `source` to `dest` preserving file permissions.
   ##
-  ## `options` specify the way file is copied; by default, if `source` is a
-  ## symlink, copies the file symlink points to.
+  ## On non-Windows OSes, `options` specify the way file is copied; by default,
+  ## if `source` is a symlink, copies the file symlink points to. `options` is
+  ## ignored on Windows: symlinks are skipped.
   ##
   ## This is a wrapper proc around `copyFile <#copyFile,string,string>`_,
   ## `getFilePermissions <#getFilePermissions,string>`_ and
@@ -2576,7 +2569,8 @@ proc copyDirWithPermissions*(source, dest: string,
    benign, noWeirdTarget.} =
   ## Copies a directory from `source` to `dest` preserving file permissions.
   ##
-  ## Symlinks are copied as symlinks.
+  ## On non-Windows OSes, symlinks are copied as symlinks. On Windows, symlinks
+  ## are skipped.
   ##
   ## If this fails, `OSError` is raised. This is a wrapper proc around `copyDir
   ## <#copyDir,string,string>`_ and `copyFileWithPermissions

lib/windows/winlean.nim

@@ -350,13 +350,6 @@ else:
 proc findClose*(hFindFile: Handle) {.stdcall, dynlib: "kernel32",
   importc: "FindClose".}
 
-type LPPROGRESS_ROUTINE* = proc(TotalFileSize, TotalBytesTransferred, 
-  StreamSize, StreamBytesTransferred: int64, dwStreamNumber, 
-  dwCallbackReason: DWORD, hSourceFile, hDestinationFile: Handle,
-  lpData: pointer): void {.stdcall.}
-
-const COPY_FILE_COPY_SYMLINK* = 0x00000800'i32
-
 when useWinUnicode:
   proc getFullPathNameW*(lpFileName: WideCString, nBufferLength: int32,
                         lpBuffer: WideCString,
@@ -373,10 +366,6 @@ when useWinUnicode:
   proc copyFileW*(lpExistingFileName, lpNewFileName: WideCString,
                  bFailIfExists: WINBOOL): WINBOOL {.
     importc: "CopyFileW", stdcall, dynlib: "kernel32", sideEffect.}
-  proc copyFileExW*(lpExistingFileName, lpNewFileName: WideCString,
-                    lpProgressRoutine: LPPROGRESS_ROUTINE, lpData: pointer,
-                    pbCancel: ptr WINBOOL, dwCopyFlags: DWORD): WINBOOL {.
-    importc: "CopyFileExW", stdcall, dynlib: "kernel32", sideEffect.}
 
   proc moveFileW*(lpExistingFileName, lpNewFileName: WideCString): WINBOOL {.
     importc: "MoveFileW", stdcall, dynlib: "kernel32", sideEffect.}
@@ -407,10 +396,6 @@ else:
   proc copyFileA*(lpExistingFileName, lpNewFileName: cstring,
                  bFailIfExists: cint): cint {.
     importc: "CopyFileA", stdcall, dynlib: "kernel32", sideEffect.}
-  proc copyFileExA*(lpExistingFileName, lpNewFileName: cstring,
-                    lpProgressRoutine: LPPROGRESS_ROUTINE, lpData: pointer,
-                    pbCancel: ptr WINBOOL, dwCopyFlags: DWORD): WINBOOL {.
-    importc: "CopyFileExA", stdcall, dynlib: "kernel32", sideEffect.}
 
   proc moveFileA*(lpExistingFileName, lpNewFileName: cstring): WINBOOL {.
     importc: "MoveFileA", stdcall, dynlib: "kernel32", sideEffect.}

tests/stdlib/tos.nim

@@ -158,11 +158,7 @@ block fileOperations:
   # Symlink handling in `copyFile`, `copyFileWithPermissions`, `copyFileToDir`,
   # `copyDir`, `copyDirWithPermissions`, `moveFile`, and `moveDir`.
   block:
-    when not defined(windows):
-      const checkExpandSymlink = true
-    else:
-      const checkExpandSymlink = false
-
+    const checkSymlink = not defined(windows)
     const dname = buildDir/"D20210116T140629"
     const subDir = dname/"sub"
     const subDir2 = dname/"sub2"
@@ -184,14 +180,14 @@ block fileOperations:
     copyFile(brokenSymlink, brokenSymlinkCopy, {cfSymlinkIgnore})
     doAssert not fileExists(brokenSymlinkCopy)
     copyFile(brokenSymlink, brokenSymlinkCopy, {cfSymlinkAsIs})
-    when checkExpandSymlink:
+    when checkSymlink:
       doAssert expandSymlink(brokenSymlinkCopy) == brokenSymlinkSrc
+      removeFile(brokenSymlinkCopy)
     else:
-      doAssert symlinkExists(brokenSymlinkCopy)
-    removeFile(brokenSymlinkCopy)
+      doAssert not fileExists(brokenSymlinkCopy)
     doAssertRaises(AssertionDefect):
       copyFile(brokenSymlink, brokenSymlinkCopy,
-                {cfSymlinkAsIs, cfSymlinkFollow})
+               {cfSymlinkAsIs, cfSymlinkFollow})
 
     # Test copyFileWithPermissions
     doAssertRaises(OSError):
@@ -204,11 +200,11 @@ block fileOperations:
     doAssert not fileExists(brokenSymlinkCopy)
     copyFileWithPermissions(brokenSymlink, brokenSymlinkCopy,
                             options = {cfSymlinkAsIs})
-    when checkExpandSymlink:
+    when checkSymlink:
       doAssert expandSymlink(brokenSymlinkCopy) == brokenSymlinkSrc
+      removeFile(brokenSymlinkCopy)
     else:
-      doAssert symlinkExists(brokenSymlinkCopy)
-    removeFile(brokenSymlinkCopy)
+      doAssert not fileExists(brokenSymlinkCopy)
     doAssertRaises(AssertionDefect):
       copyFileWithPermissions(brokenSymlink, brokenSymlinkCopy,
                               options = {cfSymlinkAsIs, cfSymlinkFollow})
@@ -221,44 +217,44 @@ block fileOperations:
     copyFileToDir(brokenSymlink, subDir, {cfSymlinkIgnore})
     doAssert not fileExists(brokenSymlinkInSubDir)
     copyFileToDir(brokenSymlink, subDir, {cfSymlinkAsIs})
-    when checkExpandSymlink:
+    when checkSymlink:
       doAssert expandSymlink(brokenSymlinkInSubDir) == brokenSymlinkSrc
+      removeFile(brokenSymlinkInSubDir)
     else:
-      doAssert symlinkExists(brokenSymlinkInSubDir)
-    removeFile(brokenSymlinkInSubDir)
+      doAssert not fileExists(brokenSymlinkInSubDir)
 
     createSymlink(brokenSymlinkSrc, brokenSymlinkInSubDir)
 
     # Test copyDir
     copyDir(subDir, subDir2)
-    when checkExpandSymlink:
+    when checkSymlink:
       doAssert expandSymlink(brokenSymlinkInSubDir2) == brokenSymlinkSrc
     else:
-      doAssert symlinkExists(brokenSymlinkInSubDir2)
+      doAssert not fileExists(brokenSymlinkInSubDir2)
     removeDir(subDir2)
 
     # Test copyDirWithPermissions
     copyDirWithPermissions(subDir, subDir2)
-    when checkExpandSymlink:
+    when checkSymlink:
       doAssert expandSymlink(brokenSymlinkInSubDir2) == brokenSymlinkSrc
     else:
-      doAssert symlinkExists(brokenSymlinkInSubDir2)
+      doAssert not fileExists(brokenSymlinkInSubDir2)
     removeDir(subDir2)
 
     # Test moveFile
     moveFile(brokenSymlink, brokenSymlinkCopy)
-    when checkExpandSymlink:
+    when checkSymlink:
       doAssert expandSymlink(brokenSymlinkCopy) == brokenSymlinkSrc
     else:
-      doAssert symlinkExists(brokenSymlinkCopy)
+      doAssert not fileExists(brokenSymlinkCopy)
     removeFile(brokenSymlinkCopy)
 
     # Test moveDir
     moveDir(subDir, subDir2)
-    when checkExpandSymlink:
+    when checkSymlink:
       doAssert expandSymlink(brokenSymlinkInSubDir2) == brokenSymlinkSrc
     else:
-      doAssert symlinkExists(brokenSymlinkInSubDir2)
+      doAssert not fileExists(brokenSymlinkInSubDir2)
 
     removeDir(dname)