Require setup-gap@v3 to simplify some code

[fingolfin]

While this is not strictly necessary, I feel that if we release a breaking v4 soon anyway, we might as well do this now.

On the other hand, it will make the transition to the new version a bit harder because the user has to first upgrade to setup-gap@v3. But then we probably should do that automatically for everything in gap-packages anyway, with some scripts...

[stertooy]

CI tests failing for this PR seems to be a problem with setup-gap@v3 (my bad!). I'll get a fix in there ASAP.

EDIT: fix is in, CI tests here work now.

[stertooy]

Looks good! (only some tiny remarks)

[fingolfin]

Hmm, this still does

         SetPackagePath(info.PackageName, "/tmp/gaproot/pkg/$(basename $PWD)");

which I really don't like. Actually, now I wonder, why doesn't it just do

         SetPackagePath(info.PackageName, "$PWD");

Am I missing something?

[wilfwilson]

LGTM.

[fingolfin]

One failure left, which seems to be due to an issue in RepnDecomp ?

#I  RepnDecomp: finish reading file 'init.g'
#I  RepnDecomp: start reading file 'read.g'
Syntax warning: Unbound global variable in /home/runner/gap/pkg/repndecomp/lib\
/serre.gi:356
        irred_decomp := ParListByFork(irreps, do_decompose, rec(NumberJobs := \
parallel));
                        ^^^^^^^^^^^^^
Syntax warning: Unbound global variable in /home/runner/gap/pkg/repndecomp/lib\
/serre.gi:358
        irred_decomp := ParListByFork(irreps, do_decompose, rec(NumberJobs := \
4));
                        ^^^^^^^^^^^^^
Syntax warning: Unbound global variable in /home/runner/gap/pkg/repndecomp/lib\
/serre.gi:402
                irred_decomp := ParListByFork(irreps, do_decompose, rec(Number\
Jobs := parallel));
                                ^^^^^^^^^^^^^
Syntax warning: Unbound global variable in /home/runner/gap/pkg/repndecomp/lib\
/serre.gi:406
                irred_decomp := ParListByFork(irreps, do_decompose, rec(Number\
Jobs := 4));
                                ^^^^^^^^^^^^^
#I  RepnDecomp: finish reading file 'read.g'
─────────────────────────────────────────────────────────────────────────────

[fingolfin]

Ah and @wilfwilson is already on top of it, great: gap-packages/RepnDecomp#24

[fingolfin]

The failing test should be taken care of by gap-actions/setup-gap#59 as a workaround, until RepnDecomp has had a release.

[wilfwilson]

Almost... I thought it would be, but I've made gap-actions/setup-gap#60 to hopefully deal with the issue for good.

[wilfwilson]

It's Whac-A-Mole! This reveals a problem with the HeLP package that @fingolfin already identified at gap-packages/HeLP#22.

We didn't encounter this problem earlier because HeLP requires IO, which wasn't compiled, and therefore couldn't be loaded, and so HeLP couldn't be loaded.

[wilfwilson]

We could probably check for these warnings in the PackageDistro when running CI against a package for acceptance. Maybe? i.e. checking that we can load the package without warnings when using OnlyNeeded := true.

[fingolfin]

@wilfwilson we do check packages in the PackageDistro using OnlyNeeded := true! But only with all other package built.

What we don't test is loading packages there with no other packages being built -- and in general this can't work as the needed dependencies for a package obviously must be there in order to be able to test it.

What one could do, at least in principle, is to run a final test round on the PackageDistro, where all the pkg/*/bin dirs are deleted for all packages except those which are direct or indirect "needed dependencies" of the package being tested. Feel free to to open an issue on the PackageDistro repo to suggest that, and/or to make a PR there to do that.

[wilfwilson]

The failing test should be taken care of by gap-actions/setup-gap#59 as a workaround, until RepnDecomp has had a release.

@fingolfin, since RepnDecomp has had a release, we could remove the workaround introduced in gap-actions/setup-gap#59 and then the tests here should then pass (since the only failing test now is HeLP, which only started failing when the workaround in gap-actions/setup-gap#59 was introduced). What do you think?

[limakzi]

I totally missed this conversation.
Love this simplification!

[fingolfin]

I've rebased this PR now; but since we already released v4, I feel this should really be waiting for a v5 release.

It's not urgent right now: before worrying about this PR at all, we should get at least all gap-packages repos to use setup-gap@v3 in their CI.yml. Right now just 9 out of 150 do that :-/.

[limakzi]

@fingolfin Shall we help them and make pull-requests? Haha!