Use JLLs to provide compiled binaries for some GAP packages #1076
Conversation
fingolfin
force-pushed
the
mh/use-package-JLLs
branch
from
4a115c2 to
759bbec
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1076 +/- ##
==========================================
+ Coverage 75.43% 75.55% +0.11%
==========================================
Files 55 55
Lines 4580 4585 +5
==========================================
+ Hits 3455 3464 +9
+ Misses 1125 1121 -4
|
fingolfin
force-pushed
the
mh/use-package-JLLs
branch
2 times, most recently
from
75a4cb8 to
69c4581
Compare
.github/workflows/CI.yml
Outdated
| @@ -69,18 +69,14 @@ jobs: | |||
| - name: "GAP tests" | |||
| run: | | |||
| julia --project=. -e 'import GAP; GAP.create_gap_sh("/tmp")' | |||
| export GAP="/tmp/gap.sh -A --quitonbreak --norepl" | |||
| export GAP="/tmp/gap.sh --quitonbreak --norepl" | |||
There was a problem hiding this comment.
So this is a nasty hack: because we specify -A now any places that already specifies -A needs to remove it... But IMHo that solution doesn't scale well.
Alternative ideas for resolving this:
- we filter the args being passed in to remove any
-Aarguments - we don't use
-Ato reduce the loaded packages, but rather--bare- optionally we could afterwards "manually" load the required packages gapdoc/smallgrp/transgrp/primgrp
- we don't use
-Anor--bareand instead patch the GAP library directly to support the "overrides" (i.e.GAP_lib_jllfor now and possibly later upstream GAP)- if we do it right then
DirectoriesPackageProgramsOverridesexists by the timegap/systemfile.gis executed and we can fill it then. - this would also allow us to load the default set of GAP packages again (I am torn whether we want that, or want
--bare, or-A...)
- if we do it right then
- as a variant of that we could also use another dirty hack and run the code from
gap/pkg.g(which patchesDirectoriesPackagePrograms) from insidepkg/JuliaInterface/PackageInfo.gafter making sure thatJuliaInterfaceis the first package to be loaded...
There was a problem hiding this comment.
Currently my point of view is:
When starting GAP from GAP.jl then we should load JuliaInterface but no other GAP packages.
This way we allow Oscar.jl (or others) to prepare special settings that will influence what happens when certain GAP packages will get loaded, and it is possible to provide/prescribe special packages versions and perhaps to load these packages.
We can provide a small function in GAP.jl that triggers GAP's LoadPackages for those packages that are "wanted by GAP" and that have not yet been loaded.
The question is then how this function can be called automatically but late enough such that the above order of package loading does not get distorted.
fingolfin
force-pushed
the
mh/use-package-JLLs
branch
from
4a05890 to
df2f935
Compare
fingolfin
force-pushed
the
mh/use-package-JLLs
branch
from
fcd015e to
4c5f508
Compare
fingolfin
force-pushed
the
mh/use-package-JLLs
branch
from
4c5f508 to
c6380a1
Compare
|
Summary discussion with @ThomasBreuer: we plan to let GAP.jl use the equivalent of For the This PR however does not do that right now, it is as close as possible to the current status quo -- my idea here being that we can merge this independently from the other change, this is simpler to review and may also help to bisect issues. |
|
I think this is ready for merging now |
|
I don't understand enough about GAP package loading to give educated feedback on this. Maybe a stupid question: The long term plan is still to have |
|
The long term plan still is to introduce |
|
Since I discussed this at length with @ThomasBreuer today I feel pretty confident. So I'll merge this now so that I can continue working, but of course there is still time for feedback and changes if there are any problems |
Resolves #1064
Resolves #928
Resolves #852