beef331 / nimscripter

Quick and easy Nim <-> Nimscript interop
MIT License
145 stars 8 forks source link

`exec` in Nimscript does not work #4

Closed TheSimpleZ closed 2 years ago

TheSimpleZ commented 2 years ago

I don't know if this is related to this lib or if it's a limitation in the nim VM. However, there are several procs that don't work as expected inside nimscripts.

import nimscripter

const script = """
proc build*(): bool =
  echo "building nim... "
  exec "sleep 10"
  echo getCurrentDir()
  echo "done"
  true

when isMainModule:
  discard build()
"""

addCallable(buildpackModule):
  proc build(): bool
const addins = implNimscriptModule(buildpackModule)
discard loadScript(NimScriptFile(script), addins, modules = ["system/nimscript"])

When I run the contents of script as a file with nim e the script outputs "building...", waits for 10 seconds, then outputs the current directory. When I run my main.nim file, I would expect it to do the same. But the script outputs "building...", then immediately prints an empty line before exiting without failures.

It seems like the exec command is being ignored. It also looks like the getCurrentDir() proc just returns an empty string.

I also tried running echo 'hello' using gorgeEx inside the nimscript. It returned no output and exitcode 0.

Any idea if it's possible to do what I'm trying to do?

beef331 commented 2 years ago

Edit: looking at it further it's odd that it does not work since it does pass the "implement stdlib flags in the interpreter", so it's an issue with Nimscripter and I'll look into it

beef331 commented 2 years ago

Looking into how the compiler implements those operations I have decided to just make a template to implement them and avoid the nimscript module. You can see the file that adds them https://github.com/beef331/nimscripter/blob/master/src/nimscripter/vmops.nim here. An example of them in use is here. https://github.com/beef331/nimscripter/blob/master/tests/tvmops.nim

TheSimpleZ commented 2 years ago

Thanks for the quick fix, however, I still have some concerns.

According to https://nim-lang.org/docs/nimscript.html#exec%2Cstring

If the external process terminates with a non-zero exit code, an OSError exception is raised.

However, this:

import nimscripter
import nimscripter/vmops

const script = """
proc build*(): bool =
  echo "building nim... "
  exec "exit 1"
  echo "done"
  true
when isMainModule:
  discard build()
"""
addVmops(buildpackModule)
addCallable(buildpackModule):
  proc build(): bool
const addins = implNimscriptModule(buildpackModule)
discard loadScript(NimScriptFile(script), addins)

Outputs this:

building nim... 
done

When I run the contents of the scripts variable with nim e, I get this:

building nim... 
stack trace: (most recent call last)
~/Documents/monorepo-manager/pkgs/mono/src/test.nims(7, 16)
~/Documents/monorepo-manager/pkgs/mono/src/test.nims(3, 8) build
~/.choosenim/toolchains/nim-1.6.2/lib/system/nimscript.nim(273, 7) exec
~/.choosenim/toolchains/nim-1.6.2/lib/system/nimscript.nim(273, 7) Error: unhandled exception: FAILED: exit 1 [OSError]

as expected.

Also, gorgeEx does not work. This:

import nimscripter
import nimscripter/vmops

const script = """
proc build*(): bool =
  echo "building nim... "
  echo gorgeEx "exit 1"
  echo "done"
  true
when isMainModule:
  discard build()
"""
addVmops(buildpackModule)
addCallable(buildpackModule):
  proc build(): bool
const addins = implNimscriptModule(buildpackModule)
discard loadScript(NimScriptFile(script), addins)

outputs this:

building nim... 
(output: "", exitCode: 0)
done

While I would expect the exit code to be 1.

Is it really necessary to re-implement and expose all the system modules I'd like to use in the scripts or is there a way to simply link it to the existing implementation in the nimscript and system modules?

beef331 commented 2 years ago

To implement all the Nimscript procedures https://github.com/nim-lang/Nim/blob/devel/compiler/scriptconfig.nim#L29 needs to be invoked, before our script is loaded, which will require a PR to the createInterpreter procedure the compiler API exposes. Given that exec runs inside the compiled binary I did not see that having it crash the entire program as a sensible solution. An exception raised in native code cannot be managed from Nimscript .

TheSimpleZ commented 2 years ago

I'd like to argue that it's better to follow the documented behavior since that's what people would expect. If the developer is loading unknown scripts that might fail they should use a try/catch to prevent their program from crashing.

At the very least, the nimscript should not continue running after the exec returns with an error. Since that should lead to potentially unwanted and unexpected behaviors.

In my case, I'm trying to build a task runner based on nimscripts. I'm using this library to load configuration values directly from the scripts. I'm also running procs as tasks. If someone was to use an exec statement to do a crucial command, they would expect their script to stop execution if the command fails.

I'm submitted a PR with some changes and other features. Feedback is welcome 😄

beef331 commented 2 years ago

try/catch to prevent their program from crashing

That's the issue you cannot wrap the native code in a try catch from Nimscript since the error occurs inside the Nim code

import nimscripter

proc doThing(): int = raise newException(ValueError, "hello")
exportTo(myImpl, doThing)
const
  scriptProcs = implNimScriptModule(myImpl)
  ourScript = NimScriptFile"""
try: 
  discard doThing()
except CatchableError as e: 
  echo e.msg"""
let intr = loadScript(ourScript, scriptProcs)

Will crash the binary due to an unhandled exception.

TheSimpleZ commented 2 years ago

No, i mean that the main program has to use a try/catch when loading the script. I do something like this in my application

import nimscripter

proc doThing(): int = raise newException(ValueError, "hello")
exportTo(myImpl, doThing)
const
  scriptProcs = implNimScriptModule(myImpl)
  ourScript = NimScriptFile"""
  discard doThing()
"""
try:
  let intr = loadScript(ourScript, scriptProcs)
except:
  echo "an error happened when loading script"

That way, the script will not continue execution when exec errors, and the exception can be caught in the main application.

beef331 commented 2 years ago

Ah I see, perhaps it's fine.