Address comments in #16709

Co-authored-by: Timothee Cour <[email protected]>

Author: Roman Inflianskas

changelog.md

@@ -118,7 +118,7 @@ with other backends. see #9125. Use `-d:nimLegacyJsRound` for previous behavior.
   symlinks point to).
 - `copyDir` and `copyDirWithPermissions` copy symlinks as symlinks (instead of
   skipping them as it was before).
-- `moveFile` and `moveDir` move symlinks as they are (instead of skipping them
+- `moveFile` and `moveDir` move symlinks as symlinks (instead of skipping them
   sometimes as it was before).
 - Added optional `followSymlinks` argument to `setFilePermissions`.
 

lib/posix/posix.nim

@@ -586,7 +586,7 @@ proc fstatvfs*(a1: cint, a2: var Statvfs): cint {.
   importc, header: "<sys/statvfs.h>".}
 
 proc chmod*(a1: cstring, a2: Mode): cint {.importc, header: "<sys/stat.h>", sideEffect.}
-when hasLchmod:
+when defined(osx) or defined(freebsd):
   proc lchmod*(a1: cstring, a2: Mode): cint {.importc, header: "<sys/stat.h>", sideEffect.}
 proc fchmod*(a1: cint, a2: Mode): cint {.importc, header: "<sys/stat.h>", sideEffect.}
 proc fstat*(a1: cint, a2: var Stat): cint {.importc, header: "<sys/stat.h>", sideEffect.}

lib/posix/posix_haiku.nim

@@ -13,7 +13,6 @@ when defined(nimHasStyleChecks):
 const
   hasSpawnH = true # should exist for every Posix system nowadays
   hasAioH = defined(linux)
-  hasLchmod* = false
 
 when defined(linux) and not defined(android):
   # On Linux:

lib/posix/posix_linux_amd64.nim

@@ -15,7 +15,6 @@
 const
   hasSpawnH = not defined(haiku) # should exist for every Posix system nowadays
   hasAioH = defined(linux)
-  hasLchmod* = false
 
 # On Linux:
 # timer_{create,delete,settime,gettime},

lib/posix/posix_macos_amd64.nim

@@ -13,7 +13,6 @@ when defined(nimHasStyleChecks):
 const
   hasSpawnH = true # should exist for every Posix system nowadays
   hasAioH = false
-  hasLchmod* = true
 
 type
   DIR* {.importc: "DIR", header: "<dirent.h>",

lib/posix/posix_nintendoswitch.nim

@@ -12,7 +12,6 @@
 const
   hasSpawnH = true
   hasAioH = false
-  hasLchmod* = false
 
 type
   DIR* {.importc: "DIR", header: "<dirent.h>",

lib/posix/posix_openbsd_amd64.nim

@@ -13,7 +13,6 @@ when defined(nimHasStyleChecks):
 const
   hasSpawnH = true # should exist for every Posix system nowadays
   hasAioH = false
-  hasLchmod* = false
 
 type
   DIR* {.importc: "DIR", header: "<dirent.h>",

lib/posix/posix_other.nim

@@ -14,12 +14,10 @@ when defined(freertos):
   const
     hasSpawnH = false # should exist for every Posix system nowadays
     hasAioH = false
-    hasLchmod* = false
 else:
   const
     hasSpawnH = true # should exist for every Posix system nowadays
     hasAioH = defined(linux)
-    hasLchmod* = false
 
 when defined(linux) and not defined(android):
   # On Linux:

lib/pure/os.nim

@@ -1598,10 +1598,11 @@ proc setFilePermissions*(filename: string, permissions: set[FilePermission],
    noWeirdTarget.} =
   ## Sets the file permissions for `filename`.
   ##
-  ## If ``followSymlinks`` set to true (default) and ``filename`` points to a
+  ## If `followSymlinks` set to true (default) and ``filename`` points to a
   ## symlink, permissions are set to the file symlink points to.
-  ## ``followSymlinks`` set to false do not affect on Windows and some POSIX
-  ## systems (including Linux) which do not have ``lchmod`` available.
+  ## `followSymlinks` set to false is a noop on Windows and some POSIX
+  ## systems (including Linux) on which `lchmod` is either unavailable or always
+  ## fails, given that symlinks permissions there are not observed.
   ##
   ## `OSError` is raised in case of an error.
   ## On Windows, only the ``readonly`` flag is changed, depending on
@@ -1625,7 +1626,7 @@ proc setFilePermissions*(filename: string, permissions: set[FilePermission],
     if fpOthersExec in permissions: p = p or S_IXOTH.Mode
 
     if not followSymlinks and filename.symlinkExists:
-      when hasLchmod:
+      when declared(lchmod):
         if lchmod(filename, cast[Mode](p)) != 0:
           raiseOSError(osLastError(), $(filename, permissions))
     else:
@@ -1653,16 +1654,16 @@ proc createSymlink*(src, dest: string) {.noWeirdTarget.} =
   ##
   ## **Warning**:
   ## Some OS's (such as Microsoft Windows) restrict the creation
-  ## of symlinks to root users (administrators).
+  ## of symlinks to root users (administrators) or users with developper mode enabled.
   ##
   ## See also:
   ## * `createHardlink proc <#createHardlink,string,string>`_
   ## * `expandSymlink proc <#expandSymlink,string>`_
 
   when defined(Windows):
-    # 2 is the SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE. This allows
-    # anyone with developer mode on to create a link
-    let flag = dirExists(src).int32 or 2
+    const SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE = 2
+    # allows anyone with developer mode on to create a link
+    let flag = dirExists(src).int32 or SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE
     when useWinUnicode:
       var wSrc = newWideCString(src)
       var wDst = newWideCString(dest)
@@ -1678,7 +1679,7 @@ proc createSymlink*(src, dest: string) {.noWeirdTarget.} =
 proc expandSymlink*(symlinkPath: string): string {.noWeirdTarget.} =
   ## Returns a string representing the path to which the symbolic link points.
   ##
-  ## On Windows this is a noop, ``symlinkPath`` is simply returned.
+  ## On Windows this is a noop, `symlinkPath` is simply returned.
   ##
   ## See also:
   ## * `createSymlink proc <#createSymlink,string,string>`_
@@ -1761,16 +1762,27 @@ proc copyFile*(source, dest: string, options = {cfSymlinkFollow}) {.rtl,
   if isSymlink and cfSymlinkIgnore in options:
     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:
-        raiseOSError(osLastError(), $(source, dest, options))
+        handleOSError()
     else:
       if copyFileExA(source, dest, nil, nil, addr pbCancel, dwCopyFlags) == 0'i32:
-        raiseOSError(osLastError(), $(source, dest, options))
+        handleOSError()
   elif hasCCopyfile:
     let state = copyfile_state_alloc()
     # xxx `COPYFILE_STAT` could be used for one-shot `copyFileWithPermissions`.

tests/stdlib/tos.nim

@@ -26,6 +26,7 @@ Raises
 # test os path creation, iteration, and deletion
 
 import os, strutils, pathnorm
+from stdtest/specialpaths import buildDir
 
 block fileOperations:
   let files = @["these.txt", "are.x", "testing.r", "files.q"]
@@ -162,13 +163,12 @@ block fileOperations:
     else:
       const checkExpandSymlink = false
 
-    const buildDir = currentSourcePath.parentDir.parentDir/"build"
     const dname = buildDir/"D20210116T140629"
     const subDir = dname/"sub"
     const subDir2 = dname/"sub2"
     const brokenSymlinkName = "D20210101T191320_BROKEN_SYMLINK"
     const brokenSymlink = dname/brokenSymlinkName
-    const brokenSymlinkSrc = "D20210101T191320_I_DO_NOT_EXIST"
+    const brokenSymlinkSrc = "D20210101T191320_nonexistant"
     const brokenSymlinkCopy = brokenSymlink & "_COPY"
     const brokenSymlinkInSubDir = subDir/brokenSymlinkName
     const brokenSymlinkInSubDir2 = subDir2/brokenSymlinkName