Cleanup, ::error:: usage, input validation, README update

[stertooy]

• Update upload-artifact version in README.
• Use ::error:: in echo statements right before exit 1, to make them more visible in GitHub Action logs.
• Replace GAP Error by Exec("echo \"::error:: [...] \""); FORCE_QUIT_GAP(1); for the same reason.
• Validate inputs when relevant (like in run-pkg-tests).
• Made indendation consistent (2 spaces per level).

[limakzi]

Either FORCE_QUIT_GAP or ForceQuitGap.
We mix these methods.
Is there any difference?

[fingolfin]

They are the same but FORCE_QUIT_GAP has been around longer and works in more GAP versions so is appropriate for this kind of script

[limakzi]

Indeed. So, we should always use FORCE_QUIT_GAP in such scripts.

[stertooy]

The capitalised versions have been marked obsolete. My thought process was to keep using the CAPS versions on scripts that are likely to be used with "old" GAP versions (setup-gap, run-pkg-tests, ...), but use the CamelCase versions on scripts that we only expect users to run on newer (or the latest) GAP version (build-pkg-docs, release-pkg, ... ).

If using the CAPS versions everywhere is preferred, I don't mind changing this. I'm guessing they're not going to be actually removed in the near future anyway?

[fingolfin]

We are in general relatively conservative about removing stuff. And only remove them after no package uses them (if we remove them at all). Right now > 100 packages use FORCE_QUIT_GAP in their tst dir, so we are not going to remove this anytime soon (and most likely, never).

So it would be safe to use it everywhere -- but honestly, I also am fine with using a mix, it's not going to cause problems 🤷

[limakzi]

What is the reson for thix change?

[fingolfin]

That is explained in the PR description I think? Or do you mean why this is necessary, why one can't simply insert the "magic string" into the error message? (I suppose one could, possibly after alao inserting a \n before it?)

[limakzi]

Or do you mean why this is necessary, why one can't simply insert the "magic string" into the error message? (I suppose one could, possibly after alao inserting a \n before it?)

Mhm. :)

[stertooy]

Well, I tried using GAP's Print(...) before, but in my experience this was rather unreliable. Sometimes it wouldn't print at all (mostly when GAP exited immediately afterwards), sometimes it would concatenate with the previous line, sometimes it would concatenate with the next line, etc.

Using Error("\n::error::This is the line GitHub puts in red.\n") would work, this would show up in the GitHub logs as

Error,
Error: This is the line GitHub puts in red.
called from [...]
Error: Process completed with exit code 1.

with the second and fourth line being in red and shown to the user on the Summary. Using Exec, we get just

Error: This is the line GitHub puts in red.
Error: Process completed with exit code 1.

which is a lot cleaner IMO.