Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] 'walkDir' now has a new 'checkDir' flag, to mimic behaviour of other languages #13642

Merged
merged 8 commits into from
Mar 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
on GCC's `__builtin_sadd_overflow` family of functions. (Clang also
supports these). Some versions of GCC lack this feature and unfortunately
we cannot detect this case reliably. So if you get compilation errors like
"undefined reference to '__builtin_saddll_overflow'" compile your programs
"undefined reference to `__builtin_saddll_overflow`" compile your programs
with `-d:nimEmulateOverflowChecks`.


Expand All @@ -34,8 +34,8 @@
- `options` now treats `proc` like other pointer types, meaning `nil` proc variables
are converted to `None`.
- `relativePath("foo", "foo")` is now `"."`, not `""`, as `""` means invalid path
and shouldn't be conflated with `"."`; use -d:nimOldRelativePathBehavior to restore the old
behavior
and shouldn't be conflated with `"."`; use -d:nimOldRelativePathBehavior to
restore the old behavior
- `joinPath(a,b)` now honors trailing slashes in `b` (or `a` if `b` = "")
- `times.parse` now only uses input to compute its result, and not `now`:
`parse("2020", "YYYY", utc())` is now `2020-01-01T00:00:00Z` instead of
Expand All @@ -44,6 +44,12 @@
- `httpcore.==(string, HttpCode)` is now deprecated due to lack of practical
usage. The `$` operator can be used to obtain the string form of `HttpCode`
for comparison if desired.
- `os.walkDir` and `os.walkDirRec` now have new flag, `checkDir` (default: false).
If it is set to true, it will throw if input dir is invalid instead of a noop
(which is the default behaviour, as it was before this change),
`os.walkDirRec` only throws if top-level dir is invalid, but ignores errors for
subdirs, otherwise it would be impossible to resume iteration.


### Breaking changes in the compiler

Expand Down
2 changes: 1 addition & 1 deletion compiler/scriptconfig.nim
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ proc setupVM*(module: PSym; cache: IdentCache; scriptName: string;
if defined(nimsuggest) or graph.config.cmd == cmdCheck:
discard
else:
os.removeDir getString(a, 0)
os.removeDir(getString(a, 0), getBool(a, 1))
cbos removeFile:
if defined(nimsuggest) or graph.config.cmd == cmdCheck:
discard
Expand Down
36 changes: 25 additions & 11 deletions lib/pure/os.nim
Original file line number Diff line number Diff line change
Expand Up @@ -2028,8 +2028,8 @@ proc staticWalkDir(dir: string; relative: bool): seq[
tuple[kind: PathComponent, path: string]] =
discard

iterator walkDir*(dir: string; relative=false): tuple[kind: PathComponent, path: string] {.
tags: [ReadDirEffect].} =
iterator walkDir*(dir: string; relative = false, checkDir = false):
tuple[kind: PathComponent, path: string] {.tags: [ReadDirEffect].} =
## Walks over the directory `dir` and yields for each directory or file in
## `dir`. The component type and full path for each item are returned.
##
Expand All @@ -2038,7 +2038,6 @@ iterator walkDir*(dir: string; relative=false): tuple[kind: PathComponent, path:
## Example: This directory structure::
## dirA / dirB / fileB1.txt
## / dirC
## / fileA1.txt
## / fileA2.txt
##
## and this code:
Expand Down Expand Up @@ -2069,7 +2068,10 @@ iterator walkDir*(dir: string; relative=false): tuple[kind: PathComponent, path:
elif defined(windows):
var f: WIN32_FIND_DATA
var h = findFirstFile(dir / "*", f)
if h != -1:
if h == -1:
if checkDir:
raiseOSError(osLastError(), dir)
else:
defer: findClose(h)
while true:
var k = pcFile
Expand All @@ -2087,7 +2089,10 @@ iterator walkDir*(dir: string; relative=false): tuple[kind: PathComponent, path:
else: raiseOSError(errCode.OSErrorCode)
else:
var d = opendir(dir)
if d != nil:
if d == nil:
if checkDir:
raiseOSError(osLastError(), dir)
else:
defer: discard closedir(d)
while true:
var x = readdir(d)
Expand Down Expand Up @@ -2122,7 +2127,7 @@ iterator walkDir*(dir: string; relative=false): tuple[kind: PathComponent, path:

iterator walkDirRec*(dir: string,
yieldFilter = {pcFile}, followFilter = {pcDir},
relative = false): string {.tags: [ReadDirEffect].} =
relative = false, checkDir = false): string {.tags: [ReadDirEffect].} =
## Recursively walks over the directory `dir` and yields for each file
## or directory in `dir`.
##
Expand Down Expand Up @@ -2159,14 +2164,20 @@ iterator walkDirRec*(dir: string,
## * `walkDir iterator <#walkDir.i,string>`_

var stack = @[""]
var checkDir = checkDir
while stack.len > 0:
let d = stack.pop()
for k, p in walkDir(dir / d, relative = true):
for k, p in walkDir(dir / d, relative = true, checkDir = checkDir):
let rel = d / p
if k in {pcDir, pcLinkToDir} and k in followFilter:
stack.add rel
if k in yieldFilter:
yield if relative: rel else: dir / rel
checkDir = false
# We only check top-level dir, otherwise if a subdir is invalid (eg. wrong
# permissions), it'll abort iteration and there would be no way to
# continue iteration.
# Future work can provide a way to customize this and do error reporting.

proc rawRemoveDir(dir: string) {.noNimScript.} =
when defined(windows):
Expand All @@ -2181,13 +2192,13 @@ proc rawRemoveDir(dir: string) {.noNimScript.} =
else:
if rmdir(dir) != 0'i32 and errno != ENOENT: raiseOSError(osLastError(), dir)

proc removeDir*(dir: string) {.rtl, extern: "nos$1", tags: [
proc removeDir*(dir: string, checkDir = false) {.rtl, extern: "nos$1", tags: [
WriteDirEffect, ReadDirEffect], benign, noNimScript.} =
## Removes the directory `dir` including all subdirectories and files
## in `dir` (recursively).
##
## If this fails, `OSError` is raised. This does not fail if the directory never
## existed in the first place.
## existed in the first place, unless `checkDir` = true
##
## See also:
## * `tryRemoveFile proc <#tryRemoveFile,string>`_
Expand All @@ -2197,10 +2208,13 @@ proc removeDir*(dir: string) {.rtl, extern: "nos$1", tags: [
## * `copyDir proc <#copyDir,string,string>`_
## * `copyDirWithPermissions proc <#copyDirWithPermissions,string,string>`_
## * `moveDir proc <#moveDir,string,string>`_
for kind, path in walkDir(dir):
for kind, path in walkDir(dir, checkDir = checkDir):
case kind
of pcFile, pcLinkToFile, pcLinkToDir: removeFile(path)
of pcDir: removeDir(path)
of pcDir: removeDir(path, true)
# for subdirectories there is no benefit in `checkDir = false`
# (unless perhaps for edge case of concurrent processes also deleting
# the same files)
rawRemoveDir(dir)

proc rawCreateDir(dir: string): bool {.noNimScript.} =
Expand Down
6 changes: 3 additions & 3 deletions lib/system/nimscript.nim
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ proc listDirsImpl(dir: string): seq[string] {.
tags: [ReadIOEffect], raises: [OSError].} = builtin
proc listFilesImpl(dir: string): seq[string] {.
tags: [ReadIOEffect], raises: [OSError].} = builtin
proc removeDir(dir: string) {.
proc removeDir(dir: string, checkDir = true) {.
tags: [ReadIOEffect, WriteIOEffect], raises: [OSError].} = builtin
proc removeFile(dir: string) {.
tags: [ReadIOEffect, WriteIOEffect], raises: [OSError].} = builtin
Expand Down Expand Up @@ -204,10 +204,10 @@ proc listFiles*(dir: string): seq[string] =
result = listFilesImpl(dir)
checkOsError()

proc rmDir*(dir: string) {.raises: [OSError].} =
proc rmDir*(dir: string, checkDir = false) {.raises: [OSError].} =
## Removes the directory `dir`.
log "rmDir: " & dir:
removeDir dir
removeDir(dir, checkDir = checkDir)
checkOsError()

proc rmFile*(file: string) {.raises: [OSError].} =
Expand Down
11 changes: 5 additions & 6 deletions tests/newconfig/tfoo.nims
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ doAssert(existsEnv("dummy") == false)

# issue #7283
putEnv("dummy", "myval")
doAssert(existsEnv("dummy") == true)
doAssert(existsEnv("dummy"))
doAssert(getEnv("dummy") == "myval")
delEnv("dummy")
doAssert(existsEnv("dummy") == false)

# issue #7393
let wd = getCurrentDir()
Expand Down Expand Up @@ -64,6 +66,8 @@ else:
assert toDll("nim") == "libnim.so"

rmDir("tempXYZ")
doAssertRaises(OSError):
rmDir("tempXYZ", checkDir = true)
assert dirExists("tempXYZ") == false
mkDir("tempXYZ")
assert dirExists("tempXYZ") == true
Expand All @@ -81,8 +85,3 @@ when false:

rmDir("tempXYZ")
assert dirExists("tempXYZ") == false

putEnv("dummy", "myval")
doAssert(existsEnv("dummy") == true)
delEnv("dummy")
doAssert(existsEnv("dummy") == false)
20 changes: 13 additions & 7 deletions tests/stdlib/tos.nim
Original file line number Diff line number Diff line change
Expand Up @@ -163,13 +163,19 @@ block walkDirRec:

removeDir("walkdir_test")

when not defined(windows):
block walkDirRelative:
createDir("walkdir_test")
createSymlink(".", "walkdir_test/c")
for k, p in walkDir("walkdir_test", true):
doAssert k == pcLinkToDir
removeDir("walkdir_test")
block: # walkDir
doAssertRaises(OSError):
for a in walkDir("nonexistant", checkDir = true): discard
doAssertRaises(OSError):
for p in walkDirRec("nonexistant", checkDir = true): discard

when not defined(windows):
block walkDirRelative:
createDir("walkdir_test")
createSymlink(".", "walkdir_test/c")
for k, p in walkDir("walkdir_test", true):
doAssert k == pcLinkToDir
removeDir("walkdir_test")

block normalizedPath:
doAssert normalizedPath("") == ""
Expand Down