Considerations for lib/slash/ as a submodule.

[kivkiv12345]

Yesterday we (@jeanbaptistelab, @troelsjessen and I) discussed how the removal of the default lib/slash/ loki implementation (see: spaceinventor/slash#7) has provided trouble for PyCSH, which recently received lib/slash/ as a submodule.

We mostly agreed that the addition of lib/slash/ to PyCSH was probably a mistake, meaning its removal is likely pending (a fact that @Ahbork and @fadeto404 may want to know).

It was decided that the desire to run .csh scripts from PyCSH is misguided, and that the ideal 'solution' is for both slash commands and PyCSH to be thin wrappers for the C implementations.

It seems to me that this would require .csh scripts to be rewritten from slash to PyCSH, which seems tedious from an operations perspective.

Personally I am still undecided, as the inclusion of lib/slash/ is quite simple, technically speaking.
And its absence may be a major burden for @Ahbork and @fadeto404.

Regarding APMs, we discussed how their authors could create both a slash and PyCSH/CPython interface.
Technically speaking, I believe it should even be possible for both the Python package and CSH APM to be compiled into the same shared object. But we could probably also have lib/apm compile a shared object for either interfacing shell or language instead.

[jeanbaptistelab]

I may be missing something: what are the troubles in PyCSH caused be removing the loki implementation ? I just tried to build PyCSH and run a few commands, it seems to work fine.

[kivkiv12345]

The inclusion of loki.c became a requirement after including lib/slash/ as a submodule (or at least it should be after merging spaceinventor/slash#7).

I found it quite nasty to copy/paste the entire source file from CSH, but that is only relevant if PyCSH keeps lib/slash/ and only logs calls to .slash_execute()

[jeanbaptistelab]

That's not the case: the removal of the loki functionality from slash was accompanied by a 3-stage refactoring of the code:

Stage 1 (merged):

• introduce a new slash hook "slash_on_execute_hook" called from "slash_execute" where the loki code used to be
• implement in the slash code a weak version of "slash_on_execute_hook" with the original loki code with #pragma warnings about the upcoming removal of loki functionality, resulting in exactly the same functionality + warnings

Stage 2 (merged):

• in CSH, implement "slash_on_execute_hook" with the original loki code
• this result in 2 identical "slash_on_execute_hook" with loki functionality, a weak one in slash, the other one in CSH

Stage 3 (spaceinventor/slash#7 yet to be merged)

• replace the loki implementation from "slash_on_execute_hook" in slash with an empty body (the weak function is still there)
• remove the related #pragma warnings

When that PR is merged, we get:

• CSH providing the loki functionality in its non-weak implementation of "slash_on_execute_hook"
• slash users (including PyCSH) not providing their "slash_on_execute_hook" implementation get the no-op weak version in slash
  In the end, this should not affect CSH, PyCSH or other users of slash...

[jeanbaptistelab]

Actually, those loki-related changes make it possible to include slash into PyCSH without having to compile loki related code...

Building PyCSH master (including slash) without the Stage 1 slash PR results in:

Python 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pycsh
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: /home/jbl/spaceinventor/PyCSH/builddir/pycsh.cpython-310-x86_64-linux-gnu.so: undefined symbol: loki_running
>>>

[kivkiv12345]

It seems you're entirely correct, my apologies for the oversight, I must have confounded loki.c with known_hosts.c

known_hosts.c also has some relevancy for lib/slash/ as a submodule, as it is likely expected that its slash commands ("node add", "node list", "node save") will work. I also recently (as of 2b78aca) made .node() support known_hosts.

[jeanbaptistelab]

No need to apologize @kivkiv12345, this has made me realize that the explanation I gave in this thread is how I should have described the original PRs. I need to be better at communicating my thoughts, at least until others can read them :)