gap-system/gap

`ReadPackage` should error when given an invalid path, not silently proceed

Closed this issue · 3 comments

fingolfin commented

Consider this:

gap> ReadPackage("io", "dsfjsdlkfdjsfj");
false

I think that's really problematic behavior. Because it means that if one has a typo in a package's init.g or read.g, one may have to search for quite a bit before discovering this.

It also has other consequences: if a file is missing by accident (error copying it over, whatever), this may manifest in weird errors.

I have one setup were multiple GAP's are loaded in parallel on a multi-core machine, and a certain path goes through a symlink, and for (bad) reasons each time a new GAP is started, that symlink is recreated. I've fixed this by now (I think) but it manifested in a really confusing way: a package would load just fine, but then later calling into it, it might complain about some function not being defined... That can happen if you read the file with DeclareGlobalFunction but not the one with InstallGlobalFunction. This one confused me a lot initially because it was obvious that files both before and after the file with the InstallGlobalFunction call had been read.

All in all, I have by now encountered (and described) multiple situations where having ReadPackage raise an error would be beneficial.

Can anyone name a plausible reason where the current behavior is preferable? (The fact that it is "documented behavior" is true but not a reason by itself, as we can always change this).

If not I'd really like to change this in 4.14 (or even before, really).

Note to anyone trying to implement this: RereadPackage is implemented by calling ReadPackage but it would be bad if ReadPackage raised an error, as that would leave REREADING in the wrong state. So implementation wise, one could either add another optional argument or an option to ReadPackage to make it return a bool again instead of raising an error; or one could rename ReadPackage into _ReadPackage (which would still return a bool) and then call that from both RereadPackage and a new ReadPackage.

ChrisJefferson commented

I agree this behaviour is bad, and we should change it.

If anyone asks for the current behaviour, we can add a function which provides it (I wouldn't bother adding that until anyone wants it)

fingolfin commented

I experimented with this. Turns out, as usual if one is lax about enforcing something, it gets misused, whether intentionally or accidentally sigh.

LoadAllPackages reports this (sorted alphabetically):

ReadPackage could not read <congruence>/lib/buildman.g
ReadPackage could not read <cvec>/gap/matrix.gd
ReadPackage could not read <cvec>/local/calibration.eduroam-ipv4-0510.triple-a.uni-kl.de
ReadPackage could not read <gbnp>/lib/printing2.gd
ReadPackage could not read <gbnp>/lib/printing2.gi
ReadPackage could not read <hap>/lib/ArithmeticGroups/sl2zresalt.gi
ReadPackage could not read <hap>/lib/NonabelianTensor/tensorPair.alt
ReadPackage could not read <hap>/lib/SimplicialGroups/identity.gi
ReadPackage could not read <hecke>/gap/dispatcher.gi
ReadPackage could not read <images>/gap/helper_functions.g
ReadPackage could not read <loops>/etc/HBHforLOOPS.g
ReadPackage could not read <lpres>/gap/homs.gd
ReadPackage could not read <numericalsgps>/gap/infolevelnumsgps
ReadPackage could not read <recog>/gap/obsolete.gi
ReadPackage could not read <semigroups>/gap/tools/enums.gi

Analysis:

  • 👀1
fingolfin commented

I believe all affected packages have now been updated to resolve the issue. That said, some new issues may have crept in, and 3rd party packages may still be affected.