epics-modules / lua

Lua Scripting Record for EPICS
https://epics-modules.github.io/lua/
7 stars 4 forks source link

redirection in luash #22

Closed bfrk closed 3 years ago

bfrk commented 3 years ago

In the iocsh I can do output redirection as in

dbl > somefile

This is quite useful IMO. I am not suggesting to add special syntax to luash to mimic that, this would make no sense at all, given that > is a valid operator symbol in lua. But it would be nice to have a built-in procedure with that effect. Something like

redirect_stdout(dbl,"somefile")

BTW, I think adding ad-hoc syntax e.g. for < luash_script and exit is not a good idea. These can (and IMO should) both be replaced with plain procedure calls.

keenanlang commented 3 years ago

Pushed some commits, redirecting stdout can be done with osi.startRedirectOut and osi.endRedirectOut.

osi = require("osi")

osi.startRedirectOut("my-pvs.txt")
dbl()
osi.endRedirectOut()
bfrk commented 3 years ago

I would prefer a less stateful (and less baroque) way to do this i.e. encapsulated as a higher order procedure as I proposed above.

There is also something strange going on with macro replacements. This works in my startup script:

set_savefile_path("${LOG_DIR}/autoSaveRestore")
set_pass1_restoreFile("${IOC}_1.sav")

but this does not:

osi.startRedirectOut("${LOG_DIR}/Database/${IOC}.dbior")
lua: stdin:1: Error in opening file (${LOG_DIR}/Database/${IOC}.dbior)

stack traceback:
        [C]: in function 'osi.startRedirectOut'
        stdin:1: in main chunk

and neither does

SIOC99C>print("${IOC}")
${IOC}

I must say I don't quite understand how macro replacement in strings works. This is certainly not a lua concept, so I guess it's a feature of luash?

keenanlang commented 3 years ago

The string replacement is weird because of how calls to the registered IOC shell functions are executed. I previously had added a function into base that allowed me to query the IOC's function registrar for the functions that had been registered. However, later on, one of the base developers decided that they didn't like the idea of having that access to outside code, so it was removed. Because that got removed, the way that calling IOC shell functions happens is by constructing a string and calling iocshCmd. iocshCmd is what is processing the string replacements and why it is only happening when calling epics functions.

Ideally, string replacement wouldn't be happening in any instance, but with the way that epics base is set up (and the way the devs want to keep it), there's no way to fully replace the IOC shell without writing a wrapper function for every epics-exposed function that exists and then having a separate registration with the lua library/function registration I have set up.

bfrk commented 3 years ago

Context: https://github.com/epics-modules/lua/issues/22

The string replacement is weird because of how calls to the registered IOC shell functions are executed. I previously had added a function into base that allowed me to query the IOC's function registrar for the functions that had been registered. However, later on, one of the base developers decided that they didn't like the idea of having that access to outside code, so it was removed.

A very bad decision IMO. Can we please reopen discussion on that?

Because that got removed, the way that calling IOC shell functions happens is by constructing a string and calling iocshCmd. iocshCmd is what is processing the string replacements and why it is only happening when calling epics functions.

You should not be forced to resort to such a horrible kludge.

Cheers Ben -- I would rather have questions that cannot be answered, than answers that cannot be questioned. -- Richard Feynman

bfrk commented 3 years ago

On 3/11/21 1:39 AM, Ben Franksen via Core-talk wrote:

Context: https://github.com/epics-modules/lua/issues/22

The string replacement is weird because of how calls to the registered IOC shell functions are executed. I previously had added a function into base that allowed me to query the IOC's function registrar for the functions that had been registered. However, later on, one of the base developers decided that they didn't like the idea of having that access to outside code, so it was removed.

A very bad decision IMO. Can we please reopen discussion on that?

My objection to iocshFindCommand() as a public API still stands. I feel it exposes too many details of how iocsh currently works. Specifically, by exposing the function pointer it prevents this type from ever being changed. eg. I'm thinking to add a second, alternative, form which can return an error, and pass in an extra void* as context.

I'm open to adding an alternative to iocshCmd() provided it doesn't close off avenues for future work. Perhaps passing in an array of strings?

Because that got removed, the way that calling IOC shell functions happens is by constructing a string and calling iocshCmd. iocshCmd is what is processing the string replacements and why it is only happening when calling epics functions.

You should not be forced to resort to such a horrible kludge.

Cheers Ben

mdavidsaver commented 3 years ago

one of the base developers

You can name me when I'm responsible :)

bfrk commented 3 years ago

Where can I read about the proposal to add iocshFindCommand?

bfrk commented 3 years ago

Am 11.03.21 um 16:13 schrieb Michael Davidsaver:

On 3/11/21 1:39 AM, Ben Franksen via Core-talk wrote:

Context: https://github.com/epics-modules/lua/issues/22

The string replacement is weird because of how calls to the registered IOC shell functions are executed. I previously had added a function into base that allowed me to query the IOC's function registrar for the functions that had been registered. However, later on, one of the base developers decided that they didn't like the idea of having that access to outside code, so it was removed.

A very bad decision IMO. Can we please reopen discussion on that?

My objection to iocshFindCommand() as a public API still stands. I feel it exposes too many details of how iocsh currently works. Specifically, by exposing the function pointer it prevents this type from ever being changed.

If I were Keenan I might have suggested you mark this API function as unstable and promised to deal with any improvements / internal restructurings by adapting my code when and if they happened.

I think this is a viable compromise between "prevent it form ever being changed" and "do not provide an API at all".

eg. I'm thinking to add a second, alternative, form which can return an error, and pass in an extra void* as context.

I'm open to adding an alternative to iocshCmd() provided it doesn't close off avenues for future work. Perhaps passing in an array of strings?

I am not sure this would address the issue, which is the handling of environment variable expansion. Perhaps what needs to be done is to duplicate that functionality in luash, then pass the expanded command line to iocshCmd? Of course in luash the expansion would be limited to (literal) strings.

BTW, the relevant discussion seems to have happened here: https://code.launchpad.net/~epics-core/epics-base/+git/Com/+merge/366876 I find it a bit hard to make sense of.

keenanlang commented 3 years ago

I would prefer a less stateful (and less baroque) way to do this i.e. encapsulated as a higher order procedure as I proposed above.

I'm going to have to end up cleaning up the backend shell code a bit because I'm pretty sure there's a cleaner way to structure everything, but in terms of user-facing code, you now have the 'exec' command, which likely won't change (or, at least, won't have breaking changes from what I describe here).

By itself, it just executes a string as a line of code, not really any different than the dostring function.

exec("print('Hello, World')")
Hello, World

However, when passed a table, it checks for fields named "output", "out", "input", and "in" for names of files for redirection. Note that lua provides syntactic sugar for easily passing tables.

-- equivalent to "print('Hello, World') > output.txt"
exec{"print('Hello, World')", output="output.txt")

-- works just like "< next_file.lua"
exec{input="next_file.lua"}

Note: If input is specified, any command that is in the positional slot is ignored as I didn't know what else would be a good way to deal with that case.

mdavidsaver commented 3 years ago

... I might have suggested you mark this API function as unstable ...

There hasn't so far been a history of this in Base, though I also think it would be a good idea. @anjohnson has been ambivalent when I've raised the possibility.

I guess we'll have to settle with that old standby of making sure it is hard to find, difficult to use, and undocumented!

bfrk commented 3 years ago

Am 14.03.21 um 09:23 schrieb Keenan Lang:

I would prefer a less stateful (and less baroque) way to do this i.e. encapsulated as a higher order procedure as I proposed above.

I'm going to have to end up cleaning up the backend shell code a bit because I'm pretty sure there's a cleaner way to structure everything, but in terms of user-facing code, you now have the 'exec' command, which likely won't change (or, at least, won't have breaking changes from what I describe here).

By itself, it just executes a string as a line of code, not really any different than the dostring function.

exec("print('Hello, World')")
Hello, World

However, when passed a table, it checks for fields named "output", "out", "input", and "in" for names of files for redirection. Note that lua provides syntactic sugar for easily passing tables.

-- equivalent to "print('Hello, World') > output.txt"
exec{"print('Hello, World')", output="output.txt")

-- works just like "< next_file.lua"
exec{input="next_file.lua"}

Note: If input is specified, any command that is in the positional slot is ignored as I didn't know what else would be a good way to deal with that case.

Thanks a lot! I was partially successful with using exec.

However, it seems using Lua for scripting EPICS is not accepted as a in our working group: people fear that they may have undue difficulty to maintain anything written in Lua. So it won't make much sense for me to continue evaluating luash as an alternative to the iocsh.

mdavidsaver commented 3 years ago

@bfrk If this is a reference to discussions with me, please understand that my objections are to specific questions about between Base and an external module. I have some experience maintaining complex lua scripts, which I haven't found to be any more or less difficult than eg, larger python scripts. imo. code size is more of a factor than language.

bfrk commented 3 years ago

Am 24.03.21 um 14:15 schrieb mdavidsaver:

@bfrk If this is a reference to discussions with me

Sorry, no, completely unrelated.

please understand that my objections are to specific questions about between Base and an external module. I have some experience maintaining complex lua scripts, which I haven't found to be any more or less difficult than eg, larger python scripts. imo. code size is more of a factor than language.

Yes, code size is very important, perhaps the most important factor. It is not independent from language though: the idiomatic solution in one language may be a lot more concise than in another one (but whether that is the case also depends on the problem to solve).

bfrk commented 3 years ago

Anyway, as far as I am concerned the ticket can be closed.

mdavidsaver commented 3 years ago

the idiomatic solution in one language may be a lot more concise than in another one

Agreed. I'll leave it to you to decide if a translation to lua would improve a complicated iocsh script https://github.com/areaDetector/ADCore/blob/master/iocBoot/EXAMPLE_commonPlugins.cmd

bfrk commented 3 years ago

Am 24.03.21 um 15:31 schrieb mdavidsaver:

the idiomatic solution in one language may be a lot more concise than in another one

Agreed. I'll leave it to you to decide if a translation to lua would make be an improvement for a complicated iocsh script https://github.com/areaDetector/ADCore/blob/master/iocBoot/EXAMPLE_commonPlugins.cmd

Impressive. There appears to be a fair amount of repetition. I haven't actually tried it but it certainly looks as if that could be reduced with plain old for loops.

bfrk commented 3 years ago

One last (belated) comment: one of the things that prompted me to investigate luash is that it would allow shell commands to return meaningful result values (besides success/failure). For instance, running a sequencer program could return the thread ID of the started instance, along with the status code. You could assign that to a variable with a meaningful name and pass that variable to e.g. seqShow, instead of having to first run seqShow, lookup the thread ID in the output, then paste the number as argument to a second seqShow call.