Improve IsPackageLoaded

First off, it now aborts immediately if the package is not loaded, and
doesn't call the package's `AvailabilityTest` function (fixes #2862).

This was achieved by not using `TestPackageAvailability` anymore, but rather
`IsPackageMarkedForLoading`. The main difference to the latter now is this:
We now record whether a package fully completed loading (in particular, we
record whether its `init.g` and `read.g` executed without an error). This
information is then used by `IsPackageLoaded` after
`IsPackageMarkedForLoading` returned `true`, to determine the final result.

Author: fingolfin

lib/package.gd

@@ -43,9 +43,10 @@
 ##  <Ref Var="GAPInfo.PackagesLoaded"/> is a mutable record,
 ##  its component names are the names of those &GAP; packages that are
 ##  already loaded.
-##  The component for each package is a list of length three, the entries
+##  The component for each package is a list of length four, the entries
 ##  being the path to the &GAP; root directory that contains the package,
-##  the package version, and the package name.
+##  the package version, the package name, and a boolean indicating whether
+##  the package finished loading.
 ##  For each package, the value gets bound in the <Ref Func="LoadPackage"/>
 ##  call.
 ##  <P/>
@@ -353,16 +354,22 @@ DeclareGlobalFunction( "TestPackageAvailability" );
 
 #############################################################################
 ##
-#F  IsPackageLoaded( <name>[, <version>][, <checkall>] )
+#F  IsPackageLoaded( <name>[, <version>] )
 ##
 ##  <#GAPDoc Label="IsPackageLoaded">
 ##  <ManSection>
-##  <Func Name="IsPackageLoaded" Arg='name[, version][, checkall]'/>
+##  <Func Name="IsPackageLoaded" Arg='name[, version]'/>
 ##
 ##  <Description>
-##  This function return <K>true</K> if the given package is loaded, and
-##  <K>false</K> otherwise. For details on the meaning of the arguments,
-##  see <Ref Func="TestPackageAvailability"/>.
+##  For strings <A>name</A> and <A>version</A>, this function tests
+##  whether the &GAP; package <A>name</A> is already loaded in a
+##  version that is at least <A>version</A>, or equal to <A>version</A>
+##  if the first character of <A>version</A> is <C>=</C>
+##  (see <Ref Func="CompareVersionNumbers"/> for further
+##  details about version numbers).
+##  <P/>
+##  The result is <K>true</K> if the package is already loaded,
+##  <K>false</K> otherwise.
 ##  </Description>
 ##  </ManSection>
 ##  <#/GAPDoc>

lib/package.gi

@@ -888,7 +888,7 @@ InstallGlobalFunction( PackageAvailabilityInfo,
       record.InstallationPaths:= record_local.InstallationPaths;
       Add( record.InstallationPaths,
            [ name, [ inforec.InstallationPath, inforec.Version,
-                     inforec.PackageName ] ] );
+                     inforec.PackageName, false ] ] );
       record.Dependencies:= record_local.Dependencies;
       record.StrongDependencies:= record_local.StrongDependencies;
       record.AlreadyHandled:= record_local.AlreadyHandled;
@@ -1005,13 +1005,20 @@ InstallGlobalFunction( TestPackageAvailability, function( arg )
 
 #############################################################################
 ##
-#F  IsPackageLoaded( <name>[, <version>][, <checkall>] )
+#F  IsPackageLoaded( <name>[, <version>] )
 ##
-InstallGlobalFunction( IsPackageLoaded, function( arg )
+InstallGlobalFunction( IsPackageLoaded, function( name, version... )
     local result;
 
-    result := CallFuncList( TestPackageAvailability, arg );
-    return result = true;
+    if Length(version) > 0 then
+        version := version[1];
+    fi;
+    result := IsPackageMarkedForLoading( name, version );
+    if result then
+        # check if the package actually completed loading
+        result := GAPInfo.PackagesLoaded.( name )[4];
+    fi;
+    return result;
     end );
 
 
@@ -1319,10 +1326,10 @@ BindGlobal( "LoadPackage_ReadImplementationParts",
     local pair, info, bannerstring, fun, u, pkgname, namespace;
 
     for pair in secondrun do
+      namespace := pair[1].PackageName;
+      pkgname := LowercaseString( namespace );
       if pair[2] <> fail then
         GAPInfo.PackageCurrent:= pair[1];
-        namespace := pair[1].PackageName;
-        pkgname := LowercaseString( namespace );
         LogPackageLoadingMessage( PACKAGE_DEBUG,
             "start reading file 'read.g'",
             namespace );
@@ -1334,6 +1341,9 @@ BindGlobal( "LoadPackage_ReadImplementationParts",
             "finish reading file 'read.g'",
             namespace );
       fi;
+      # mark the package as completely loaded
+      GAPInfo.PackagesLoaded.(pkgname)[4] := true;
+      MakeImmutable( GAPInfo.PackagesLoaded.(pkgname) );
     od;
 
     # Show the banners.
@@ -1560,7 +1570,8 @@ InstallGlobalFunction( LoadPackage, function( arg )
       # inside the package code causes the files to be read more than once.
       for pkgname in cycle do
         pos:= PositionSorted( paths[1], pkgname );
-        GAPInfo.PackagesLoaded.( pkgname ):= MakeImmutable(paths[2][ pos ]);
+        # the following entry is made immutable in LoadPackage_ReadImplementationParts
+        GAPInfo.PackagesLoaded.( pkgname ):= paths[2][ pos ];
       od;
 
       # If the weight is 1 and the GAP library is not yet loaded

tst/mockpkg/PackageInfo.g

@@ -85,7 +85,10 @@ Dependencies := rec(
   ExternalConditions := [ ],
 ),
 
-AvailabilityTest := ReturnTrue,
+AvailabilityTest := function()
+  Print("oops, should not print here\n");
+  return true;
+end,
 
 # use an empty banner string, so that we get identical output regardless
 # of whether GAP is started with -q or -b, or not.

tst/testinstall/package.tst

@@ -182,6 +182,47 @@ true
 
 #
 # Deal with mock package
+#
+
+# first, force "unload" it (this is a very bad idea in general,
+# but for this mock package, it is OK because we control everything)
+gap> Unbind(GAPInfo.PackagesInfo.mockpkg);
+gap> Unbind(GAPInfo.PackagesLoaded.mockpkg);
+gap> for n in [ "mockpkg_GlobalFunction", "mockpkg_Operation", "mockpkg_Attribute", "mockpkg_Property" ] do
+>   if IsBoundGlobal(n) then
+>     MakeReadWriteGlobal(n);
+>     UnbindGlobal(n);
+>   fi;
+> od;
+
+#
+gap> TestPackageAvailability("non-existing-package");
+fail
+gap> TestPackageAvailability("mockpkg");
+fail
+gap> TestPackageAvailability("mockpkg", "=0.1");
+fail
+gap> TestPackageAvailability("mockpkg", ">=0.1");
+fail
+gap> TestPackageAvailability("mockpkg", "=2.0");
+fail
+gap> TestPackageAvailability("mockpkg", ">=2.0");
+fail
+
+#
+gap> IsPackageLoaded("non-existing-package");
+false
+gap> IsPackageLoaded("mockpkg");
+false
+gap> IsPackageLoaded("mockpkg", "=0.1");
+false
+gap> IsPackageLoaded("mockpkg", ">=0.1");
+false
+gap> IsPackageLoaded("mockpkg", "=2.0");
+false
+gap> IsPackageLoaded("mockpkg", ">=2.0");
+false
+
 #
 gap> mockpkgpath := DirectoriesLibrary("tst/mockpkg")[1];;
 gap> ValidatePackageInfo(Filename(mockpkgpath, "PackageInfo.g"));
@@ -196,12 +237,45 @@ gap> GetPackageNameForPrefix("mock");
 # point GAP at mockpkg
 gap> SetPackagePath("mockpkg", mockpkgpath);
 
-# ... now it "knows" it
+# ... now GAP "knows" the package
 gap> GetPackageNameForPrefix("mock");
 "mockpkg"
 
+#
+gap> TestPackageAvailability("non-existing-package");
+fail
+gap> TestPackageAvailability("mockpkg") = Filename(mockpkgpath, "");
+oops, should not print here
+true
+gap> TestPackageAvailability("mockpkg", "=0.1") = Filename(mockpkgpath, "");
+oops, should not print here
+true
+gap> TestPackageAvailability("mockpkg", ">=0.1") = Filename(mockpkgpath, "");
+oops, should not print here
+true
+gap> TestPackageAvailability("mockpkg", "=2.0");
+fail
+gap> TestPackageAvailability("mockpkg", ">=2.0");
+fail
+
+#
+gap> IsPackageLoaded("non-existing-package");
+false
+gap> IsPackageLoaded("mockpkg");
+false
+gap> IsPackageLoaded("mockpkg", "=0.1");
+false
+gap> IsPackageLoaded("mockpkg", ">=0.1");
+false
+gap> IsPackageLoaded("mockpkg", "=2.0");
+false
+gap> IsPackageLoaded("mockpkg", ">=2.0");
+false
+
 # instruct GAP to load the package, and record all its declarations
 gap> PackageVariablesInfo("mockpkg", "0.1");;
+oops, should not print here
+oops, should not print here
 gap> ShowPackageVariables("mockpkg");
 new global functions:
   mockpkg_GlobalFunction(  )*