From dfda3271cd7e737c9507f83ab3fda75afdc32498 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Thu, 20 Feb 2020 15:01:00 -0800 Subject: [PATCH 1/9] * relativePath(foo) now works * relativePath(rel, abs) and relativePath(abs, rel) now work (fixes #13222) * relativePath, absolutePath, getCurrentDir now available in more targets (eg: vm, nodejs etc) --- compiler/vmops.nim | 4 ++- lib/pure/os.nim | 65 ++++++++++++++++++++++++++++++++-------------- 2 files changed, 48 insertions(+), 21 deletions(-) diff --git a/compiler/vmops.nim b/compiler/vmops.nim index ef36419a70f6e..a89926d1763a0 100644 --- a/compiler/vmops.nim +++ b/compiler/vmops.nim @@ -13,7 +13,7 @@ from math import sqrt, ln, log10, log2, exp, round, arccos, arcsin, arctan, arctan2, cos, cosh, hypot, sinh, sin, tan, tanh, pow, trunc, floor, ceil, `mod` -from os import getEnv, existsEnv, dirExists, fileExists, putEnv, walkDir, getAppFilename +from os import getEnv, existsEnv, dirExists, fileExists, putEnv, walkDir, getAppFilename, getCurrentDir from md5 import getMD5 from sighashes import symBodyDigest from times import cpuTime @@ -198,6 +198,8 @@ proc registerAdditionalOps*(c: PCtx) = registerCallback c, "stdlib.os.getCurrentCompilerExe", proc (a: VmArgs) {.nimcall.} = setResult(a, getAppFilename()) + registerCallback c, "stdlib.os.getCurrentDir", proc (a: VmArgs) {.nimcall.} = + setResult(a, getCurrentDir()) registerCallback c, "stdlib.macros.symBodyHash", proc (a: VmArgs) {.nimcall.} = let n = getNode(a, 0) diff --git a/lib/pure/os.nim b/lib/pure/os.nim index e1e5f31b4b241..3936b40317f5c 100644 --- a/lib/pure/os.nim +++ b/lib/pure/os.nim @@ -74,6 +74,9 @@ elif defined(posix): else: {.error: "OS module not ported to your operating system!".} +template noNimScriptIgnore(body: untyped) {.dirty.} = + when not defined(nimscript): body + when weirdTarget and defined(nimErrorProcCanHaveBody): {.pragma: noNimScript, error: "this proc is not available on the NimScript target".} else: @@ -98,6 +101,8 @@ type include "includes/osseps" +proc absolutePathInternal(path: string): string + proc normalizePathEnd(path: var string, trailingSep = false) = ## Ensures ``path`` has exactly 0 or 1 trailing `DirSep`, depending on ## ``trailingSep``, and taking care of edge cases: it preservers whether @@ -360,8 +365,8 @@ when doslikeFileSystem: else: return false -proc relativePath*(path, base: string; sep = DirSep): string {. - noSideEffect, rtl, extern: "nos$1", raises: [].} = +proc relativePath*(path, base: string, sep = DirSep): string {. + rtl, extern: "nos$1".} = ## Converts `path` to a path relative to `base`. ## ## The `sep` (default: `DirSep <#DirSep>`_) is used for the path normalizations, @@ -390,6 +395,12 @@ proc relativePath*(path, base: string; sep = DirSep): string {. var path = path path.normalizePathAux base.normalizePathAux + let a1 = isAbsolute(path) + let a2 = isAbsolute(base) + if a1 and not a2: + base = absolutePathInternal(base) + elif a2 and not a1: + path = absolutePathInternal(path) when doslikeFileSystem: if isAbsolute(path) and isAbsolute(base): @@ -1293,7 +1304,7 @@ proc fileNewer*(a, b: string): bool {.rtl, extern: "nos$1", noNimScript.} = else: result = getLastModificationTime(a) > getLastModificationTime(b) -proc getCurrentDir*(): string {.rtl, extern: "nos$1", tags: [], noNimScript.} = +proc getCurrentDir*(): string {.rtl, extern: "nos$1", tags: [], noNimScriptIgnore.} = ## Returns the `current working directory`:idx: i.e. where the built ## binary is run. ## @@ -1306,7 +1317,13 @@ proc getCurrentDir*(): string {.rtl, extern: "nos$1", tags: [], noNimScript.} = ## * `setCurrentDir proc <#setCurrentDir,string>`_ ## * `currentSourcePath template `_ ## * `getProjectPath proc `_ - when defined(windows): + when defined(nodejs): + var ret: cstring + {.emit: "`ret` = process.cwd();".} + return $ret + elif defined(js): + doAssert false, "use -d:nodejs to have `getCurrentDir` defined" + elif defined(windows): var bufsize = MAX_PATH.int32 when useWinUnicode: var res = newWideCString("", bufsize) @@ -1348,6 +1365,11 @@ proc getCurrentDir*(): string {.rtl, extern: "nos$1", tags: [], noNimScript.} = else: raiseOSError(osLastError()) +proc relativePath*(path: string, sep = DirSep): string = + ## returns `path` relative to current directory + let base = if path.isAbsolute: getCurrentDir() else: "." + relativePath(path, base, sep) + proc setCurrentDir*(newDir: string) {.inline, tags: [], noNimScript.} = ## Sets the `current working directory`:idx:; `OSError` ## is raised if `newDir` cannot been set. @@ -1366,23 +1388,26 @@ proc setCurrentDir*(newDir: string) {.inline, tags: [], noNimScript.} = else: if chdir(newDir) != 0'i32: raiseOSError(osLastError(), newDir) -when not weirdTarget: - proc absolutePath*(path: string, root = getCurrentDir()): string {.noNimScript.} = - ## Returns the absolute path of `path`, rooted at `root` (which must be absolute; - ## default: current directory). - ## If `path` is absolute, return it, ignoring `root`. - ## - ## See also: - ## * `normalizedPath proc <#normalizedPath,string>`_ - ## * `normalizePath proc <#normalizePath,string>`_ - runnableExamples: - assert absolutePath("a") == getCurrentDir() / "a" - if isAbsolute(path): path - else: - if not root.isAbsolute: - raise newException(ValueError, "The specified root is not absolute: " & root) - joinPath(root, path) +proc absolutePath*(path: string, root = getCurrentDir()): string = + ## Returns the absolute path of `path`, rooted at `root` (which must be absolute; + ## default: current directory). + ## If `path` is absolute, return it, ignoring `root`. + ## + ## See also: + ## * `normalizedPath proc <#normalizedPath,string>`_ + ## * `normalizePath proc <#normalizePath,string>`_ + runnableExamples: + assert absolutePath("a") == getCurrentDir() / "a" + + if isAbsolute(path): path + else: + if not root.isAbsolute: + raise newException(ValueError, "The specified root is not absolute: " & root) + joinPath(root, path) + +proc absolutePathInternal(path: string): string = + absolutePath(path, getCurrentDir()) proc normalizePath*(path: var string) {.rtl, extern: "nos$1", tags: [].} = ## Normalize a path. From 25a2000498c6b177d144ab79b6b419400948d623 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Thu, 20 Feb 2020 18:26:27 -0800 Subject: [PATCH 2/9] add tests --- tests/stdlib/tos.nim | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/stdlib/tos.nim b/tests/stdlib/tos.nim index be538401586a1..ca8e64fd1a558 100644 --- a/tests/stdlib/tos.nim +++ b/tests/stdlib/tos.nim @@ -348,6 +348,10 @@ block ospaths: doAssert relativePath("", "foo") == "" doAssert relativePath("././/foo", "foo//./") == "." + doAssert relativePath(getCurrentDir() / "bar") == "bar" + doAssert relativePath(getCurrentDir() / "bar", "foo") == "../bar".unixToNativePath + doAssert relativePath("bar", getCurrentDir() / "foo") == "../bar".unixToNativePath + when doslikeFileSystem: doAssert relativePath(r"c:\foo.nim", r"C:\") == r"foo.nim" doAssert relativePath(r"c:\foo\bar\baz.nim", r"c:\foo") == r"bar\baz.nim" From d1c3577cb45f7b0a5b2a608a2f2ea8d7688b1b53 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Thu, 20 Feb 2020 18:26:38 -0800 Subject: [PATCH 3/9] fix bootstrap --- lib/pure/os.nim | 114 ++++++++++++++++++++++++------------------------ 1 file changed, 56 insertions(+), 58 deletions(-) diff --git a/lib/pure/os.nim b/lib/pure/os.nim index 3936b40317f5c..d210d28a0bd42 100644 --- a/lib/pure/os.nim +++ b/lib/pure/os.nim @@ -74,9 +74,6 @@ elif defined(posix): else: {.error: "OS module not ported to your operating system!".} -template noNimScriptIgnore(body: untyped) {.dirty.} = - when not defined(nimscript): body - when weirdTarget and defined(nimErrorProcCanHaveBody): {.pragma: noNimScript, error: "this proc is not available on the NimScript target".} else: @@ -1304,66 +1301,67 @@ proc fileNewer*(a, b: string): bool {.rtl, extern: "nos$1", noNimScript.} = else: result = getLastModificationTime(a) > getLastModificationTime(b) -proc getCurrentDir*(): string {.rtl, extern: "nos$1", tags: [], noNimScriptIgnore.} = - ## Returns the `current working directory`:idx: i.e. where the built - ## binary is run. - ## - ## So the path returned by this proc is determined at run time. - ## - ## See also: - ## * `getHomeDir proc <#getHomeDir>`_ - ## * `getConfigDir proc <#getConfigDir>`_ - ## * `getTempDir proc <#getTempDir>`_ - ## * `setCurrentDir proc <#setCurrentDir,string>`_ - ## * `currentSourcePath template `_ - ## * `getProjectPath proc `_ - when defined(nodejs): - var ret: cstring - {.emit: "`ret` = process.cwd();".} - return $ret - elif defined(js): - doAssert false, "use -d:nodejs to have `getCurrentDir` defined" - elif defined(windows): - var bufsize = MAX_PATH.int32 - when useWinUnicode: - var res = newWideCString("", bufsize) - while true: - var L = getCurrentDirectoryW(bufsize, res) - if L == 0'i32: - raiseOSError(osLastError()) - elif L > bufsize: - res = newWideCString("", L) - bufsize = L - else: - result = res$L - break +when not defined(nimscript): + proc getCurrentDir*(): string {.rtl, extern: "nos$1", tags: [].} = + ## Returns the `current working directory`:idx: i.e. where the built + ## binary is run. + ## + ## So the path returned by this proc is determined at run time. + ## + ## See also: + ## * `getHomeDir proc <#getHomeDir>`_ + ## * `getConfigDir proc <#getConfigDir>`_ + ## * `getTempDir proc <#getTempDir>`_ + ## * `setCurrentDir proc <#setCurrentDir,string>`_ + ## * `currentSourcePath template `_ + ## * `getProjectPath proc `_ + when defined(nodejs): + var ret: cstring + {.emit: "`ret` = process.cwd();".} + return $ret + elif defined(js): + doAssert false, "use -d:nodejs to have `getCurrentDir` defined" + elif defined(windows): + var bufsize = MAX_PATH.int32 + when useWinUnicode: + var res = newWideCString("", bufsize) + while true: + var L = getCurrentDirectoryW(bufsize, res) + if L == 0'i32: + raiseOSError(osLastError()) + elif L > bufsize: + res = newWideCString("", L) + bufsize = L + else: + result = res$L + break + else: + result = newString(bufsize) + while true: + var L = getCurrentDirectoryA(bufsize, result) + if L == 0'i32: + raiseOSError(osLastError()) + elif L > bufsize: + result = newString(L) + bufsize = L + else: + setLen(result, L) + break else: + var bufsize = 1024 # should be enough result = newString(bufsize) while true: - var L = getCurrentDirectoryA(bufsize, result) - if L == 0'i32: - raiseOSError(osLastError()) - elif L > bufsize: - result = newString(L) - bufsize = L - else: - setLen(result, L) + if getcwd(result, bufsize) != nil: + setLen(result, c_strlen(result)) break - else: - var bufsize = 1024 # should be enough - result = newString(bufsize) - while true: - if getcwd(result, bufsize) != nil: - setLen(result, c_strlen(result)) - break - else: - let err = osLastError() - if err.int32 == ERANGE: - bufsize = bufsize shl 1 - doAssert(bufsize >= 0) - result = newString(bufsize) else: - raiseOSError(osLastError()) + let err = osLastError() + if err.int32 == ERANGE: + bufsize = bufsize shl 1 + doAssert(bufsize >= 0) + result = newString(bufsize) + else: + raiseOSError(osLastError()) proc relativePath*(path: string, sep = DirSep): string = ## returns `path` relative to current directory From 9d44f6da1a7e74c575bd2a8671e93df83889d8cf Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Thu, 20 Feb 2020 19:16:25 -0800 Subject: [PATCH 4/9] fixup --- lib/pure/os.nim | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pure/os.nim b/lib/pure/os.nim index d210d28a0bd42..39a9a79e8ef13 100644 --- a/lib/pure/os.nim +++ b/lib/pure/os.nim @@ -98,7 +98,7 @@ type include "includes/osseps" -proc absolutePathInternal(path: string): string +proc absolutePathInternal(path: string): string {.gcsafe.} proc normalizePathEnd(path: var string, trailingSep = false) = ## Ensures ``path`` has exactly 0 or 1 trailing `DirSep`, depending on From 17eb541d40685cefbea5fc5d085f6d4cbc875828 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Fri, 21 Feb 2020 00:45:29 -0800 Subject: [PATCH 5/9] remove VM getCurrentDir --- compiler/vmops.nim | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/compiler/vmops.nim b/compiler/vmops.nim index a89926d1763a0..ef36419a70f6e 100644 --- a/compiler/vmops.nim +++ b/compiler/vmops.nim @@ -13,7 +13,7 @@ from math import sqrt, ln, log10, log2, exp, round, arccos, arcsin, arctan, arctan2, cos, cosh, hypot, sinh, sin, tan, tanh, pow, trunc, floor, ceil, `mod` -from os import getEnv, existsEnv, dirExists, fileExists, putEnv, walkDir, getAppFilename, getCurrentDir +from os import getEnv, existsEnv, dirExists, fileExists, putEnv, walkDir, getAppFilename from md5 import getMD5 from sighashes import symBodyDigest from times import cpuTime @@ -198,8 +198,6 @@ proc registerAdditionalOps*(c: PCtx) = registerCallback c, "stdlib.os.getCurrentCompilerExe", proc (a: VmArgs) {.nimcall.} = setResult(a, getAppFilename()) - registerCallback c, "stdlib.os.getCurrentDir", proc (a: VmArgs) {.nimcall.} = - setResult(a, getCurrentDir()) registerCallback c, "stdlib.macros.symBodyHash", proc (a: VmArgs) {.nimcall.} = let n = getNode(a, 0) From 9a9028b56062a3ba2cffae8558ccd2a07464f995 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Sat, 22 Feb 2020 01:02:36 -0800 Subject: [PATCH 6/9] fix bug: isAbsolutePath now works with -d:js; add tests --- lib/pure/os.nim | 4 +++- tests/js/tos.nim | 8 ++++++++ tests/stdlib/tos.nim | 1 + 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/pure/os.nim b/lib/pure/os.nim index 39a9a79e8ef13..9a682760686ae 100644 --- a/lib/pure/os.nim +++ b/lib/pure/os.nim @@ -299,8 +299,10 @@ proc isAbsolute*(path: string): bool {.rtl, noSideEffect, extern: "nos$1", raise result = path[0] != ':' elif defined(RISCOS): result = path[0] == '$' - elif defined(posix): + elif defined(posix) or defined(js): result = path[0] == '/' + else: + doAssert false # if ever hits here, adapt as needed when FileSystemCaseSensitive: template `!=?`(a, b: char): bool = a != b diff --git a/tests/js/tos.nim b/tests/js/tos.nim index 7395a0ad7a99d..2530209a1891a 100644 --- a/tests/js/tos.nim +++ b/tests/js/tos.nim @@ -5,3 +5,11 @@ import os block: doAssert "./foo//./bar/".normalizedPath == "foo/bar" doAssert relativePath(".//foo/bar", "foo") == "bar" + doAssert "/".isAbsolute + doAssert not "".isAbsolute + doAssert not ".".isAbsolute + doAssert not "foo".isAbsolute + doAssert relativePath(getCurrentDir()) == "." + doAssert relativePath(getCurrentDir() / "foo", "bar") == "../foo" + doAssert relativePath("", "bar") == "" + doAssert normalizedPath(".///foo//./") == "foo" diff --git a/tests/stdlib/tos.nim b/tests/stdlib/tos.nim index ca8e64fd1a558..edad80621395e 100644 --- a/tests/stdlib/tos.nim +++ b/tests/stdlib/tos.nim @@ -348,6 +348,7 @@ block ospaths: doAssert relativePath("", "foo") == "" doAssert relativePath("././/foo", "foo//./") == "." + doAssert relativePath(getCurrentDir()) == "." doAssert relativePath(getCurrentDir() / "bar") == "bar" doAssert relativePath(getCurrentDir() / "bar", "foo") == "../bar".unixToNativePath doAssert relativePath("bar", getCurrentDir() / "foo") == "../bar".unixToNativePath From 7b4956e46cd0c5dcc2c954993218099cd5c9af9d Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Sat, 22 Feb 2020 04:53:17 -0800 Subject: [PATCH 7/9] workaround https://github.com/nim-lang/Nim/issues/13469 --- lib/pure/os.nim | 2 ++ tests/js/tos.nim | 11 +++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/pure/os.nim b/lib/pure/os.nim index 9a682760686ae..bf1b743edbc3c 100644 --- a/lib/pure/os.nim +++ b/lib/pure/os.nim @@ -300,6 +300,8 @@ proc isAbsolute*(path: string): bool {.rtl, noSideEffect, extern: "nos$1", raise elif defined(RISCOS): result = path[0] == '$' elif defined(posix) or defined(js): + # `or defined(js)` wouldn't be needed pending https://github.com/nim-lang/Nim/issues/13469 + # This works around the problem for posix, but windows is still broken with nim js -d:nodejs result = path[0] == '/' else: doAssert false # if ever hits here, adapt as needed diff --git a/tests/js/tos.nim b/tests/js/tos.nim index 2530209a1891a..8b3f622372aa5 100644 --- a/tests/js/tos.nim +++ b/tests/js/tos.nim @@ -9,7 +9,14 @@ block: doAssert not "".isAbsolute doAssert not ".".isAbsolute doAssert not "foo".isAbsolute - doAssert relativePath(getCurrentDir()) == "." - doAssert relativePath(getCurrentDir() / "foo", "bar") == "../foo" doAssert relativePath("", "bar") == "" doAssert normalizedPath(".///foo//./") == "foo" + let cwd = getCurrentDir() + + let isWindows = '\\' in cwd + # defined(windows) doesn't work with -d:nodejs but should + # these actually break because of that (see https://github.com/nim-lang/Nim/issues/13469) + if not isWindows: + doAssert cwd.isAbsolute + doAssert relativePath(getCurrentDir() / "foo", "bar") == "../foo" + doAssert relativePath(getCurrentDir()) == "." From e5ded7e64502e94cc8903e12e66ff1c74653b302 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Tue, 21 Apr 2020 12:03:57 -0700 Subject: [PATCH 8/9] remove `relativePath(path)` overload for now --- lib/pure/os.nim | 5 ----- tests/js/tos.nim | 1 - tests/stdlib/tos.nim | 2 -- 3 files changed, 8 deletions(-) diff --git a/lib/pure/os.nim b/lib/pure/os.nim index bf1b743edbc3c..2c3e91be89430 100644 --- a/lib/pure/os.nim +++ b/lib/pure/os.nim @@ -1367,11 +1367,6 @@ when not defined(nimscript): else: raiseOSError(osLastError()) -proc relativePath*(path: string, sep = DirSep): string = - ## returns `path` relative to current directory - let base = if path.isAbsolute: getCurrentDir() else: "." - relativePath(path, base, sep) - proc setCurrentDir*(newDir: string) {.inline, tags: [], noNimScript.} = ## Sets the `current working directory`:idx:; `OSError` ## is raised if `newDir` cannot been set. diff --git a/tests/js/tos.nim b/tests/js/tos.nim index 8b3f622372aa5..30c19c1aef3db 100644 --- a/tests/js/tos.nim +++ b/tests/js/tos.nim @@ -19,4 +19,3 @@ block: if not isWindows: doAssert cwd.isAbsolute doAssert relativePath(getCurrentDir() / "foo", "bar") == "../foo" - doAssert relativePath(getCurrentDir()) == "." diff --git a/tests/stdlib/tos.nim b/tests/stdlib/tos.nim index edad80621395e..20e82173d7217 100644 --- a/tests/stdlib/tos.nim +++ b/tests/stdlib/tos.nim @@ -348,8 +348,6 @@ block ospaths: doAssert relativePath("", "foo") == "" doAssert relativePath("././/foo", "foo//./") == "." - doAssert relativePath(getCurrentDir()) == "." - doAssert relativePath(getCurrentDir() / "bar") == "bar" doAssert relativePath(getCurrentDir() / "bar", "foo") == "../bar".unixToNativePath doAssert relativePath("bar", getCurrentDir() / "foo") == "../bar".unixToNativePath From 569d8b7e609bd9c85caa10656b8248db66097d38 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Tue, 21 Apr 2020 12:19:07 -0700 Subject: [PATCH 9/9] add back changelog after rebase --- changelog.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/changelog.md b/changelog.md index c30abab9f7eec..4a603e91c95b4 100644 --- a/changelog.md +++ b/changelog.md @@ -30,6 +30,12 @@ - The file descriptors created for internal bookkeeping by `ioselector_kqueue` and `ioselector_epoll` will no longer be leaked to child processes. +- `relativePath(rel, abs)` and `relativePath(abs, rel)` used to silently give wrong results + (see #13222); instead they now use `getCurrentDir` to resolve those cases, + and this can now throw in edge cases where `getCurrentDir` throws. + `relativePath` also now works for js with `-d:nodejs`. + + ## Language changes - In newruntime it is now allowed to assign discriminator field without restrictions as long as case object doesn't have custom destructor. Discriminator value doesn't have to be a constant either. If you have custom destructor for case object and you do want to freely assign discriminator fields, it is recommended to refactor object into 2 objects like this: ```nim