bitwalker / distillery

Simplify deployments in Elixir with OTP releases!
MIT License
2.96k stars 398 forks source link

Add consistency to output handling in runtime code execution #583

Open rjanja opened 5 years ago

rjanja commented 5 years ago

Summary of changes

The node execution function eval/2 in Mix.Releases.Runtime.Control has been modified to be consistent with rpc/2 with regard to how it handles the result.

In rpc, the result is always printed to stdout. But in eval nothing is printed.

Current behavior

Explicit IO

$ ./bin/myapp rpc --mfa "IO.inspect/1" "foo"
"foo"
"foo"
$ ./bin/myapp eval --mfa "IO.inspect/1" "foo"
"foo"

Without explicit IO

$ ./bin/myapp rpc --mfa "Enum.join/1" --argv -- foo bar
"foobar"
$ ./bin/myapp eval --mfa "Enum.join/1" --argv -- foo bar
$

I expected "foobar" to be printed.

I suppose the inconsistency could instead be fixed by never printing the result, but that would make these commands less useful I think. If silent operation is desired, perhaps that could be added later as an optional flag.

Proposed behavior

Explicit IO

$ ./bin/myapp rpc --mfa "IO.inspect/1" "foo"
"foo"
"foo"
$ ./bin/myapp eval --mfa "IO.inspect/1" "foo"
"foo"
"foo"

Without explicit IO

$ ./bin/myapp rpc --mfa "Enum.join/1" --argv -- foo bar
"foobar"
$ ./bin/myapp eval --mfa "Enum.join/1" --argv -- foo bar
"foobar"

Checklist

Also:

Licensing/Copyright

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for Distillery (the "Contribution"). My Contribution is licensed under the MIT License.

rjanja commented 5 years ago

I've been able to get the tests running locally, and the four failures reported by AppVeyor are passing for me. Not sure about those other skipped tests ("cannot run clustered test cases if distribution is not active"). I've tried kicking the build off a few times and it's the same failures every time.

bitwalker commented 5 years ago

I think is probably reasonable - though I think my preference would probably be to always require explicit I/O for eval, but implicit for rpc. The reason is that they are generally used for different things. The reason eval requires explicit I/O is because you generally use it to build CLIs, or at least scripts to back custom comands; and in such cases you want control over the output. On the other hand, rpc is generally used to interrogate the remote node, so it is more convenient to have the result always be printed. If we really want to unify them, I would prefer to have rpc also do what eval did before, which is require explicit I/O to print the result. That said, I would like to know your thoughts on what I've just described, and whether your use cases differ, and if so, how; that may help me form a better mental model for what this should look like :)

rjanja commented 5 years ago

The distinction makes a little more sense now that I understand the reasoning behind it. I think I'd still favor consistent I/O handling, or a command line flag to enable (or disable, if enabled was the default) the printing of return value.

One point of confusion (for me) was that eval and rpc essentially do the same thing, just in different environments (local vs remote node). The names of these tasks being so different made it harder to grok.

As for my use case, I'm adding a couple of helpers to Bootleg for invoking these release tasks with arguments. It's a WIP but the syntax is currently:

Would it be overkill to have as Distillery tasks:

Where eval_* doesn't print but run_* does?

I'm happy to move this discussion to a new issue, and pare this PR back to just some minor documentation improvements. Let me know what you think. Thanks!