Implements #20611 - Adds findExeAll to os #21578
Conversation
lib/pure/os.nim
Outdated
| result = iterator() : string = | ||
| template checkCurrentDir() = | ||
| for ext in extensions: | ||
| var exe_with_ext = addFileExt(exe, ext) |
There was a problem hiding this comment.
Use Nim naming conventions, for stdlib at least.
lib/pure/os.nim
Outdated
| @@ -219,10 219,11 @@ const | |||
| ExeExts* = ## Platform specific file extension for executables. | |||
| ## On Windows ``["exe", "cmd", "bat"]``, on Posix ``[""]``. | |||
| when defined(windows): ["exe", "cmd", "bat"] else: [""] | |||
| ExeExtsSeq: seq[string] = toSeq(ExeExts) | |||
There was a problem hiding this comment.
Why this line?, openArray[string] can take array and seq.
lib/pure/os.nim
Outdated
| x = r | ||
| checkCurrentDir() | ||
| let path = getEnv("PATH") | ||
| for candidate in split(path, PathSep): |
There was a problem hiding this comment.
split should use maxsplit ?.
lib/pure/os.nim
Outdated
| var r = newString(maxSymlinkLen) | ||
| var len = readlink(x.cstring, r.cstring, maxSymlinkLen) | ||
| if len < 0: | ||
| raiseOSError(osLastError(), exe) |
lib/pure/os.nim
Outdated
| if len < 0: | ||
| raiseOSError(osLastError(), exe) | ||
| if len > maxSymlinkLen: | ||
| r = newString(len 1) |
There was a problem hiding this comment.
Use spaces around operators, at least for stdlib.
lib/pure/os.nim
Outdated
| when defined(windows): | ||
| var x = (if candidate[0] == '"' and candidate[^1] == '"': | ||
| substr(candidate, 1, candidate.len-2) else: candidate) / | ||
| exe |
There was a problem hiding this comment.
This way kinda copies the string too much ?.
lib/pure/os.nim
Outdated
| if fileExists(exe_with_ext): | ||
| yield exe_with_ext | ||
| when defined(posix): | ||
| if '/' in exe: checkCurrentDir() |
There was a problem hiding this comment.
So exe="/foo.bin" checks current directory ?.
lib/pure/os.nim
Outdated
| ## meets the actual file. This behavior can be disabled if desired | ||
| ## by setting `followSymlinks = false`. | ||
|
|
||
| var results : seq[string] |
There was a problem hiding this comment.
Use result instead.
lib/pure/os.nim
Outdated
| let exes = iterExes(exe, followSymlinks, extensions) | ||
| for exe in exes(): | ||
| results.add(exe) | ||
| return results |
There was a problem hiding this comment.
No need to return if you just assign to result.
| ## | ||
| ## If the system supports symlinks it also resolves them until it | ||
| ## meets the actual file. This behavior can be disabled if desired | ||
| ## by setting `followSymlinks = false`. |
There was a problem hiding this comment.
This needs some kind of max limit integer Positive for safety.
I can just touch and chmod x a ton of files on a folder,
and the function will hang the system for years...
lib/pure/os.nim
Outdated
| var x = addFileExt(x, ext) | ||
| if fileExists(x): | ||
| when not (defined(windows) or defined(nintendoswitch)): | ||
| while followSymlinks: # doubles as if here |
There was a problem hiding this comment.
" while doubles as if " ? 🤔
Why is that needed anyways.
| if isAbsolute(r): | ||
| x = r | ||
| checkCurrentDir() | ||
| let path = getEnv("PATH") |
There was a problem hiding this comment.
path can be empty string.
| ## listed in the PATH environment variable. Returns "" if the exe cannot be | ||
| ## found. | ||
| builtin | ||
|
|
There was a problem hiding this comment.
Have you actually confirmed all this works on Nimscript ?.
lib/pure/os.nim
Outdated
| break | ||
| yield x | ||
|
|
||
| proc findExe*(exe: string, followSymlinks: bool=true, maxChecks=1000, |
There was a problem hiding this comment.
maxChecks = 1000.Positive IMHO.
(Positive is not perfect for advanced users, but still better than just plain int because it wont allow negative values)
No description provided.