Closed nthiery closed 5 years ago
Description changed:
---
+++
@@ -3,9 +3,34 @@
so that we can get rid of our separate libgap package.
The branch to be attached to this ticket is a preliminary experiment
-from a fork of Markus to be merged in the devel version of GAP.
+from a fork by Markus to be merged in the devel version of GAP.
-Fetching the Markus's GAP sources:
+
+What the branch does:
+- Remove the libgap spkg
+
+- Update the gap spkg to the new build system and build and install libgap
+
+- Replace gap.shi.patch by a plain gap startup script for Sage
+
+ Rationale: GAP used to provide a startup shell script. The GAP devs
+ are in the process of getting rid of it and provide a very minimal
+ one. They recommend to just write our own rather than patching it.
+
+- Update a few doctests w.r.t. changes of output of some GAP functions
+
+- ???
+
+TODO:
+
+- Automatic handling of headers (see below for how to do it by hand).
+ GAP's build system will eventuall provide a rule to install headers
+ which will make this trivial.
+
+- Use GAP's own `make install` when it will be implemented.
+
+
+Fetching Markus's GAP sources:
git clone git@github.com:markuspf/gap.git $LIBGAP
@@ -42,9 +67,10 @@ sage -f gap # -s
-Copy GAP's header files, as well as gen/config.h to $SAGE/local/include
-
-Fix them to adapt the include path: #include <src/...> -> #include <gap/...>
+Header files:
+- Copy GAP's header files, as well as gen/config.h to $SAGE/local/include
+- Fix them to adapt the include path: #include <src/...> -> #include <gap/...>
+- **Replace T_INT by 0 in TNUM_OBJ, around line 414 of objects.h**
Run:
@@ -52,3 +78,52 @@
sage -b
+Basic tests on libgap: + +```
+Running most relevant tests:
+```
+Testing packages with dynamic loading (e.g. IO):
+Install IO:
+```
+Test it locally:
+```
+This does not yet work:
+```
Description changed:
---
+++
@@ -18,6 +18,16 @@
one. They recommend to just write our own rather than patching it.
- Update a few doctests w.r.t. changes of output of some GAP functions
+
+- **Possibly controversial:** The new libgap currently *does not come*
+ with symbol rewriting (Foo -> libGAP_Foo). This avoids messing
+ around with GAP's sources; in particular opening the door for using
+ a stock GAP from the OS distribution. However there always is a risk
+ of name conflict. And indeed, GAP's constants (actually cpp macros)
+ T_INT, T_FLOAT, ... conflict with Python's constants. This is worked
+ around by duplicating and forcing their value in gap_include.pxd.
+ Another conflict in objects.h currently needs to be workedaround by
+ hand (see below).
- ???
Description changed:
---
+++
@@ -26,7 +26,7 @@
of name conflict. And indeed, GAP's constants (actually cpp macros)
T_INT, T_FLOAT, ... conflict with Python's constants. This is worked
around by duplicating and forcing their value in gap_include.pxd.
- Another conflict in objects.h currently needs to be workedaround by
+ Another conflict in objects.h currently needs to be worked around by
hand (see below).
- ???
Description changed:
---
+++
@@ -39,6 +39,8 @@
- Use GAP's own `make install` when it will be implemented.
+- Update the documentation in sage.libs.gap.libgap.pyx to not mention
+ the `libgap_` prefix
Fetching Markus's GAP sources:
Description changed:
---
+++
@@ -28,6 +28,11 @@
around by duplicating and forcing their value in gap_include.pxd.
Another conflict in objects.h currently needs to be worked around by
hand (see below).
+
+- Removes configure.patch: it was patching configure for better GMP
+ detection under Cygwin (#13954). This should not be needed anymore
+ with the new build system and use of --with-gmp. If it is, upstream
+ asked for it to be reported and they will fix it.
- ???
Branch: u/nthiery/upgrade_to_gap_4_9
Branch pushed to git repo; I updated commit sha1. New commits:
1c645e0 | #22626: remove GAP's symbol prefixing in libgap: libGAP_Foo -> Foo |
1ecc67b | #22626: doctest update w.r.t. minor changes of output in GAP |
e8ebbca | #22626: GMP detection patch for cygwin should not be needed anymore |
fd65b06 | #22626: Remove libgap spkg |
9511733 | #22626: replace patch for GAP's startup script template in favor of a custom script |
550625e | #22626: remove GAP's symbol prefixing in libgap: libGAP_Foo -> Foo, and workaround GAP <-> Python symbol conflict |
a9c859c | #22626: updated gap spkg w.r.t. GAP's devel version and its new build system; also include compilation and installation of libgap |
3011ac0 | Merge branch 'develop' into t/22626/upgrade_to_gap_4_9 |
Commit: 3011ac0
Description changed:
---
+++
@@ -1,9 +1,12 @@
GAP 4.9 will come with a completely rewritten build system that will
-simplify our packaging. It will also enable building GAP as a library,
-so that we can get rid of our separate libgap package.
+simplify our packaging. In fact, it may well enable Sage to use a vanilla GAP installation as provided by the distribution. It will also enable building GAP as a library, so that we can get rid of our separate libgap package.
-The branch to be attached to this ticket is a preliminary experiment
-from a fork by Markus to be merged in the devel version of GAP.
+The branch attached to this ticket updates Sage to run on top of
+[a branch of GAP](https://github.com/markuspf/gap/tree/hpc-merge-libgap)
+by Markus Pfeiffer that adds libgap compilation and will merged soon
+in the devel version of GAP.
+
+Tentatively, all tests pass!
What the branch does:
Description changed:
---
+++
@@ -3,7 +3,7 @@
The branch attached to this ticket updates Sage to run on top of
[a branch of GAP](https://github.com/markuspf/gap/tree/hpc-merge-libgap)
-by Markus Pfeiffer that adds libgap compilation and will merged soon
+by Markus Pfeiffer that adds libgap compilation and will be merged soon
in the devel version of GAP.
Tentatively, all tests pass!
Description changed:
---
+++
@@ -3,7 +3,7 @@
The branch attached to this ticket updates Sage to run on top of
[a branch of GAP](https://github.com/markuspf/gap/tree/hpc-merge-libgap)
-by Markus Pfeiffer that adds libgap compilation and will be merged soon
+by Markus Pfeiffer that adds libgap compilation and [|will be merged](https://github.com/fingolfin/gap/pull/64) soon
in the devel version of GAP.
Tentatively, all tests pass!
Description changed:
---
+++
@@ -45,7 +45,7 @@
GAP's build system will eventuall provide a rule to install headers
which will make this trivial.
-- Use GAP's own `make install` when it will be implemented.
+- Use GAP's own `make install` [when it will be implemented](https://github.com/fingolfin/gap/issues/44).
- Update the documentation in sage.libs.gap.libgap.pyx to not mention
the `libgap_` prefix
Description changed:
---
+++
@@ -30,14 +30,12 @@
T_INT, T_FLOAT, ... conflict with Python's constants. This is worked
around by duplicating and forcing their value in gap_include.pxd.
Another conflict in objects.h currently needs to be worked around by
- hand (see below).
+ hand (see below). Something similar was started at #19915.
- Removes configure.patch: it was patching configure for better GMP
detection under Cygwin (#13954). This should not be needed anymore
with the new build system and use of --with-gmp. If it is, upstream
asked for it to be reported and they will fix it.
-
-- ???
TODO:
@@ -49,6 +47,11 @@
- Update the documentation in sage.libs.gap.libgap.pyx to not mention
the `libgap_` prefix
+
+- Check against #19915 to see if any of the changes there should be
+ ported here. Then close as won't fix.
+
+- ???
Fetching Markus's GAP sources:
Work Issues: Wait for gap 4.9 release
Changed keywords from none to days85, libgap
Description changed:
---
+++
@@ -36,6 +36,8 @@
detection under Cygwin (#13954). This should not be needed anymore
with the new build system and use of --with-gmp. If it is, upstream
asked for it to be reported and they will fix it.
+
+- Revert #19726 (not needed anymore)
TODO:
Branch pushed to git repo; I updated commit sha1. New commits:
234c54b | #22626: revert #19726 as it won't be needed for gap 4.9 |
Description changed:
---
+++
@@ -3,10 +3,11 @@
The branch attached to this ticket updates Sage to run on top of
[a branch of GAP](https://github.com/markuspf/gap/tree/hpc-merge-libgap)
-by Markus Pfeiffer that adds libgap compilation and [|will be merged](https://github.com/fingolfin/gap/pull/64) soon
-in the devel version of GAP.
+by Markus Pfeiffer that adds libgap compilation and [will be merged](https://github.com/fingolfin/gap/pull/64) soon in the devel version of GAP.
Tentatively, all tests pass!
+
+See https://github.com/markuspf/gap/issues/2 for the few sticking points that could prevent using a vanilla GAP from the distribution (please edit further if you think about more of them).
What the branch does:
Attachment: ptestlong.log
Description changed:
---
+++
@@ -4,8 +4,6 @@
The branch attached to this ticket updates Sage to run on top of
[a branch of GAP](https://github.com/markuspf/gap/tree/hpc-merge-libgap)
by Markus Pfeiffer that adds libgap compilation and [will be merged](https://github.com/fingolfin/gap/pull/64) soon in the devel version of GAP.
-
-Tentatively, all tests pass!
See https://github.com/markuspf/gap/issues/2 for the few sticking points that could prevent using a vanilla GAP from the distribution (please edit further if you think about more of them).
@@ -40,6 +38,15 @@
- Revert #19726 (not needed anymore)
+Status: Most long test pass. Tentatively, the 38 remaining failing
+tests are due to changes in GAP since 4.8.6: Max mentioned that the
+library has been cleaned up to always use the same random generation
+source, and some of the group algorithms were changed as well, which
+can explain, e.g. change of orders in lists of elements. So those
+should be nothing to worry about. There is not much point in updating
+those doctests right away; we may as well wait for a more final
+version of 4.9 to be out.
+
TODO:
- Automatic handling of headers (see below for how to do it by hand).
@@ -53,6 +60,8 @@
- Check against #19915 to see if any of the changes there should be
ported here. Then close as won't fix.
+
+ - Update doctests as needed
- ???
Description changed:
---
+++
@@ -26,10 +26,11 @@
around with GAP's sources; in particular opening the door for using
a stock GAP from the OS distribution. However there always is a risk
of name conflict. And indeed, GAP's constants (actually cpp macros)
- T_INT, T_FLOAT, ... conflict with Python's constants. This is worked
- around by duplicating and forcing their value in gap_include.pxd.
- Another conflict in objects.h currently needs to be worked around by
- hand (see below). Something similar was started at #19915.
+ T_INT, T_FLOAT, ... conflict with Python's constants. This is
+ currently worked around by forcing the inclusion of Python's
+ `structmember.h` before the gap headers.
+
+ Something similar was started by Volker at #19915.
- Removes configure.patch: it was patching configure for better GMP
detection under Cygwin (#13954). This should not be needed anymore
Branch pushed to git repo; I updated commit sha1. New commits:
7c04025 | 22626: document the work around for the GAP-Python name clash on T_INT, ... |
So if I understand the mechanics involved, at the moment you have to configure and install gap. Then clean and configure and install for libgap. So it is still behaving like "two packages" with the same source code. Do we have an idea on when we'll have just have gap linking on libgap which would also solve this particular problem?
Salut François,
Replying to @kiwifb:
So if I understand the mechanics involved, at the moment you have to configure and install gap. Then clean and configure and install for libgap. So it is still behaving like "two packages" with the same source code.
Exactly.
Do we have an idea on when we'll have just have gap linking on libgap which would also solve this particular problem?
The GAP developers are well aware of that and planning to implement it. They have not yet done yet just to be more incremental; their new build system is a big PR already :-) I would assume that this will be done before GAP 4.9 which they are planning for a couple months from now.
See: https://github.com/markuspf/gap/issues/2
Btw: feel free to comment / expand on the list there. I'll advertise this issue for additional feedback on the sage-packaging mailing list.
Replying to @kiwifb:
So if I understand the mechanics involved, at the moment you have to configure and install gap. Then clean and configure and install for libgap. So it is still behaving like "two packages" with the same source code.
For this reason, I wonder why you didn't keep the separation GAP + libGAP in Sage too. If it effectively behaves as two packages, it seems more natural to keep it as two packages in Sage too. Imagine for example that we need to patch GAP but not libGAP or conversely, that would be harder with the current approach. That being said, this is mostly bikeshedding. So, if the current setup works well, there might be no reason to change it.
Replying to @jdemeyer:
Replying to @kiwifb:
So if I understand the mechanics involved, at the moment you have to configure and install gap. Then clean and configure and install for libgap. So it is still behaving like "two packages" with the same source code.
For this reason, I wonder why you didn't keep the separation GAP + libGAP in Sage too. If it effectively behaves as two packages, it seems more natural to keep it as two packages in Sage too. Imagine for example that we need to patch GAP but not libGAP or conversely, that would be harder with the current approach. That being said, this is mostly bikeshedding. So, if the current setup works well, there might be no reason to change it.
Two completely separate packages would also mean that gap_packages
should come in two different flavours, etc. I'd rather keep it simple and do not multiply these instances (but rather hope that libGAP
will support all of GAP packages soon).
Replying to @dimpase:
Two completely separate packages would also mean that
gap_packages
should come in two different flavours, etc.
So, you are saying that we need to install gap_packages
twice too? Once for GAP and once for libGAP?
Replying to @jdemeyer:
For this reason, I wonder why you didn't keep the separation GAP + libGAP in Sage too.
I wanted to experiment with how far we were from having a proper single package, so as to give early feedback to the gap developers on any sticking points. And indeed, linking gap to libgap is basically the only sticking point. Since this point will be most likely resolved by the time this ticket gets merged in, we might as well shoot directly for the "right thing".
Replying to @jdemeyer:
Replying to @dimpase:
Two completely separate packages would also mean that
gap_packages
should come in two different flavours, etc.So, you are saying that we need to install
gap_packages
twice too? Once for GAP and once for libGAP?
no, not at all---I mean to say that there exist GAP packages (not part of Sage ATM) that break libGAP now. As some of these packages are very useful, it's important to have this fixed (but not at expense of having 2 separate gap_packages
).
Replying to @dimpase:
As some of these packages are very useful, it's important to have this fixed
YES. In fact that is my main motivation to work on the libgap integration: I badly want to use Semigroups that requires IO! We went through the details with Markus and Max, and the shared aim is indeed to have this resolved with the upgrade to 4.9.
Is there any news here?
GAP 4.9.0, a beta release for GAP 4.9, has been released.
This means we can start working on building Sage with it.
Do you know if they now provide a replacement for libGAP
?
Description changed:
---
+++
@@ -7,8 +7,8 @@
See https://github.com/markuspf/gap/issues/2 for the few sticking points that could prevent using a vanilla GAP from the distribution (please edit further if you think about more of them).
+What the branch does:
-What the branch does:
- Remove the libgap spkg
- Update the gap spkg to the new build system and build and install libgap
@@ -69,38 +69,38 @@
Fetching Markus's GAP sources:
git clone git@github.com:markuspf/gap.git $LIBGAP
cd $LIBGAP
git remote add markuspf git@github.com:markuspf/gap.git
git fetch markuspf
git checkout -b markuspf/hpc-merge-libgap
./autogen.sh
./configure
make bootstrap-pkg-minimal +git clone git@github.com:markuspf/gap.git $LIBGAP +cd $LIBGAP +git remote add markuspf git@github.com:markuspf/gap.git +git fetch markuspf +git checkout -b markuspf/hpc-merge-libgap +./autogen.sh +./configure +make bootstrap-pkg-minimal
Testing libgap:
./configure --enable-libgap
make -j4 libgap
make test-libgap +./configure --enable-libgap +make -j4 libgap +make test-libgap
Build and install a tardist for Sage, and rebuild the spkg:
make distclean
./autogen.sh
./configure
make manuals
make clean +make distclean +./autogen.sh +./configure +make manuals +make clean
(cd ..; tar zcvf $SAGE/upstream/$GAP.tar.gz --exclude .git $GAP) +(cd ..; tar zcvf $SAGE/upstream/$GAP.tar.gz --exclude .git $GAP)
sage --package fix-checksum
sage -f gap # -s +sage --package fix-checksum +sage -f gap # -s
Header files:
@@ -111,23 +111,23 @@
Run:
sage -b +sage -b
Basic tests on libgap:
sage: libgap.eval("GAPInfo.Version")
sage: libgap.DihedralGroup(10).CharacterTable()
CharacterTable( <pc group of size 10 with 2 generators> )
sage: libgap.Group(libgap.eval("[(1,2,3),(1,2)]")).Size()
6 +sage: libgap.eval("GAPInfo.Version") +sage: libgap.DihedralGroup(10).CharacterTable() +CharacterTable( <pc group of size 10 with 2 generators> ) +sage: libgap.Group(libgap.eval("[(1,2,3),(1,2)]")).Size() +6
Running most relevant tests:
sage -tp 8 sage/groups sage/libs/gap +sage -tp 8 sage/groups sage/libs/gap
Current status: all tests pass!
@@ -136,29 +136,29 @@ Install IO:
- cd $SAGE/local/gap/latest/pkg
- wget http://www.gap-system.org/pub/gap/gap4/tar.gz/packages/io-4.4.6.tar.gz
- tar xvf /tmp/io-4.4.6.tar.gz
- mv io-4.4.6 io
- cd io
- ./configure
- make
+cd $SAGE/local/gap/latest/pkg
+wget http://www.gap-system.org/pub/gap/gap4/tar.gz/packages/io-4.4.6.tar.gz
+tar xvf /tmp/io-4.4.6.tar.gz
+mv io-4.4.6 io
+cd io
+./configure
+make
Test it locally:
- cd ../..
- ./gap -l .
- gap> LoadPackage("IO");
- true
+cd ../..
+./gap -l .
+gap> LoadPackage("IO");
+true
This does not yet work:
- sage: libgap.LoadPackage("IO")
- ValueError: libGAP: Error, module '/opt/sage-git/local/gap/latest/pkg/io/bin/x86_64-pc-linux-gnu-gcc-default64/io.so' not found
+sage: libgap.LoadPackage("IO")
+ValueError: libGAP: Error, module '/opt/sage-git/local/gap/latest/pkg/io/bin/x86_64-pc-linux-gnu-gcc-default64/io.so' not found
This should be fixed once GAP's gap binary is built on top of libgap. See: https://github.com/markuspf/gap/issues/1.
Cc-ing @alex-konovalov and @sagetrac-markuspf.
Replying to @dimpase:
Do you know if they now provide a replacement for
libGAP
?
In principle yes: the branch I posted here last April was using their stock alpha version. Now the next step is to revive the branch and adapt it if needed.
I can work on this, but probably not at once. Any volunteer to take over welcome.
Markus: do you foresee any major changes in how GAP's lib is to be built that could affect what I was doing last April?
I just noticed that in 4.9 smallgrps and transgrps are released under a GPL-compatible license. This is great! We really should make them (i.e. database_gap) standard Sage package, and do away with the extra hurdle of having database_gap optional.
If you're willing to wait 2 months, this might be a good task for the Cernay workshop https://github.com/OpenDreamKit/OpenDreamKit/issues/251
Replying to @jdemeyer:
If you're willing to wait 2 months, this might be a good task for the Cernay workshop https://github.com/OpenDreamKit/OpenDreamKit/issues/251
Yes indeed; it will be helpful to have GAP people under hand. I may just become impatient before that for some research project of mine, in which case I'll have a head start :-)
Replying to @dimpase:
I just noticed that in 4.9 smallgrps and transgrps are released under a GPL-compatible license. This is great! We really should make them (i.e. database_gap) standard Sage package, and do away with the extra hurdle of having database_gap optional.
Yes! I can't wait for all the simplifications this will bring to us!
Replying to @kiwifb:
So if I understand the mechanics involved, at the moment you have to configure and install gap. Then clean and configure and install for libgap. So it is still behaving like "two packages".
With the past and future modifications to the Sage build system in mind, I now object more strongly than before to treating two separate packages (GAP + libGAP) as one package.
The reason is that we now try to separate the build and install stages of packages. But that is incompatible with the recipe here of first "build and install GAP" and then "build and install libGAP".
For an example of two Sage packages with the same sources, see gcc
and gfortran
. IMHO, you should do the same for GAP and libGAP (unless they are merged upstream but my impression is that this has not happened yet).
Replying to @jdemeyer:
Replying to @kiwifb:
So if I understand the mechanics involved, at the moment you have to configure and install gap. Then clean and configure and install for libgap. So it is still behaving like "two packages".
With the past and future modifications to the Sage build system in mind, I now object more strongly than before to treating two separate packages (GAP + libGAP) as one package.
The reason is that we now try to separate the build and install stages of packages. But that is incompatible with the recipe here of first "build and install GAP" and then "build and install libGAP".
Hopefully we will be getting something more serious at final release time, like with pari where you build libpari and then the gp executable. That would be the ideal scenario where it is really one package. Have to see how far they have gone with 4.9.
A lot of changes on this ticket (also the changes likely to lead to merge conflicts) are related to the unprefixing of libGAP symbols. To ease development, maybe we should try to do that separately from the upgrade to GAP 4.9.
I was thinking to do something like #19915 but changing only the Sage source code (not libGAP) and undoing the prefixing with macros like #define SomeGapFunction libGAP_SomeGapFunction
. What do you think?
there is also libgap unprefixing in Python/Cython that has to be done. Perhaps separating these from C level unprefixing would help making it smoother.
GAP 4.10 comes with a completely rewritten build system that will simplify our packaging. In particular, libGAP no longer needs to be a separate package.
What the branch does:
Remove the libgap spkg
Update the gap spkg to the new build system and build and install libgap
Replace
gap.shi.patch
by a plain gap startup script for SageRationale: GAP used to provide a startup shell script. The GAP devs are in the process of getting rid of it and provide a very minimal one. They recommend to just write our own rather than patching it.
Update a few doctests w.r.t. changes of output of some GAP functions
Reworks how gap is installed in
$SAGE_LOCAL
: Rather than installing the entire source tree we just install thegap
andlibgap
binaries to standard locations, and add a$SAGE_LOCAL/share/gap
containing theGAP_ROOT_DIR
which is stripped down to the minimum needed for GAP to work (standard libs and packages, docs, as well as the source code for introspection of kernel functions, but all build artifacts are carefully omitted).foobar
->libGAP_foobar
). This avoids messing around with GAP's sources; in particular opening the door for using a stock GAP from the OS distribution. However there always is a risk of name conflicts. And indeed, GAP's enumsT_INT
,T_FLOAT
, ... conflict with Python's constants defined instructmember.h
. This is hopefully not actually a problem in practice due to the way how Cython orders includes.Something similar was started by Volker at #19915.
Removes configure.patch: it was patching configure for better GMP detection under Cygwin (#13954). This should not be needed anymore with the new build system and use of --with-gmp. If it is, upstream asked for it to be reported and they will fix it.
Removes additional patches for now--we would like to focus on being able to work with a system GAP as much as possible.
Revert #19726 (not needed anymore)
Status
Currently broken - crashes deep inside GAP error handling system after few simple commands.
Basic tests on libgap:
Running most relevant tests:
Current status: lots of errors
Still have some miscellaneous segfaults and other weird crashes
Still have work to do on improving error handling; replacing the built-in
ErrorInner
function might help here.SIGINT handling by Python is broken by
GAP_Intialize
; need to work around this.Testing packages with dynamic loading (e.g. IO):
Install IO:
Test it locally:
This does not yet work:
This should be fixed once GAP's gap binary is built on top of libgap. See: https://github.com/markuspf/gap/issues/1 I believe this is fixed, but there are still some problems with the way this ticket is "installing" GAP for
$SAGE_LOCAL
.Note:
Upstream PRs
A few pull requests we made to improve use of GAP as a library:
Other open issues that don't have PRs yet:
Some other PRs useful to this effort:
The update of gap_packages and database_gap is on #26856
Tarball: https://www.gap-system.org/pub/gap/gap-4.10/tar.gz/gap-4.10.0.tar.gz
Depends on #26874
CC: @alex-konovalov @dimpase @embray @kiwifb @antonio-rojas @sebasguts @jpflori @sagetrac-markuspf @nthiery @slel @vbraun @williamstein @timokau
Component: interfaces
Keywords: days85, libgap
Stopgaps: error handling in libgap, documentation display
Author: Nicolas M. Thiéry, Dima Pasechnik, Erik Bray, Jeroen Demeyer
Branch:
b446ebb
Reviewer: Erik Bray, Dima Pasechnik, Jeroen Demeyer
Issue created by migration from https://trac.sagemath.org/ticket/22626