From d3bd6dca1d2c9658f53e042cb14b795c4616dcef Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Sun, 31 Jan 2021 09:04:51 +0200 Subject: [PATCH] Address comments in #16709 Co-authored-by: Timothee Cour --- changelog.md | 2 +- lib/posix/posix.nim | 2 +- lib/posix/posix_haiku.nim | 1 - lib/posix/posix_linux_amd64.nim | 1 - lib/posix/posix_macos_amd64.nim | 1 - lib/posix/posix_nintendoswitch.nim | 1 - lib/posix/posix_openbsd_amd64.nim | 1 - lib/posix/posix_other.nim | 2 -- lib/pure/os.nim | 34 ++++++++++++++++++++---------- tests/stdlib/tos.nim | 4 ++-- tests/test_nimscript.nims | 5 +++-- 11 files changed, 30 insertions(+), 24 deletions(-) diff --git a/changelog.md b/changelog.md index e9cc3a35f2ea..26c24f215849 100644 --- a/changelog.md +++ b/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`. diff --git a/lib/posix/posix.nim b/lib/posix/posix.nim index db5e2d681abb..e8ad786e90b5 100644 --- a/lib/posix/posix.nim +++ b/lib/posix/posix.nim @@ -586,7 +586,7 @@ proc fstatvfs*(a1: cint, a2: var Statvfs): cint {. importc, header: "".} proc chmod*(a1: cstring, a2: Mode): cint {.importc, header: "", sideEffect.} -when hasLchmod: +when defined(osx) or defined(freebsd): proc lchmod*(a1: cstring, a2: Mode): cint {.importc, header: "", sideEffect.} proc fchmod*(a1: cint, a2: Mode): cint {.importc, header: "", sideEffect.} proc fstat*(a1: cint, a2: var Stat): cint {.importc, header: "", sideEffect.} diff --git a/lib/posix/posix_haiku.nim b/lib/posix/posix_haiku.nim index ab977527d607..eaf2cfb857f5 100644 --- a/lib/posix/posix_haiku.nim +++ b/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: diff --git a/lib/posix/posix_linux_amd64.nim b/lib/posix/posix_linux_amd64.nim index 9e7fe192082b..7f6a589f0850 100644 --- a/lib/posix/posix_linux_amd64.nim +++ b/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}, diff --git a/lib/posix/posix_macos_amd64.nim b/lib/posix/posix_macos_amd64.nim index f408c93399d9..94c08acad468 100644 --- a/lib/posix/posix_macos_amd64.nim +++ b/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: "", diff --git a/lib/posix/posix_nintendoswitch.nim b/lib/posix/posix_nintendoswitch.nim index 19ce37a22dc0..44e431437995 100644 --- a/lib/posix/posix_nintendoswitch.nim +++ b/lib/posix/posix_nintendoswitch.nim @@ -12,7 +12,6 @@ const hasSpawnH = true hasAioH = false - hasLchmod* = false type DIR* {.importc: "DIR", header: "", diff --git a/lib/posix/posix_openbsd_amd64.nim b/lib/posix/posix_openbsd_amd64.nim index d342f50bc021..584d0650116f 100644 --- a/lib/posix/posix_openbsd_amd64.nim +++ b/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: "", diff --git a/lib/posix/posix_other.nim b/lib/posix/posix_other.nim index 1492f57d75c3..a5eb32d2267f 100644 --- a/lib/posix/posix_other.nim +++ b/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: diff --git a/lib/pure/os.nim b/lib/pure/os.nim index 5e6f0df57713..443cabb5bf63 100644 --- a/lib/pure/os.nim +++ b/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`. diff --git a/tests/stdlib/tos.nim b/tests/stdlib/tos.nim index dac06e512759..c22682561bd6 100644 --- a/tests/stdlib/tos.nim +++ b/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 diff --git a/tests/test_nimscript.nims b/tests/test_nimscript.nims index b492bb529c69..9bfdff55ec80 100644 --- a/tests/test_nimscript.nims +++ b/tests/test_nimscript.nims @@ -3,6 +3,8 @@ {.warning[UnusedImport]: off.} +from stdtest/specialpaths import buildDir + import std/[ # Core: bitops, typetraits, lenientops, macros, volatile, @@ -88,8 +90,7 @@ block: except Exception as e: discard -block: # #16709 - const buildDir = currentSourcePath.parentDir.parentDir/"build" +block: # cpDir, cpFile, dirExists, fileExists, mkDir, mvDir, mvFile, rmDir, rmFile const dname = buildDir/"D20210121T175016" const subDir = dname/"sub" const subDir2 = dname/"sub2"