Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
fingolfin opened this issue Jun 17, 2024 · 2 comments · May be fixed by #5762
Open

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

fingolfin opened this issue Jun 17, 2024 · 2 comments · May be fixed by #5762

Comments

@fingolfin
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

fingolfin commented Jun 27, 2024 •
edited
Loading

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:

@fingolfin fingolfin linked a pull request Jul 3, 2024 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

ReadPackage now raises an error if file is missing / cannot be read fingolfin/gap
2 participants
@fingolfin @ChrisJefferson