Diffs when running testbugfix.g without certain packages available

[wilfwilson]

I was considering running tst/testbugfix.g in the Semigroups CI setup, as an extra check that Semigroups doesn't break things in GAP. But I can't be bothered installing all the necessary packages that some of the tests use. However, if I don't do so, I get diffs:

Observed behaviour

For example (GAP master):

testing: /home/travis/gap/tst/testbugfix/2012-06-18-t00328.tst
########> Diff in /home/travis/gap/tst/testbugfix/2012-06-18-t00328.tst:2
# Input is:
if LoadPackage("anupq",false) <> fail then
for i in [1..192] do Q:=Pq( FreeGroup(2) : Prime:=3, ClassBound:=1 ); od; fi;
# Expected output:
# But found:
#I  anupq package is not available. Check that the name is correct
#I  and it is present in one of the GAP root directories (see '??RootPaths')
########

My understanding of the intention behind the line if LoadPackage("anupq",false) <> fail then is that is the package is installed, then run the following test, and if it is not installed, then don't do anything at all (in particular, no warnings). Instead, it prints some info statements.

Expected behaviour

GAP should not print such info statements when running tests.

[olexandr-konovalov]

I don't see this diff, because I run it with anupq being present in the system. The warning that you see above is displayed when one tries to call LoadPackage with the package name that is not known to GAP at all. Compare:

gap> LoadPackage("linboxing");
#I  linboxing: The compiled linboxing kernel module is not present.
#I  linboxing package is not available. To see further details, enter
#I  SetInfoLevel(InfoPackageLoading,4); and try to load the package again.
fail
gap> LoadPackage("aaa");
#I  aaa package is not available. Check that the name is correct
#I  and it is present in one of the GAP root directories (see '??RootPaths')
fail
brk> IsPackageMarkedForLoading("aaa","");
false

If you want to adapt bugfix test to run in your setup, please submit a PR to add to testbugfix/2012-06-18-t00328.tst an extra check for IsPackageMarkedForLoading("anupq","") before attempting to load it.

[wilfwilson]

Thanks @alex-konovalov, to solve my problem I would have to do this against every testbugfix file that requires a package, and there are quite a few. But I am willing to do this. (Even though why do such tests exist in GAP? Surely they should exist in the appropriate packages? Actually, fair enough that they exist in GAP).

However, I'm concerned that this behaviour is a bug. I think that Info statements are being shown when they shouldn't be. I would expect the presence of these warnings to be related to the InfoClass InfoPackageLoading. But it is not:

 ┌───────┐   GAP 4.10dev-1144-g992fd48 of today
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-apple-darwin17.7.0-default64
 Configuration:  gmp 6.1.2
 Loading the library and packages ...
 Packages:   GAPDoc 1.6.1.dev, PrimGrp 3.3.1, SmallGrp 1.3, TransGrp 2.0.4
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
gap> InfoLevel(InfoPackageLoading);
1
gap> LoadPackage("notarealpackage");
#I  notarealpackage package is not available. Check that the name is correct
#I  and it is present in one of the GAP root directories (see '??RootPaths')
fail
gap> SetInfoLevel(InfoPackageLoading, 0);
gap> InfoLevel(InfoPackageLoading);
0
gap> LoadPackage("notarealpackage");
#I  notarealpackage package is not available. Check that the name is correct
#I  and it is present in one of the GAP root directories (see '??RootPaths')
fail

If we look at lib/package.gi, we find the following culprit:

if InfoLevel(InfoPackageLoading) < 4 then
  Info(InfoWarning,1, name, " package is not available. Check that the name is correct");
  Info(InfoWarning,1, "and it is present in one of the GAP root directories (see '??RootPaths')");
fi;

This is really weird! I can't interpret this in a way that is sensible. Why should the InfoLevel of InfoPackageLoading be relevant if you're going to make an Info statement to the InfoWarning class? And moreover, what is the logic behind the check that the level is less than 4? This means that a sufficiently high level of InfoPackageLoading means fewer InfoStatements, which is exactly the opposite of how things should work. (Max explains below that a separate info statement is given when InfoPackageLoading is 4 or more).

[wilfwilson]

By the way, the default InfoLevel of InfoPackageLoading being 1 contradicts the following from the documentation: Ab initio all info classes have level zero except InfoWarning (7.4-7) which initially has level 1.

[wilfwilson]

By the way @alex-konovalov, I think the appropriate function is TestPackageAvailability rather than IsPackageMarkedForLoading.

[olexandr-konovalov]

@wilfwilson thanks, please feel free to adjust documentation for info levels, and try TestPackageAvailability.

[wilfwilson]

TestPackageAvailability seems to work fine :) I'll prepare a PR soon. I'm going to leave the Info documentation as it is for now, because I'm not sure whether the current behaviour is what is expected or not, or even what the current behaviour is exactly. I'll bring it up at GAP Days next week.

[olexandr-konovalov]

I am sure that we do not want zero as default for InfoPackageLoading, otherwise packages would silently fail to load without any explanation at all.

[fingolfin]

Regarding the test using anupq you quote above: This was added by me, and it was added to GAP on purpose because it tests for a bug that used to be in GAP -- and the easiest way for me to trigger the bug was to use anupq (where the bug was originally reported). Now, it would be possible to rewrite the tests to work without GAP, but that'd require quite a lot of work, and it just didn't seem worth it back then (and to be frank, still doesn't). I imagine the same holds for many other bugfix tests involving packages: they check for issues in GAP, but use a package to trigger the issue.

[fingolfin]

As to the code involving InfoPackageLoading and InfoWarning: My guess is that it looks like this because if InfoPackageLoading is set high, then the message printed by that InfoWarning statement is superfluous. Indeed:

gap> SetInfoLevel(InfoPackageLoading, 4);
gap> LoadPackage("foobsdfsdfsd");
#I  foobsdfsdfsd: no package with this name is installed, return 'fail'
fail

Don't get me wrong, I am not saying that this is great behaviour or clear or anything, just trying to explain why it does what it does.

That said, I still wish GAP would show more helpful errors when LoadPackage failed -- like "couldn't load package FOO because it requires package IO, and IO failed to load because it was not compiled". But doing that well is tricky; plus, people are concerned about these messages showing in undesirable places. I think all of that could be overcome, though, if somebody just invested some time and energy into it. But for now, we gotta work with what we have.

[wilfwilson]

Thanks for the detailed explanation @fingolfin. After thinking about it a bit more I realised why it makes sense for such tests to exist in GAP itself. As for more helpful LoadPackage failure messages, this is something that I often wanted when I was new to GAP; I can imagine how improving that would be tricky.

[fingolfin]

@wilfwilson with PR #2810 merged, is this issue resolved now? If not, could you briefly summarize what problems are left? Thanks!

[wilfwilson]

Unfortunately I missed one, which I have made a PR for on #2848. Once that is merged, this issue is resolved.