catch exceptions in GAP.Packages.install

[ThomasBreuer]

This addresses oscar-system/Oscar.jl/issues/1364.

[codecov]

Codecov Report

Merging #815 (7abc854) into master (8dd4fb2) will decrease coverage by 0.02%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master     #815      +/-   ##
==========================================
- Coverage   78.05%   78.03%   -0.03%     
==========================================
  Files          48       48              
  Lines        3669     3687      +18     
==========================================
+ Hits         2864     2877      +13     
- Misses        805      810       +5     

Impacted Files	Coverage Δ
src/packages.jl	63.46% <72.72%> (-3.21%)	⬇️
src/adapter.jl	70.78% <0.00%> (+2.88%)	⬆️
pkg/JuliaInterface/gap/utils.gi	22.95% <0.00%> (+4.76%)	⬆️

[fingolfin]

It addresses it only partially, though: Oscar expects these packages (well, at least ferret) to be loaded in certain places. And by simply silencing the error during startup, we will cause these functions later on to break with no helpful error.

As it is, if go this way, then at the least we should also change Oscar.jl to explicitly check for an error return value when calling this install method and report an appropriate warning to the user (i.e., that a bunch of computations will be slower); or, perhaps better, to adjust the code which relies on e.g. ferret to issue that warning.

Anyway, of course this is still vastly better than the current situation, where some people can't load Oscar at all.

As always, the real fix would be to get GAP_pkg_ferret and so on ready... I'll try to put that in focus again sigh.

[ThomasBreuer]

@fingolfin I agree.

In the current setup, the GAP.Packages.load calls in Oscar set quiet to true (for good reasons), thus the warnings have to be added to the Oscar code. Showing the warnings in the initializations has the advantage that we cannot miss some place in the code where they are relevant (and that they are not shown over and over again). The code in Oscar/experimental/GaloisGrp is already safe w.r.t. the possibility that ferret is not available: There is one place in GaloisGrp.jl where ferret's Solve is called, and this call is avoided if GAP.Globals.ConStabilize is not bound.

Thus the only additional question seems to be where to add suitable warnings about possible performance problems.

(Another point: A quick fix for Oscar would be to wrap the GAP.Packages.load calls into try/catch. Shall we do this for the moment?)

[fingolfin]

I've thought some more about it, and I guess perhaps the "best" fix actually would be to adjust our hack further up in the file you are editing here, where we overwrite a PackageManager function to make it use Julia's download() function. I think we should catch the exception in there because it's actually not advisable to throw a Julia exception through GAP code (in theory it usually works, in practice we (well, probably me) should at some point sit down for 2-3 days and careful audit that case and make sure all corner cases are really dealt with)

[ThomasBreuer]

@fingolfin My idea was to catch any error that may happen on the GAP side during the attempt to install the package, not only the "offline problem". Perhaps we should use each of the two ideas?

[fingolfin]

Yeah, sounds sensible

[ThomasBreuer]

O.k., I will modify the pull request.

[fingolfin]

@ThomasBreuer do you think you'll have a chance to revise this today? Otherwise, I'd just merge this as-is and make a release, so that the immediate bug is fixed; a "better" fix can be done later.

[ThomasBreuer]

@fingolfin I hope the variant I just pushed is good.
Since GAP's PackageManager does not deal with exceptions, the only idea I had was to catch the error in the Julia code that is
executed by PackageManager, in order not to run into a GAP error, and to rethrow the Julia exception.
Now the Julia error message is shown in the end if one is offline.

[fingolfin]

But that's completely redundant, it does nothing at all. I.e. you could just leave it out.

How about this instead?

Suggested change

	catch e
	rethrow()
	end
	catch e
	return GapObj(Dict{Symbol, Any}(:success => false))
	end