Nim: Can't tell return value of programs with staticExec

[def-]

Using staticExec the return value of the executed program can't be checked. Even the manual's example of git rev-parse HEAD can fail if you're not inside a git repo, which you can't catch nicely. I think staticExec should either return an empty string if the return value isn't 0, or have a way to get back the return value as well as stdout.

[hildjj]

Throw an exception if the return code is non-zero?

[Varriount]

It's not quite as simple as that (besides, exceptions should be used for unexpected, exceptional cases). Different platforms have different ideas on what a error return code is.

[refi64]

There are three ways I've seen:

1. Return a tuple.
2. Return a custom object. This is what Python does.
3. Use var parameters. This seems very ugly to me.

Personally, I think 2 would be the best, since platform-specific error codes might not matter; it could just have a succeeded boolean attribute.

[Araq]

_2_ is by far the hardest to implement and _1_ affects backwards compatibility. What should be done is that the proc grows an "onErrorRaise= true" flag and then an OSError exception is raised. If set to false the empty string is returned instead.

[dom96]

Wouldn't it make sense to force users to check the return code of the process? Therefore breaking backwards compatibility should be acceptable in this case.

[Araq]

@dom96 Returning a tuple is quite hard to do in the VM though.

[dom96]

@Araq Why is it hard?

[Araq]

Now at least it produces "" instead of crashing, I think.

[dom96]

That sounds like it will lead to some ambiguities.

[dom96]

Bah, so a staticExecEx was added. This just caused a bug in Nimble:

~ » nimble -v
nimble v0.8.8 compiled at 2017-09-07 20:23:12
git hash: fatal: Not a git repository (or any of the parent directories): .git

Silently accepting output when the command fails is as bad as JavaScript's undefined. I vote to make this a breaking change and throw an exception when the command fails.

[skellock]

I'm with @dom96 and php ceo on this one. If i can't staticExec, I want the build to burn to the ground.

However, if this ship has sailed, so be it. At least we have gorgeEx's exit code.

const goods = gorgeEx("exit 69")

echo goods.output # ""
echo goods.exitCode # 69

My question is this: what's a guy gotta do to get a t-shirt around here?

Should we be adding a staticExecEx alias, documentation and call it a day? Should we check the exitCode and raise?

I'm up for doing the work if you can tell me which direction you'd like.

[dom96]

I still want what I asked for initially. But best wait for @Araq to give his okay, of course if you've got a PR ready it'll be harder to say "no" ;)

[dom96]

IMO this is a breaking change that needs to happen before v1.0 so I'm adding this to the 1.0 milestone.

[Araq]

Seriously? Ok then, do what you think is best.

[skellock]

I had a look. With a handful of small changes to the gorge implementation we could die gloriously with a well placed localError().

However...

My concern is, doing this would mean we won't be returning our tuple (output, exitcode). This would be the type of breaking change where we remove capabilities entirely (e.g. what if someone had a gorgeEx call with another gorgeEx call as a fallback?).

Seems like we could "configure" our way out of this (e.g. yet another param to govern whether we localError or return the tuple), but I'm not really liking how my implementation is turning out.

I do believe the default should blow up, but do y'all have any advice to do that along with keeping exitcode/output on the table?

I'd love to see this feature through as I'm in way over my head and learning tonnes.

[genotrance]

Let the user decide if it needs to blow up. I'm using gorgeEx() in many .nimble files and looking at return values works just fine.

[timotheecour]

fixed in PR https://github.com/nim-lang/Nim/pull/10345

[narimiran]

The original issue ("Can't tell return value of programs with staticExec") has been solved by the addition of gorgeEx (link), which returns a tuple with, I'm quoting, "the precious exit code". :)

Since this is one of the issues marked with "v1.0 milestone", my question is: is the above enough and this can be closed, or is there some more (breaking) work that remains to be done (soon-ish)?

[Araq]

Use gorgeEx or look at gorge's return value. Not worth breaking somebody's code at this point.