Various regressions when using `--norepl`

[zickgraf]

2a9d2748cf477b18608346709d78f001278c2023 disables running the program cleanup logic when using --norepl for use with libgap. This has unintended side effects:

• gap --norepl -c "DirectoryTemporary();" creates a temporary directory but never removes it.
• Commands entered in the break loop in gap --norepl -c "Error(\"test\");" are not stored in history.

I don't know what the original problem with libgap was but to me it looks like the program cleanup logic should always be executed, at least when using the standalone gap binary.

[fingolfin]

See #4695

[zickgraf]

Ah, sorry, I should have looked for existing issues first (I remember at one point GitHub automatically suggested similar issues when creating a new issue, was this feature removed?).

The regressing commit is from 2019. Have things possibly improved/changed with libgap so PROGRAM_CLEAN_UP(); could be run with --norepl again? If not, I would close this issue as to be fixed by #4695.

[fingolfin]

Ah, sorry, I should have looked for existing issues first

Nah. While it's of course a good idea to do a quick search, it can be difficult to find those, and I have zero expectations that you do. Also I am not even sure this exactly the same; but it certainly is closely related, so I pointed out the relation.

(I remember at one point GitHub automatically suggested similar issues when creating a new issue, was this feature removed?).

It exists but is fairly limited to match keywords. Perhaps if they extend their Copilot AI or use ChatGPR or so, they could give better suggestions :-).

The regressing commit is from 2019.

I never tracked it down to that commit. But that commit is doing the right thing: when starting GAP, we process init.g. And init.g runs a REPL. But if we use libgap, we don't start a REPL, and we finish processing init.g. So of course it is wrong to execute PROGRAM_CLEAN_UP(); at the end of it.

Have things possibly improved/changed with libgap so PROGRAM_CLEAN_UP(); could be run with --norepl again? If not, I would close this issue as to be fixed by #4695.

We need some way to not run PROGRAM_CLEAN_UP(); when using GAP via libgap.

The problem ultimately comes from --norepl having been added with one specific use case in mind, but it then being used for other, slightly different use cases.

The --norepl flag has multiple issues. First off, it does (at least) two things: it suppresses the REPL and it removes the call to PROGRAM_CLEAN_UP. A better design might be to have two separate switches for both. Alternatively, one could (re)introduce an IsLibgap() helper function and then use a patch like this:

diff --git a/lib/init.g b/lib/init.g
index 6fec1b150..c4d1ef247 100644
--- a/lib/init.g
+++ b/lib/init.g
@@ -952,9 +952,10 @@ if GAPInfo.CommandLineOptions.norepl then
 elif IsHPCGAP and THREAD_UI() then
   ReadLib("hpc/consoleui.g");
   MULTI_SESSION();
-  PROGRAM_CLEAN_UP();
 else
   SESSION();
-  PROGRAM_CLEAN_UP();
 fi;
 
+if not IsLibGap() then
+  PROGRAM_CLEAN_UP();
+fi;

The second issue I have with --norepl is that its name does not fit with the other names, it is negative, and there is no way to undo (i.e., no --repl). The whole set of REPL/error handling command line options is super complicated and difficult to understand. This has of course historic reasons (we tried to stay compatible with existing flags). But I wonder if we should perhaps start over with a new and clean set of alternative flags that make more sense (we could still allow the old names for backwards compatibility, but just map them to a sane combination of the new flags).

But that's getting quite off-topic.

Back on topic: In GAP.jl, we also must disable ProcessInitFiles so that we can handle it a bit later. Considering all that, yet another option that comes to mind would be to cut lib/init.g after line 945 (i.e. right after the definition of ProcessInitFiles), and move the calls afterwards to one or more separate files.

[fingolfin]

The bit about splitting init.g would also allow for an alternate approach to issue #4695 resp. #5414: suppose we process init.g, then afterwards a new run.g, then finally cleanup.g, all invoked from the C code. Then the libgap code would just do the first one. Using --no-repl would just skip the middle bit. And finally regular GAP could always execute cleanup.g, even and especially if run.g exited early with an error.

[zickgraf]

Thanks for the thorough explanation! Splitting init.g sounds nice but also quite intricate (at least for me after a quick grep). Two other thoughts just came to my mind:

1. Is libgap using --norepl or --nointeract? (I could not find the options libgap uses in the code). If it is using --nointeract, we could execute the cleanup logic for --norepl but not for --nointeract. This would at least fix the problem that the history in break-loops is not saved (which I think is the most annoying part of this issue) because with --nointeract no break loops can occur anyway.
2. Can libgap be called with -c and/or files? If not, we could maybe use that to distinguish between the cases. This would also semantically make sense: If no commands are executed or files are read, many cleanup functions are irrelevant.