Quit GAP with non-zero exit code if processing init files fails and --quitonbreak is being used

[zickgraf]

Currently, GAP prints a warning when a nonexistent file is specified on the command line but otherwise continues unharmed:

$ gap nonexistent_file_1.g nonexistent_file_2.g
 ┌───────┐   GAP 4.13dev-120-g7da3f5a built on 2022-10-27 15:48:01+0200
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-linux-gnu-default64-kv8
 Configuration:  gmp 6.2.1, GASMAN, readline
 Loading the library and packages ...
 Packages:   GAPDoc 1.dev, IO 4.7.0dev, PrimGrp 3.4.0, SmallGrp 1.4.1, TransGrp 2.0.5
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
Could not read file "nonexistent_file_1.g".
Could not read file "nonexistent_file_2.g".
gap>

In particular, it exits with exit code 0 if no other error occurs. In CI tests I would like exit GAP with a non-zero exit code in this case to catch typos, problems in the setup, etc.

I expect that this PR will not be accepted as-is. If this expectation is correct, I would like to use it as a starting point for a discussion how this could be achieved. For example, I could imagine tying this to the options --quitonbreak or --norepl because those are often used in automation where the warning "Could not read file" would possibly go unnoticed.

Text for release notes

TBD

---

Resolves #4994

[fingolfin]

Related issue: #4994

Personally I'd really prefer the behavior in this PR -- but of course it'd change a long standing behavior, and as such might disrupt the workflows of some users... Then again, the reason I'd prefer this to abort GAP is that I have more than once been in a situation where I missed the warning and then wasted time searching for a problem in the wrong place -- i.e., the behavior in this PR would have saved me time. I can't think of any good reason for the current behavior.

So here's my two cents: if e.g. @ThomasBreuer @frankluebeck @ChrisJefferson can think of actual or at least somewhat realistic scenarios where the current behavior is desirable and not easy to replicate by other means, we won't merge this... Otherwise I'd say let's go with it for GAP 4.13.0, and wait for any negative feedback -- we can always revert it in 4.13.1

[ChrisJefferson]

I'm happy with this change, as you say I can't imagine why anyone would want the current behaviour, and it makes it easy to miss bugs.

[ThomasBreuer]

Thanks for pointing to #4994. Because of the inconsistency described there, something has to be fixed, and the question is how.

From the viewpoint of the documentation, I think it would be the easiest solution if files read via the command line behave as if one would call Read (or Test) at the GAP prompt. That is, one gets an error message if a file is missing or not readable or if an error occurs while reading the file.
Or are there reasons why it should make a difference whether one uses the command line or Read?

[frankluebeck]

I agree with @ThomasBreuer : The documentation of GAPs command line options says that filenames on the command line are read with Read (or Test in case of a .tst extension) after intialization and before printing the first prompt. This sounds very sensible to me. So, it would be desirable that gap fn1 fn2 behaved exactly as gap with first two commands Read("fn1"); Read("fn2");.

[zickgraf]

Simply using Read instead of READ_NORECOVERY (and removing all the status checks) seems to work fine: If a file is missing, one gets into a break loop and can return; to continue reading further files. And with --quitonbreak GAP exits with a non-zero exit code as expected.

[fingolfin]

Simply using Read instead of READ_NORECOVERY (and removing all the status checks) seems to work fine: If a file is missing, one gets into a break loop and can return; to continue reading further files. And with --quitonbreak GAP exits with a non-zero exit code as expected.

I'd be very happy with that behavior.

However, for me it doesn't work like you describe: If I replace READ_NORECOVERY by Read and then do gap not_a_file it does not show me a break loop, and gap --quitonbreak not_a_file also does not quit GAP... Does it for you?

[fingolfin]

Nevermind, changing READ_NORECOVERY to Read does work -- I was just stupid and modified the wrong lib/init.g (I have too many GAP's sitting around 😂).

So, updating the PR with that would be great :-)

[zickgraf]

I have updated the PR to use Read and have removed the "and will abort" part from the corresponding documentation. Some more things:

1. READ_NORECOVERY is now unused. Should I remove it?
2. This PR now seems to more or less implement option 2. in Discrepancy between how GAP handles errors when filenames are given as command line argument, and actual behavior #4994, which @fingolfin did not favor.
3. Discrepancy between how GAP handles errors when filenames are given as command line argument, and actual behavior #4994 also mentions syntax errors. I think it would be a good thing to enter a break loop when syntax errors are found in a file because those are easy to miss when reading a file with lots of output. I tried to implement this but noticed that one really does not want a break loop when creating a syntax error in the REPL. I'm not sure how/if the two cases could be distinguished, so I gave up on this.

[fingolfin]

1. yes, please remove READ_NORECOVERY
2. well, what I disliked about option 2 is that it leaves open how syntax errors or genuine errors are handled -- in other words, I think the manual should be improved. So I am happy with the change in this PR, but it does not resolve Discrepancy between how GAP handles errors when filenames are given as command line argument, and actual behavior #4994 in its current form; to do that, it should also adjust the documentation to specify what happens in case of errors; which would be easy with the changes in this PR: "we enter a break loop"
3. Hmmm, for me a syntax error already enters a break loop with this PR? Am I again confusing something?

$ ./gap -b foobar.g
Syntax error: expression expected in foobar.g:1
x:=1-;
     ^
Error, Function Calls: <func> must return a value in
  Read( f ) at /Users/mhorn/Projekte/GAP/gap/lib/init.g:958 called from
ProcessInitFiles( GAPInfo.InitFiles ); at /Users/mhorn/Projekte/GAP/gap/lib/init.g:984 called from
func(  ); at /Users/mhorn/Projekte/GAP/gap/lib/system.g:253 called from
<function "InstallAndCallPostRestore">( <arguments> )
 called from read-eval loop at /Users/mhorn/Projekte/GAP/gap/lib/init.g:985
type 'quit;' to quit to outer loop
brk>

[fingolfin]

Nevermind, I still got an error because I accidentally left it at status := Read instead of just Read.

Anyway, of course we could add another READ variant (replacing READ_NORECOVERY) which would only differ from regular READ in that in it, a syntax error is treated as a hard error. Since we'd only call it from init.g, it would not interfere with the REPL.

[fingolfin]

(And I guess READ_INNER could get an argument BOOL ignoreSyntaxError which would affect how it treats STATUS_ERROR... and possibly immediately abort the reading. Alternatively, no additional argument, but add a return value which reflects if we ever saw STATUS_ERROR.

[zickgraf]

Next iteration:

1. READ_INNER now breaks when encountering errors, in particular syntax errors. When Reading a file, breaking on syntax errors always makes sense, I would say. This does not interfere with syntax errors produced by typing in the REPL.
2. READ_NORECOVERY is gone.
3. I have adjusted the documentation.

[zickgraf]

Argh, this affects the test system which (as I could have expected) also uses READ. I will have to rework this again once I find the time.

[fingolfin]

I would really strongly urge a conservative approach here: don't mess with a central and essential function like READ unless you want to risk having to audit 150 packages and deal with bug reports from an unknown number of end users whose code breaks over this ;-).

Instead, perhaps just keep READ_NORECOVERY but modified to abort upon syntax error. That way the change is strictly localized to those command line init files.

[zickgraf]

Instead, perhaps just keep READ_NORECOVERY but modified to abort upon syntax error. That way the change is strictly localized to those command line init files.

Then there would be a difference between gap file.g and gap with command Read( "file.g" ); again. Thus, I have reverted the latest change.

I.e., this PR now simply uses Read instead of READ_NORECOVERY, and the documentation for "Command Line Options" does not talk about the handling of errors anymore because Read has proper documentation for this.

[fingolfin]

Looks good to me, thanks a lot, I think it's a nice improvement.

What you say about using Read to be faithful to the documentation makes perfect sense to me!

Perhaps we could also improve the documentation, though? It still has this in it: It used to have this in it

If a file cannot be opened GAP will print an error message and will abort.

I guess strictly speaking this is fully accurate, but perhaps not helpful, and could just be removed (UPDATE: which this PR does, and that's fine). But instead I propose we improve it, to tell the user a bit more about what to expect... How about this:

If a file cannot be opened or if executing the code in it raises an error, then the usual error handling for Read respectively Test kicks in. If this enters a break loop, then exiting that break loop also exits GAP.

[fingolfin]

Hmmm... This looks good:

$ ./gap -A -b --quitonbreak foodsfdsfs ; echo $?
Error, file "foodsfdsfs" must exist and be readable at GAPROOT/lib/files.gi:208 called from
1

But if I don't disable break loops, we still get exit code zero:

$ ./gap -A -b foodsfdsfs ; echo $?
Error, file "foodsfdsfs" must exist and be readable at GAPROOT/lib/files.gi:208 called from
Read( f ); at GAPROOT/lib/init.g:957 called from
ProcessInitFiles( GAPInfo.InitFiles ); at GAPROOT/lib/init.g:963 called from
func(  ); at GAPROOT/lib/system.g:253 called from
<function "InstallAndCallPostRestore">( <arguments> )
 called from read-eval loop at GAPROOT/lib/init.g:964
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk> quit;
0

Now, I agree this is still useful for 99% of scripts involving GAP, as they all should use --quitonbreak. But it'd still be nicer if we had a non-zero exit code in both cases? But right now I have no idea what's going on, and don't have time to dive in. If @zickgraf by chance thinks he knows how to quickly fix it: great. If not: no worries, I don't think this should block this PR (nor my remark about documentation).

I'll wait a bit (tomorrow at least) so see if @zickgraf or @ChrisJefferson have further comments, then will merge (possibly after adjusting the headline a bit to make it clear that the non-zero exit code only appears when disabling break loops)

[fingolfin]

Oops, noticed my comment on the docs was not quite correct, updated it. But again, I already approved the PR and won't block it over this, just soliciting more thoughts on this, can change it myself in a different PR later on, too (or not, dunno, need to sleep about it)

[zickgraf]

Ah, so there still is a difference between gap nonexistent_file.g and gap with command Read( "nonexistent_file.g" );. In the first case, typing quit; will exit GAP, while in the second case typing quit; will return to the REPL.

As a possible solution I have pushed commit 3619192 which handles "READ_GAP_ROOT" as an "outer loop" in the sense of the message "you can 'quit;' to quit to outer loop" (the code change is analogous to

gap/src/gap.c

Lines 341 to 342 in 6f3f907

	FlushRestOfInputLine(&input);
	STATE(UserHasQuit) = FALSE;

). With this, the first case above will continue and enter the REPL after typing quit, i.e. it will match the second case. Then not having a non-zero exit code in this case is also expected, because we "handled" the error ourselves by exiting the break loop. However, I expect that this change will be too fundamental to be acceptable :D

The proper fix would be to let SHELL handle the input files really like if one would have typed Read in the REPL. But then one has to handle the case of not REPL separately again.

So in the case the change to READ_GAP_ROOT is too fundamental, I would simply update the documentation in the way you have suggested and say we have to live with it :D

[fingolfin]

Ah, so there still is a difference between gap nonexistent_file.g and gap with command Read( "nonexistent_file.g" );. In the first case, typing quit; will exit GAP, while in the second case typing quit; will return to the REPL.

Yes, and tbh I am happy with that difference :-). so yeah just change the docs

[zickgraf]

Yes, and tbh I am happy with that difference :-). so yeah just change the docs

Done :-)