Send output of 'print' statements in notionflux scripts to the invoking shell

[wilhelmy]

This patchset implements a print() function for use in mod_notionflux.

This can theoretically be merged, but the question is whether instead of collecting print() output and displaying it at the very end (and losing it in case of an error), passing a file descriptor (e.g. stdout) from notionflux to mod_notionflux over the unix socket is a better solution.

[wilhelmy]

Maybe it's even better to pass stderr, leaving stdout for replies which are formatted as string.format('%q') and can therefore be piped back into lua... the question is, I guess, what kind of tool we want notionflux to be. For me it's an interactive debugging tool first and foremost.

[raboof]

For me it's both an interactive exploration/debugging tool (and a much better one since your recent changes!), but also a building block for scripts and utilities to call into Notion from 'outside' as needed.

[wilhelmy]

Do you have any suggestion what we should do about print, then? :)
I think I'd prefer passing a file descriptor (also because we avoid running into MAX_DATA that way) but maybe that's "ooh cool unix domain sockets can pass file descriptors, new toy!!" clouding my judgement.

[wilhelmy]

The new toy has a pretty awful API...
Edit: It is a very useful toy, however. Now I have some code to copypaste :) Please review.

[wilhelmy]

I think I might have screwed up with extl_unref_fn'ing the tostringstr function, because I moved the lookup for the function around.

[wilhelmy]

Another thing I noticed: notionflux -e 'print "foo"' >/dev/full currently results in SIGPIPE. I know how to fix it.

[raboof]

This does not seem very elegant: this means now suddenly all print calls will be redirected to this new notionflux connection, also print calls that don't have anything to do with this notionflux invocation (either from notion itself or from another notionflux invocation).

I suspect the pcall below will be executed on one thread, and that one thread will not be used for any other processing until pcall returns. If that is indeed correct, we could use an approach like the one described at https://stackoverflow.com/questions/24356520/thread-locals-in-lua to store a 'print target' per thread, and only forward print calls from this thread to this notionflux instance. WDYT?

[wilhelmy]

It isn't very elegant, indeed.

Are there multiple threads involved? I thought notion is singlethreaded, and control doesn't pass back out of lua until the function returns, therefore it's a viable option.

A trustworthy source claims that changing _ENV or using setfenv while the code is already loaded here would mean that print() calls in function calls further down the chain are not replaced, and he suggested that I should replace the global print. It would also be possible to load the code snippet using lua_load() in C (or lua's load()), passing a modified environment.

Other than not being very elegant, I don't see a reason, though.

[raboof]

I thought notion is singlethreaded, and control doesn't pass back out of lua until the function returns, therefore it's a viable option

Aah, indeed everything is generally done from the mainloop, so indeed this should be safe.

It would also be possible to load the code snippet using lua_load() in C (or lua's load()), passing a modified environment. Other than not being very elegant, I don't see a reason, though.

I agree that's not really a relevant difference.

[raboof]

Maybe it's even better to pass stderr, leaving stdout for replies which are formatted as string.format('%q') and can therefore be piped back into lua

In the current version of the PR the 'print' output and the return value are both written to stdout, meaning you can't separate them anymore. I think it would indeed be neat to send the 'print' output to 'stderr' and the 'returned' value to stdout.

[wilhelmy]

That's what I was thinking, too (it does however change normal expectations about print semantics) - I also considered adding a commandline flag but at this point I'm probably hopelessly overengineering it and might as well add getopt...

I've just checked with ack --lua print and nothing that comes with normal notion operation ever seems to call print, so this would be the user knowingly calling print and still expecting a return value.

Either way, this is easy enough to change. I'm still undecided.

[wilhelmy]

Another thing I noticed: notionflux -e 'print "foo"' >/dev/full currently results in SIGPIPE. I know how to fix it.

The SIGPIPE message was actually printed by gdb, notion ignores SIGPIPE so this is actually harmless.

[wilhelmy]

I think with some further testing, this should be ready to merge.

@knixeur (or anyone else reading this) any opinion on whether notionflux's stdout or stderr is the right place to print messages to?

[knixeur]

I'd actually prefer stdout, that's what I would expect if I'm scripting something and calling a print method.
I know it'd be a breaking change so it's a no go, but I'd actually prefer return to return nothing, it's not that I can interact with that result if I don't interpret it as text, which is what I'd actually do with print()'s output :)

[raboof]

I know it'd be a breaking change so it's a no go

I don't think we should be so strict about this for Notion 4.

I'd actually prefer stdout, that's what I would expect if I'm scripting something and calling a print method.

I agree if there is a 'print' call in your script it would make sense for its output to go to stdout. However, what about print statements that are in the code of the loaded modules? I think it would be bad if adding such print statements to a module for diagnostics would break scripts that interact with notion through notionflux.

One option would be to introduce a mod_notionflux.print() function that prints to the notionflux stdout, and document that we recommend only using that from notionflux scripts?

[wilhelmy]

After some thought I'm favouring stdout.

[…] I'd actually prefer return to return nothing, it's not that I can interact with that result if I don't interpret it as text, which is what I'd actually do with print()'s output :)

Good point. I agree it's rather useless for the most part, unless it's a POD type (number, string, boolean or nil), otherwise it gets turned into table: 0xf00b4r or similar by tostring(), which can be used to compare types and object equality in a script calling notionflux at best.

I'd actually prefer stdout, that's what I would expect if I'm scripting something and calling a print method.

Thanks, I completely missed this, errors already go to stderr both in old as well as new notionflux, so they'd be mixed with the print() output if print() goes to stderr.

However, what about print statements that are in the code of the loaded modules? I think it would be bad if adding such print statements to a module for diagnostics would break scripts that interact with notion through notionflux.

I checked and print() is currently not called anywhere except in some contrib/ scripts.

notion's normal stdout is still reachable by calling io.stdout:write() and stderr by io.stderr:write() and warn() (which is used for warning messages all over the codebase) instead of print().

One option would be to introduce a mod_notionflux.print() function that prints to the notionflux stdout, and document that we recommend only using that from notionflux scripts?

That's cumbersome to use, I think I'd prefer stderr before that and I think it makes sense for a print() function to print to the closest user-visible stdout, relative to where the call took place, including functions called indirectly.

The reason for introducing print() in the first place is that long return values are cumbersome to pass out of notionflux, especially in loops, where you have to mess with a temporary table that you need to concatenate and return in the end.

Now that I think about it, It might actually be counterproductive that warn() doesn't go to notionflux's stderr. Opinions?

[raboof]

what about print statements that are in the code of the loaded modules? I think it would be bad if adding such print statements to a module for diagnostics would break scripts that interact with notion through notionflux

I checked and print() is currently not called anywhere except in some contrib/ scripts. (...) warn() (which is used for warning messages all over the codebase) instead of print().

Right, if we document that you should never use print in your regular modules (and always use the logging subsystem there instead) I agree this should be fine 👍

Now that I think about it, It might actually be counterproductive that warn() doesn't go to notionflux's stderr. Opinions?

Sounds reasonable to me (for warnings/errors but not for info/debug) - but let's track that in a separate issue/PR ;)

[wilhelmy]

Right, if we document that you should never use print in your regular modules (and always use the logging subsystem there instead) I agree this should be fine +1

Where should this documentation go?

[raboof]

I'd say for now add them to http://notion.sourceforge.net/notionnotes/node7.html (https://github.com/raboof/notion-doc/blob/master/cstyle.tex), though eventually 'logging' deserves a section of its own (along with notionflux)

[wilhelmy]

Printing from C (printf/puts/...) is unaffected by my patch, I'm only changing lua's global print function temporarily while lua code is running in mod_notionflux, I don't think the C coding style section would be a good fit?

[raboof]

I don't think the C coding style section would be a good fit?

You're right - let's perhaps rename it 'coding style' so we can record conventions for both Lua and C together?

[raboof]

Good to go! Updating the docs can be done in a separate PR