Skip to content

Commit c704902

Browse files
author
Roman Inflianskas
committed
Address comments in #16709 (second iteration)
Skip symlinks on Windows.
1 parent d3bd6dc commit c704902

File tree

4 files changed

+63
-85
lines changed

4 files changed

+63
-85
lines changed

‎changelog.md‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -114,12 +114,14 @@ with other backends. see #9125. Use `-d:nimLegacyJsRound` for previous behavior.
114114

115115

116116
- Added optional `options` argument to `copyFile`, `copyFileToDir`, and
117-
`copyFileWithPermissions`. By default, symlinks are followed (copy files
118-
symlinks point to).
119-
- `copyDir` and `copyDirWithPermissions` copy symlinks as symlinks (instead of
120-
skipping them as it was before).
121-
- `moveFile` and `moveDir` move symlinks as symlinks (instead of skipping them
122-
sometimes as it was before).
117+
`copyFileWithPermissions`. By default, on non-Windows OSes, symlinks are
118+
followed (copy files symlinks point to); on Windows, `options` argument is
119+
ignored and symlinks are skipped.
120+
- On non-Windows OSes, `copyDir` and `copyDirWithPermissions` copy symlinks as
121+
symlinks (instead of skipping them as it was before); on Windows symlinks are
122+
skipped.
123+
- On non-Windows OSes, `moveFile` and `moveDir` move symlinks as symlinks
124+
(instead of skipping them sometimes as it was before).
123125
- Added optional `followSymlinks` argument to `setFilePermissions`.
124126

125127
## Language changes

‎lib/pure/os.nim‎

Lines changed: 20 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1725,8 +1725,9 @@ proc copyFile*(source, dest: string, options = {cfSymlinkFollow}) {.rtl,
17251725
noWeirdTarget.} =
17261726
## Copies a file from `source` to `dest`, where `dest.parentDir` must exist.
17271727
##
1728-
## `options` specify the way file is copied; by default, if `source` is a
1729-
## symlink, copies the file symlink points to.
1728+
## On non-Windows OSes, `options` specify the way file is copied; by default,
1729+
## if `source` is a symlink, copies the file symlink points to. `options` is
1730+
## ignored on Windows: symlinks are skipped.
17301731
##
17311732
## If this fails, `OSError` is raised.
17321733
##
@@ -1759,30 +1760,17 @@ proc copyFile*(source, dest: string, options = {cfSymlinkFollow}) {.rtl,
17591760
doAssert card(copyFlagSymlink * options) == 1, "There should be exactly " &
17601761
"one cfSymlink* in options"
17611762
let isSymlink = source.symlinkExists
1762-
if isSymlink and cfSymlinkIgnore in options:
1763+
if isSymlink and (cfSymlinkIgnore in options or defined(windows)):
17631764
return
17641765
when defined(Windows):
1765-
proc handleOSError =
1766-
const ERROR_PRIVILEGE_NOT_HELD = 1314
1767-
let errCode = osLastError()
1768-
let context = $(source, dest, options)
1769-
if isSymlink and errCode.int32 == ERROR_PRIVILEGE_NOT_HELD:
1770-
stderr.write("Failed copy the symlink: error " & $errCode & ": " &
1771-
osErrorMsg(errCode) & "Additional info: " & context &
1772-
"\n")
1773-
else:
1774-
raiseOSError(errCode, context)
1775-
1776-
let dwCopyFlags = if cfSymlinkAsIs in options: COPY_FILE_COPY_SYMLINK else: 0'i32
1777-
var pbCancel = 0'i32
17781766
when useWinUnicode:
17791767
let s = newWideCString(source)
17801768
let d = newWideCString(dest)
1781-
if copyFileExW(s, d, nil, nil, addr pbCancel, dwCopyFlags) == 0'i32:
1782-
handleOSError()
1769+
if copyFileW(s, d, 0'i32) == 0'i32:
1770+
raiseOSError(osLastError(), $(source, dest))
17831771
else:
1784-
if copyFileExA(source, dest, nil, nil, addr pbCancel, dwCopyFlags) == 0'i32:
1785-
handleOSError()
1772+
if copyFileA(source, dest, 0'i32) == 0'i32:
1773+
raiseOSError(osLastError(), $(source, dest))
17861774
elif hasCCopyfile:
17871775
let state = copyfile_state_alloc()
17881776
# xxx `COPYFILE_STAT` could be used for one-shot `copyFileWithPermissions`.
@@ -1824,8 +1812,9 @@ proc copyFileToDir*(source, dir: string, options = {cfSymlinkFollow})
18241812
{.noWeirdTarget, since: (1,3,7).} =
18251813
## Copies a file `source` into directory `dir`, which must exist.
18261814
##
1827-
## `options` specify the way file is copied; by default, if `source` is a
1828-
## symlink, copies the file symlink points to.
1815+
## On non-Windows OSes, `options` specify the way file is copied; by default,
1816+
## if `source` is a symlink, copies the file symlink points to. `options` is
1817+
## ignored on Windows: symlinks are skipped.
18291818
##
18301819
## See also:
18311820
## * `CopyFlag enum <#CopyFlag>`_
@@ -2460,7 +2449,8 @@ proc copyDir*(source, dest: string) {.rtl, extern: "nos$1",
24602449
tags: [ReadDirEffect, WriteIOEffect, ReadIOEffect], benign, noWeirdTarget.} =
24612450
## Copies a directory from `source` to `dest`.
24622451
##
2463-
## Symlinks are copied as symlinks.
2452+
## On non-Windows OSes, symlinks are copied as symlinks. On Windows, symlinks
2453+
## are skipped.
24642454
##
24652455
## If this fails, `OSError` is raised.
24662456
##
@@ -2491,8 +2481,8 @@ proc copyDir*(source, dest: string) {.rtl, extern: "nos$1",
24912481
proc moveDir*(source, dest: string) {.tags: [ReadIOEffect, WriteIOEffect], noWeirdTarget.} =
24922482
## Moves a directory from `source` to `dest`.
24932483
##
2494-
## Symlinks are not followed: if `source` contains symlinks, they itself are
2495-
## moved, not theirs target.
2484+
## Symlinks are not followed: if `source` contains symlinks, they themself are
2485+
## moved, not their target.
24962486
##
24972487
## If this fails, `OSError` is raised.
24982488
##
@@ -2536,8 +2526,9 @@ proc copyFileWithPermissions*(source, dest: string,
25362526
options = {cfSymlinkFollow}) {.noWeirdTarget.} =
25372527
## Copies a file from `source` to `dest` preserving file permissions.
25382528
##
2539-
## `options` specify the way file is copied; by default, if `source` is a
2540-
## symlink, copies the file symlink points to.
2529+
## On non-Windows OSes, `options` specify the way file is copied; by default,
2530+
## if `source` is a symlink, copies the file symlink points to. `options` is
2531+
## ignored on Windows: symlinks are skipped.
25412532
##
25422533
## This is a wrapper proc around `copyFile <#copyFile,string,string>`_,
25432534
## `getFilePermissions <#getFilePermissions,string>`_ and
@@ -2576,7 +2567,8 @@ proc copyDirWithPermissions*(source, dest: string,
25762567
benign, noWeirdTarget.} =
25772568
## Copies a directory from `source` to `dest` preserving file permissions.
25782569
##
2579-
## Symlinks are copied as symlinks.
2570+
## On non-Windows OSes, symlinks are copied as symlinks. On Windows, symlinks
2571+
## are skipped.
25802572
##
25812573
## If this fails, `OSError` is raised. This is a wrapper proc around `copyDir
25822574
## <#copyDir,string,string>`_ and `copyFileWithPermissions

‎lib/windows/winlean.nim‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -350,13 +350,6 @@ else:
350350
proc findClose*(hFindFile: Handle) {.stdcall, dynlib: "kernel32",
351351
importc: "FindClose".}
352352

353-
type LPPROGRESS_ROUTINE* = proc(TotalFileSize, TotalBytesTransferred,
354-
StreamSize, StreamBytesTransferred: int64, dwStreamNumber,
355-
dwCallbackReason: DWORD, hSourceFile, hDestinationFile: Handle,
356-
lpData: pointer): void {.stdcall.}
357-
358-
const COPY_FILE_COPY_SYMLINK* = 0x00000800'i32
359-
360353
when useWinUnicode:
361354
proc getFullPathNameW*(lpFileName: WideCString, nBufferLength: int32,
362355
lpBuffer: WideCString,
@@ -373,10 +366,6 @@ when useWinUnicode:
373366
proc copyFileW*(lpExistingFileName, lpNewFileName: WideCString,
374367
bFailIfExists: WINBOOL): WINBOOL {.
375368
importc: "CopyFileW", stdcall, dynlib: "kernel32", sideEffect.}
376-
proc copyFileExW*(lpExistingFileName, lpNewFileName: WideCString,
377-
lpProgressRoutine: LPPROGRESS_ROUTINE, lpData: pointer,
378-
pbCancel: ptr WINBOOL, dwCopyFlags: DWORD): WINBOOL {.
379-
importc: "CopyFileExW", stdcall, dynlib: "kernel32", sideEffect.}
380369

381370
proc moveFileW*(lpExistingFileName, lpNewFileName: WideCString): WINBOOL {.
382371
importc: "MoveFileW", stdcall, dynlib: "kernel32", sideEffect.}
@@ -407,10 +396,6 @@ else:
407396
proc copyFileA*(lpExistingFileName, lpNewFileName: cstring,
408397
bFailIfExists: cint): cint {.
409398
importc: "CopyFileA", stdcall, dynlib: "kernel32", sideEffect.}
410-
proc copyFileExA*(lpExistingFileName, lpNewFileName: cstring,
411-
lpProgressRoutine: LPPROGRESS_ROUTINE, lpData: pointer,
412-
pbCancel: ptr WINBOOL, dwCopyFlags: DWORD): WINBOOL {.
413-
importc: "CopyFileExA", stdcall, dynlib: "kernel32", sideEffect.}
414399

415400
proc moveFileA*(lpExistingFileName, lpNewFileName: cstring): WINBOOL {.
416401
importc: "MoveFileA", stdcall, dynlib: "kernel32", sideEffect.}

‎tests/stdlib/tos.nim‎

Lines changed: 35 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -158,11 +158,7 @@ block fileOperations:
158158
# Symlink handling in `copyFile`, `copyFileWithPermissions`, `copyFileToDir`,
159159
# `copyDir`, `copyDirWithPermissions`, `moveFile`, and `moveDir`.
160160
block:
161-
when not defined(windows):
162-
const checkExpandSymlink = true
163-
else:
164-
const checkExpandSymlink = false
165-
161+
const symlinksAreHandled = not defined(windows)
166162
const dname = buildDir/"D20210116T140629"
167163
const subDir = dname/"sub"
168164
const subDir2 = dname/"sub2"
@@ -177,88 +173,91 @@ block fileOperations:
177173
createSymlink(brokenSymlinkSrc, brokenSymlink)
178174

179175
# Test copyFile
180-
doAssertRaises(OSError):
181-
copyFile(brokenSymlink, brokenSymlinkCopy)
182-
doAssertRaises(OSError):
183-
copyFile(brokenSymlink, brokenSymlinkCopy, {cfSymlinkFollow})
176+
when symlinksAreHandled:
177+
doAssertRaises(OSError):
178+
copyFile(brokenSymlink, brokenSymlinkCopy)
179+
doAssertRaises(OSError):
180+
copyFile(brokenSymlink, brokenSymlinkCopy, {cfSymlinkFollow})
184181
copyFile(brokenSymlink, brokenSymlinkCopy, {cfSymlinkIgnore})
185182
doAssert not fileExists(brokenSymlinkCopy)
186183
copyFile(brokenSymlink, brokenSymlinkCopy, {cfSymlinkAsIs})
187-
when checkExpandSymlink:
184+
when symlinksAreHandled:
188185
doAssert expandSymlink(brokenSymlinkCopy) == brokenSymlinkSrc
186+
removeFile(brokenSymlinkCopy)
189187
else:
190-
doAssert symlinkExists(brokenSymlinkCopy)
191-
removeFile(brokenSymlinkCopy)
188+
doAssert not fileExists(brokenSymlinkCopy)
192189
doAssertRaises(AssertionDefect):
193190
copyFile(brokenSymlink, brokenSymlinkCopy,
194-
{cfSymlinkAsIs, cfSymlinkFollow})
191+
{cfSymlinkAsIs, cfSymlinkFollow})
195192

196193
# Test copyFileWithPermissions
197-
doAssertRaises(OSError):
198-
copyFileWithPermissions(brokenSymlink, brokenSymlinkCopy)
199-
doAssertRaises(OSError):
200-
copyFileWithPermissions(brokenSymlink, brokenSymlinkCopy,
201-
options = {cfSymlinkFollow})
194+
when symlinksAreHandled:
195+
doAssertRaises(OSError):
196+
copyFileWithPermissions(brokenSymlink, brokenSymlinkCopy)
197+
doAssertRaises(OSError):
198+
copyFileWithPermissions(brokenSymlink, brokenSymlinkCopy,
199+
options = {cfSymlinkFollow})
202200
copyFileWithPermissions(brokenSymlink, brokenSymlinkCopy,
203201
options = {cfSymlinkIgnore})
204202
doAssert not fileExists(brokenSymlinkCopy)
205203
copyFileWithPermissions(brokenSymlink, brokenSymlinkCopy,
206204
options = {cfSymlinkAsIs})
207-
when checkExpandSymlink:
205+
when symlinksAreHandled:
208206
doAssert expandSymlink(brokenSymlinkCopy) == brokenSymlinkSrc
207+
removeFile(brokenSymlinkCopy)
209208
else:
210-
doAssert symlinkExists(brokenSymlinkCopy)
211-
removeFile(brokenSymlinkCopy)
209+
doAssert not fileExists(brokenSymlinkCopy)
212210
doAssertRaises(AssertionDefect):
213211
copyFileWithPermissions(brokenSymlink, brokenSymlinkCopy,
214212
options = {cfSymlinkAsIs, cfSymlinkFollow})
215213

216214
# Test copyFileToDir
217-
doAssertRaises(OSError):
218-
copyFileToDir(brokenSymlink, subDir)
219-
doAssertRaises(OSError):
220-
copyFileToDir(brokenSymlink, subDir, {cfSymlinkFollow})
215+
when symlinksAreHandled:
216+
doAssertRaises(OSError):
217+
copyFileToDir(brokenSymlink, subDir)
218+
doAssertRaises(OSError):
219+
copyFileToDir(brokenSymlink, subDir, {cfSymlinkFollow})
221220
copyFileToDir(brokenSymlink, subDir, {cfSymlinkIgnore})
222221
doAssert not fileExists(brokenSymlinkInSubDir)
223222
copyFileToDir(brokenSymlink, subDir, {cfSymlinkAsIs})
224-
when checkExpandSymlink:
223+
when symlinksAreHandled:
225224
doAssert expandSymlink(brokenSymlinkInSubDir) == brokenSymlinkSrc
225+
removeFile(brokenSymlinkInSubDir)
226226
else:
227-
doAssert symlinkExists(brokenSymlinkInSubDir)
228-
removeFile(brokenSymlinkInSubDir)
227+
doAssert not fileExists(brokenSymlinkInSubDir)
229228

230229
createSymlink(brokenSymlinkSrc, brokenSymlinkInSubDir)
231230

232231
# Test copyDir
233232
copyDir(subDir, subDir2)
234-
when checkExpandSymlink:
233+
when symlinksAreHandled:
235234
doAssert expandSymlink(brokenSymlinkInSubDir2) == brokenSymlinkSrc
236235
else:
237-
doAssert symlinkExists(brokenSymlinkInSubDir2)
236+
doAssert not fileExists(brokenSymlinkInSubDir2)
238237
removeDir(subDir2)
239238

240239
# Test copyDirWithPermissions
241240
copyDirWithPermissions(subDir, subDir2)
242-
when checkExpandSymlink:
241+
when symlinksAreHandled:
243242
doAssert expandSymlink(brokenSymlinkInSubDir2) == brokenSymlinkSrc
244243
else:
245-
doAssert symlinkExists(brokenSymlinkInSubDir2)
244+
doAssert not fileExists(brokenSymlinkInSubDir2)
246245
removeDir(subDir2)
247246

248247
# Test moveFile
249248
moveFile(brokenSymlink, brokenSymlinkCopy)
250-
when checkExpandSymlink:
249+
when symlinksAreHandled:
251250
doAssert expandSymlink(brokenSymlinkCopy) == brokenSymlinkSrc
252251
else:
253-
doAssert symlinkExists(brokenSymlinkCopy)
252+
doAssert not fileExists(brokenSymlinkCopy)
254253
removeFile(brokenSymlinkCopy)
255254

256255
# Test moveDir
257256
moveDir(subDir, subDir2)
258-
when checkExpandSymlink:
257+
when symlinksAreHandled:
259258
doAssert expandSymlink(brokenSymlinkInSubDir2) == brokenSymlinkSrc
260259
else:
261-
doAssert symlinkExists(brokenSymlinkInSubDir2)
260+
doAssert not fileExists(brokenSymlinkInSubDir2)
262261

263262
removeDir(dname)
264263

0 commit comments

Comments
 (0)