nake icon indicating copy to clipboard operation
nake copied to clipboard

Fix rebuild logic binary checks for windows

Open gradha opened this issue 9 years ago • 8 comments

Looking at the logic of #23 the code performs the nakefile check like:

binaryTime = toSeconds(getLastModificationTime("nakefile"))

This doesn't include the exe extension for the windows binary, so it will likely fail. Or did windows drop file extensions for binaries?

gradha avatar Mar 19 '15 17:03 gradha

turns out those dont work on windows, it just raises the exception and goes along, can be fixed with findExe("nakefile"), we can expect there to not be another nakefile.exe on PATH, right?

fowlmouth avatar Mar 21 '15 02:03 fowlmouth

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 .cmd launchers

fowlmouth avatar Mar 21 '15 02:03 fowlmouth

Instead of using findExe I was more thinking of something like getFilePermissions("."/addFileExt("nakefile", ExeExt)) to avoid looking at the path and be more in line with the last shell call which also prefixes the current directory to avoid troubles.

The .cmd extension you mention looks problematic, should then nim's stdlib findExe be upgraded to look for such extension on windows? nake could of course provide it's own implementation, but if this is the usual way of installing binaries on windows with nimble, other programs depending on calling findExe for nimble binaries would fail too?

gradha avatar Mar 21 '15 09:03 gradha

os.findExe doesnt look like it would be easy to fix, it relies on a const that says what an "exe" is for a platform and it only considers there to be one exe suffix for any platform. I'll write a specialized version for nake

fowlmouth avatar Mar 26 '15 07:03 fowlmouth

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 findBinary calls, even those not in reach by end user code, so maybe its better? Your call.

gradha avatar Mar 26 '15 21:03 gradha

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)

fowlmouth avatar Mar 26 '15 23:03 fowlmouth

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 fileExists.

gradha avatar Mar 27 '15 16:03 gradha

Newer Nim's findExe also considers .cmd and .bat extensions out of the box.

Araq avatar Apr 05 '17 08:04 Araq