search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.38k stars 469 forks source link

Upgrade: GAP 4.12 #34391

Closed slel closed 1 year ago

slel commented 2 years ago

GAP 4.12.1 fixes critical bugs in 4.12.0

GAP 4.12.1 release page

GAP 4.12.0 was released in August 2022.

We should upgrade our corresponding spkgs:

Previous upgrades:

Upstream: Completely fixed; Fix reported upstream

CC: @antonio-rojas @dimpase @slel @tornaria @kiwifb @collares @tscrim

Component: packages: standard

Keywords: spkg, upgrade, gap, libsemigroups

Author: Antonio Rojas, Dima Pasechnik

Reviewer: Dima Pasechnik, Antonio Rojas, Mauricio Collares, François Bissey

Issue created by migration from https://trac.sagemath.org/ticket/34391

collares commented 1 year ago
comment:42

Max Horn also pointed out that radiroot is now a dependency of polenta, one of the autoloaded packages. Also, although not strictly necessary, atlasrep uses io as the preferred method for fetching files over the network (but it will fall back to running wget if that's not available).

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

Branch pushed to git repo; I updated commit sha1. New commits:

84a527dmoved install of radiroot to gap from gap_packages
eb8cd42moved install of autodoc and utils to gap from gap_packages
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

Changed commit from 02b6b4b to eb8cd42

dimpase commented 1 year ago
comment:44

done what's requested in comment:41 and comment:42, please review.

collares commented 1 year ago
comment:45

Replying to Antonio Rojas:

Feel free to push your changes, I can only test on x86_64 where I haven't seen any such issue so far.

I think I will submit my changes in a separate ticket, since the present update seems ready and does not make things any worse on aarch64 as far as I can tell. What is the best way to submit a ticket/change which depends on an open one and touches the same lines of code? I think I will just branch off this one and mark the ticket as a dependency.

Edit: Opened #34701, sorry for the noise!

dimpase commented 1 year ago
comment:46

Replying to Mauricio Collares:

What is the best way to submit a ticket/change which depends on an open one and touches the same lines of code? I think I will just branch off this one and mark the ticket as a dependency.

yes, that's how this is normally done.

collares commented 1 year ago
comment:47

Forwarding another suggestion from Max Horn (in https://github.com/NixOS/nixpkgs/pull/192548#discussion_r989552690): config.h is not meant to be installed anymore. The corresponding line in spkg-install.in can just be removed, I think.

antonio-rojas commented 1 year ago
comment:48

Replying to Mauricio Collares:

Forwarding another suggestion from Max Horn (in https://github.com/NixOS/nixpkgs/pull/192548#discussion_r989552690): config.h is not meant to be installed anymore. The corresponding line in spkg-install.in can just be removed, I think.

This will solve itself once we switch to make install. Let's get this is as is and move that to a follow-up ticket (there are some non-trivial adjustments needed for gap-packages, and probably the GAP_ROOT variable should be rethought or removed completely)

dimpase commented 1 year ago
comment:49

In #34711 we will switch to using GAP's make install. Meanwhile I'm giving this a positive review.

dimpase commented 1 year ago

Reviewer: Dima Pasechnik, Antonio Rojas, Mauricio Collares

dimpase commented 1 year ago
comment:50

I spoke too soon - tested with unclean SAGE_LOCAL :-(

With everything clean, while building gap_packages, I get

gcc -g -O2 -fPIC -MQ gen/src/crypting.o -MMD -MP -MF gen/src/crypting.d -g -O2 -Wno-implicit-function-declaration -Wno-implicit-function-declaration -o gen/src/crypting.o -I/home/scratch/scratch2/dimpase/sage/sagetrac-mirror/local/share/gap/build -I/home/scratch/scratch2/dimpase/sage/sagetrac-mirror/local/share/gap/src -I/home/scratch/scratch2/dimpase/sage/sagetrac-mirror/local/share/gap -DUSE_GASMAN=1 -c src/crypting.c
In file included from /home/scratch/scratch2/dimpase/sage/sagetrac-mirror/local/share/gap/src/gap_all.h:51,
                 from /home/scratch/scratch2/dimpase/sage/sagetrac-mirror/local/share/gap/src/compiled.h:26,
                 from src/crypting.c:5:
/home/scratch/scratch2/dimpase/sage/sagetrac-mirror/local/share/gap/src/modules.h:19:10: fatal error: version.h: No such file or directory
   19 | #include "version.h"
      |          ^~~~~~~~~~~
compilation terminated.

The problem is that version.h gets installed into SAGE_LOCAL/include/gap - a location unknown to crypting package makefile.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

061b6fbMark test as random. With gap 4.12 on x86_64 it is no longer 2
a5a4423Remove test that is now redundant, all seeds give the same answer
af5f91binstall gap 4.12.0
46ef5f8upgrade libsemigroup and gap_packages
d57f502add GAP upstream PR 5077
34b03edalso, fix the package names etc
60ac451update GAP to 4.12.1
412fec4install GAP package singular - new dependency of liepring
f3ce16dmoved install of radiroot to gap from gap_packages
a1e6e43moved install of autodoc and utils to gap from gap_packages
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

Changed commit from eb8cd42 to a1e6e43

dimpase commented 1 year ago
comment:52

rebased over latest beta, 9.8.beta3

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

Changed commit from a1e6e43 to 72d57e0

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

Branch pushed to git repo; I updated commit sha1. New commits:

72d57e0don't use SAGE_LOCAL/share/gap, switch to GAP's "make install"
dimpase commented 1 year ago
comment:54

the current issue is to fix gap's spkg-check - currently broken due to a mess in some GAP's packages we install, see https://github.com/gap-packages/crypting/issues/21#issuecomment-1303503670

antonio-rojas commented 1 year ago
comment:55

libgap doesn't find any package

sage: from sage.tests.gap_packages import all_installed_packages
sage: all_installed_packages()
()
antonio-rojas commented 1 year ago

Changed branch from u/dimpase/packages/gap/4_12_1 to u/arojas/packages/gap/4_12_1

antonio-rojas commented 1 year ago

Changed commit from 72d57e0 to 16c2ccb

antonio-rojas commented 1 year ago
comment:57

Ideally libgap should know where to look for its files without needing to pass the paths with -l, but apparently that's not the case (it works fine in the gap command line though, GAPInfo.RootPaths returns both paths, lib and share).

New commits:

16c2ccbSearch for files in both lib/gap and share/gap
antonio-rojas commented 1 year ago
comment:58

There's still one test failure

sage -t --long --random-seed=259290265121646386777269003923470404522 src/sage/tests/gap_packages.py
**********************************************************************
File "src/sage/tests/gap_packages.py", line 8, in sage.tests.gap_packages
Failed example:
    test_packages(pkgs, only_failures=True)    # optional - gap_packages
Exception raised:
    Traceback (most recent call last):
      File "/home/antonio/Software/sage/sage-gap/src/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/antonio/Software/sage/sage-gap/src/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.tests.gap_packages[2]>", line 1, in <module>
        test_packages(pkgs, only_failures=True)    # optional - gap_packages
      File "/home/antonio/Software/sage/sage-gap/src/sage/tests/gap_packages.py", line 71, in test_packages
        output = libgap.LoadPackage(pkg)
      File "sage/libs/gap/element.pyx", line 2524, in sage.libs.gap.element.GapElement_Function.__call__
        sig_on()
    sage.libs.gap.util.GAPError: Error, no method found! Error, no 1st choice method found for `Filename' on 2 arguments
    The 1st argument is 'fail' which might point to an earlier problem
    Error, was not in any namespace
    Error, was not in any namespace
**********************************************************************

It happens when libgap tries to load the packagemanager package

sage: libgap.LoadPackage("packagemanager")
---------------------------------------------------------------------------
GAPError                                  Traceback (most recent call last)
Input In [1], in <cell line: 1>()
----> 1 libgap.LoadPackage("packagemanager")

File ~/Software/sage/sage-gap/src/sage/libs/gap/element.pyx:2524, in sage.libs.gap.element.GapElement_Function.__call__()
   2522 try:
   2523     sig_GAP_Enter()
-> 2524     sig_on()
   2525     if n == 0:
   2526         result = CALL_0ARGS(self.value)

GAPError: Error, no method found! Error, no 1st choice method found for `Filename' on 2 arguments
The 1st argument is 'fail' which might point to an earlier problem
Error, was not in any namespace
Error, was not in any namespace

It (seems to) work fine the second time

sage: libgap.LoadPackage("packagemanager")
true
dimpase commented 1 year ago
comment:59

Do you need to look for packages in share/gap? There should not be any. That's why I doubt your line from ​16c2ccb:

s = str_to_bytes(sage.env.GAP_LIB_DIR + ";" + sage.env.GAP_SHARE_DIR, FS_ENCODING, "surrogateescape")
antonio-rojas commented 1 year ago
comment:60

Replying to Dima Pasechnik:

Do you need to look for packages in share/gap? There should not be any. That's why I doubt your line from ​16c2ccb:

That is not only for packages. Without this, libgap can't find the lib/init.g file which is installed under share/gap and just crashes on initialization.

antonio-rojas commented 1 year ago
comment:61

Replying to Antonio Rojas:

It happens when libgap tries to load the packagemanager package

Looks like packagemanager expects a lib/gap/bin dir to exist, which is not created by make install

https://github.com/gap-packages/PackageManager/blob/master/gap/PackageManager.gd#L294

Simply creating an empty bin subdir fixes the test.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

Changed commit from 16c2ccb to 3abfbd0

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

Branch pushed to git repo; I updated commit sha1. New commits:

3abfbd0Create lib/gap/bin on install
antonio-rojas commented 1 year ago
comment:63

Reported upstream

https://github.com/gap-packages/PackageManager/issues/105

dimpase commented 1 year ago
comment:64

Something is fishy with PackageManager in another way, too: https://github.com/gap-packages/PackageManager/issues/106

dimpase commented 1 year ago
comment:65

Replying to @sagetrac-git:

Branch pushed to git repo; I updated commit sha1. New commits:

3abfbd0Create lib/gap/bin on install

but we don't need lib/gap/bin, it's an upstream bug.

antonio-rojas commented 1 year ago
comment:66

Replying to Dima Pasechnik:

Replying to @sagetrac-git:

Branch pushed to git repo; I updated commit sha1. New commits:
3abfbd0 Create lib/gap/bin on install

but we don't need lib/gap/bin, it's an upstream bug.

It breaks a test. We either need to work around this, or we need to stop testing loading the package.

kiwifb commented 1 year ago
comment:67

Replying to Antonio Rojas:

Replying to Dima Pasechnik:

Do you need to look for packages in share/gap? There should not be any. That's why I doubt your line from ​16c2ccb:

That is not only for packages. Without this, libgap can't find the lib/init.g file which is installed under share/gap and just crashes on initialization.

I feel like it is an oversight upstream. They should move that code under lib/gap too to be consistent. It is probably just a matter of fixing the install target in the makefile.

antonio-rojas commented 1 year ago
comment:68

Replying to François Bissey:

I feel like it is an oversight upstream. They should move that code under lib/gap too to be consistent. It is probably just a matter of fixing the install target in the makefile.

Technically it is the correct place. Arch-independent files belong in share.

kiwifb commented 1 year ago
comment:69

Replying to Antonio Rojas:

Replying to François Bissey:

I feel like it is an oversight upstream. They should move that code under lib/gap too to be consistent. It is probably just a matter of fixing the install target in the makefile.

Technically it is the correct place. Arch-independent files belong in share.

I cannot argue with that. I know I had complaint that in previous gap version, gap packages installed binaries and libraries under share which is not right of course. But gap packages are a mixed bag. Do we need to sort them out or to make sure only the binary bits get shipped under lib? It may become complicated very fast. I feel like the right decision is just to put everything under lib. Python does it, that's a big example to follow.

antonio-rojas commented 1 year ago
comment:70

Actually, the bin/gap script created by make install contains both paths

https://github.com/gap-system/gap/blob/v4.12.1/Makefile.rules#L582

My commit is just replicating that for the libgap call in sage.

Do we need to sort them out or to make sure only the binary bits get shipped under lib? It may become complicated very fast. I feel like the right decision is just to put everything under lib.

This is something for upstream to figure out when they integrate package installation in make install, which I believe is planned. Until then every distro will manually install packages where they choose to, so we need to search both paths.

kiwifb commented 1 year ago
comment:71

Replying to Antonio Rojas:

Actually, the bin/gap script created by make install contains both paths

https://github.com/gap-system/gap/blob/v4.12.1/Makefile.rules#L582

That, I noticed when I packaged 4.12.0. I only thought of it as a backward compatible move though. It didn't quite percolate into my brain that it would be needed for the stuff that ship with gap itself.

dimpase commented 1 year ago
comment:72

another issue is that make gap-clean does not seem to work.

dimpase commented 1 year ago
comment:73

And installing GAP with SAGE_CHECK=yes hangs for me at gap-4.12.1/src/tst/testinstall/grp/basic.tst

collares commented 1 year ago
comment:74

Does it hang if you disable your internet connection? atlasgrp fetches precomputed data from the network (using either the io GAP package or the wget binary) if available, so connection failures/slowness may cause hangs occasionally. It falls back to local computation if there's no internet access.

tornaria commented 1 year ago
comment:75

Replying to Mauricio Collares:

Does it hang if you disable your internet connection? atlasgrp fetches precomputed data from the network (using either the io GAP package or the wget binary) if available, so connection failures/slowness may cause hangs occasionally. It falls back to local computation if there's no internet access.

This might be useful: https://github.com/void-linux/void-packages/blob/master/srcpkgs/gap/patches/atlasrep-dont_use_network_by_default.patch

mkoeppe commented 1 year ago
comment:76

Replying to Dima Pasechnik:

another issue is that make gap-clean does not seem to work.

The install script does not use DESTDIR staging (sdh_make install installs directly into the prefix)

dimpase commented 1 year ago
comment:77

Replying to Matthias Köppe:

Replying to Dima Pasechnik:

another issue is that make gap-clean does not seem to work.

The install script does not use DESTDIR staging (sdh_make install installs directly into the prefix)

hmm, is this a GAP bug then? Or a Sage bug?

mkoeppe commented 1 year ago
comment:78

Try changing sdh_make install to sdh_make_install.

dimpase commented 1 year ago

Description changed:

--- 
+++ 
@@ -1,3 +1,8 @@
+GAP 4.12.1 fixes critical bugs in 4.12.0
+
+[GAP 4.12.1 release page](https://www.gap-system.org/Releases/4.12.1.html)
+
+
 GAP 4.12.0 was released in August 2022.

 - [GAP 4.12.0 release announcement](https://lists.uni-kl.de/gap/arc/forum/2022-08/msg00031.html)
dimpase commented 1 year ago
comment:80

Replying to Matthias Köppe:

Try changing sdh_make install to sdh_make_install.

right, thanks - this worked after I removed build/pkgs/gap/spkg-prerm.in as well. I'll post a new branch.

dimpase commented 1 year ago

Changed branch from u/arojas/packages/gap/4_12_1 to u/dimpase/packages/gap/4_12_1

dimpase commented 1 year ago

Changed commit from 3abfbd0 to df6d32e

dimpase commented 1 year ago

New commits:

df6d32eensure make gap-clean still works
dimpase commented 1 year ago
comment:82

With gap_packages, cleaning is still not 100% right. Namely, compiled GAP packages, e.g. cohomolo, leave a skeleton directory after make gap_packages-clean. It's harmless in this particular case though.

Let's leave this to another ticket.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 1 year ago

Branch pushed to git repo; I updated commit sha1. New commits:

97dc1eawith SAGE_CHECK=yes, one needs io installed to prevent hangs