-
Notifications
You must be signed in to change notification settings - Fork 12
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
Fix rebuild logic binary checks for windows #24
Comments
turns out those dont work on windows, it just raises the exception and goes along, can be fixed with |
ok so I had this mostly working then I discovered why findExe("nake") wasnt working, in my nimble/bin the exes don't have exe in them, just |
note nake.exe time check will not work on windows because there is no .exe file in .nimble/bin refs #24
Instead of using The |
|
Maybe a good idea to avoid painting ourselves into a corner would be to leave the interface open through a default parameter or a global variable: import os
when defined(windows):
const defaultBinaryExtensions = ["exe", "com", "bat"]
else:
const defaultBinaryExtensions = ["", "sh", "bin"]
proc findBinary(filename: string,
extensions: varargs[string] = defaultBinaryExtensions) =
let dummyPaths = ["dir1", "dir2", "dir3"]
for path in dummyPaths:
for extension in extensions:
echo "Testing ", path/filename.addFileExt(extension)
when isMainModule:
findBinary("compiler")
findBinary("compiler", ["cmd", "shell"]) The global variable version would have the advantage of affecting all |
I'd prefer something like this, only difference is slightly less string ops (we can delay adding the path until the file is found) proc findFile(filename: string,
extensions: openarray[string] = defaultBinaryExtensions,
paths: varargs[string]): string =
result = ""
if paths.len < 1: return
let curdir = getCurrentDir()
block search:
for path in paths:
setCurrentDir(path)
for extension in extensions:
let f = filename.addFileExt(extension)
if fileExists(f):
result = path / f
break search
setCurrentDir(curdir) |
I imagine changing the current environment directory would be more expensive than a few string concatenations, but then it's not like performance is critical, it is already going to be slow with the calls to |
Newer Nim's |
Looking at the logic of #23 the code performs the nakefile check like:
This doesn't include the
exe
extension for the windows binary, so it will likely fail. Or did windows drop file extensions for binaries?The text was updated successfully, but these errors were encountered: