update GAP to 4.12.2 by dimpase · Pull Request #35093 · sagemath/sage

dimpase · 2023-02-12T16:35:33Z

upgrades our corresponding spkgs:

gap
gap_packages
libsemigroups
Previous upgrades:
- upgrade GAP to 4.11.1 or newer #31498: GAP 4.11.1
- update GAP to version 4.11, libsemigroup to 1.0.9 #29314: GAP 4.11, libsemigroup 1.0.9
Upstream: Completely fixed; Fix reported upstream

Closes #34391

codecov-commenter · 2023-02-12T19:51:36Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02 ⚠️

Comparison is base (f449b14) 88.62% compared to head (9990163) 88.60%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #35093      +/-   ##
===========================================
- Coverage    88.62%   88.60%   -0.02%     
===========================================
  Files         2148     2148              
  Lines       398653   398656       +3     
===========================================
- Hits        353308   353239      -69     
- Misses       45345    45417      +72

Impacted Files	Coverage Δ
src/sage/coding/linear_code.py	`81.12% <ø> (ø)`
src/sage/combinat/posets/posets.py	`93.87% <ø> (ø)`
...mbinat/root_system/hecke_algebra_representation.py	`93.11% <ø> (ø)`
src/sage/combinat/symmetric_group_algebra.py	`94.35% <ø> (ø)`
src/sage/groups/finitely_presented.py	`95.88% <ø> (ø)`
src/sage/groups/fqf_orthogonal.py	`88.48% <ø> (ø)`
src/sage/groups/matrix_gps/finitely_generated.py	`90.50% <ø> (ø)`
src/sage/groups/perm_gps/permgroup.py	`90.56% <ø> (-0.49%)`	⬇️
src/sage/tests/gap_packages.py	`94.59% <ø> (ø)`
src/sage/env.py	`88.05% <100.00%> (+0.05%)`	⬆️
... and 3 more

... and 27 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

mkoeppe · 2023-02-12T21:07:12Z

Testing at https://github.com/mkoeppe/sage/actions/runs/4158995778

tornaria · 2023-02-13T15:35:03Z

We've been shipping an earlier version of this PR in void linux since last october when we upgraded gap to 4.12.1, without any trouble. Upgrading to 4.12.2 works ok.

My patches:

For the upgrade to sagemath 9.8 I tried including the changes here and in #35094 in full, as obtained from

https://github.com/sagemath/sage/pull/35094.diff
but I get lots of segfaults...

However, if I keep instead the two patches I am using on 9.7, everything seems to work ok on 9.8.

I'll investigate further. Do you mind if I rebase #35094 on top of 9.8? I think that is independent of the gap upgrade.

tornaria · 2023-02-13T18:00:19Z

I found the issue: I was setting GAP_ROOT_DIR in my sage_conf but these variables have changed to GAP_SHARE_DIR and GAP_LIB_DIR, so gap init cannot find lib/init.g. Setting the new variables fixes the issue.

It's a big drastic to segfault because a file cannot be found... The message about not finding lib/init.g is there, but the reported segmentation fault makes the issue look much worse than it is.

I'm back to preparing a void package for 9.8 using the whole PR together with #35094 as in https://github.com/sagemath/sage/pull/35094.diff.

I will report back, this will build and check on x86_64 (glibc and musl), and on i686 (glibc) using system gap 4.12.2.

kiwifb · 2023-02-13T21:05:47Z

I decided to ship 9.8 with gap 4.12.2 in sage-on-gentoo as well. In fact I backported to sage 9.7 a few weeks ago to avoid issues with two versions of gap being available in the overlay. This upgrade has been relatively smooth compared to previous ones in gentoo.

kiwifb · 2023-02-13T22:45:01Z

Most of the errors I see on the runner so far, are unrelated to this upgrade. Did we figure what blocked it for 9.8 when Volker tried to merge it?

dimpase · 2023-02-13T22:51:35Z

Most of the errors I see on the runner so far, are unrelated to this upgrade. Did we figure what blocked it for 9.8 when Volker tried to merge it?

imho it was just a random timeout failure on a dodgy 32-bit buildbot

dimpase · 2023-02-13T22:55:09Z

Testing at https://github.com/mkoeppe/sage/actions/runs/4158995778

there is apparently a failure of the multi-stage build - any idea why, @mkoeppe ?

kiwifb · 2023-02-13T22:56:26Z

Most of the errors I see on the runner so far, are unrelated to this upgrade. Did we figure what blocked it for 9.8 when Volker tried to merge it?

imho it was just a random timeout failure on a dodgy 32-bit buildbot

Right, I'll still want to see what happens to the runner when it hits the currently queued 32bit systems but my default position is get it in ASAP.

mkoeppe · 2023-02-13T23:10:35Z

Testing at https://github.com/mkoeppe/sage/actions/runs/4158995778

there is apparently a failure of the multi-stage build - any idea why, @mkoeppe ?

Replaced by https://github.com/mkoeppe/sage/actions/runs/4167928897, which will take some time to complete

tornaria · 2023-02-14T00:00:04Z

Once I fixed the GAP_* variables, everything works, so this is 👍 from me.

Note that I am also testing 32 bit. See e.g. https://github.com/void-linux/void-packages/actions/runs/4166635040/jobs/7211215245 (there are a few unrelated failures -- because I patched sage to support singular 4.3.1p3 but I didn't upgrade singular yet).

tornaria · 2023-02-14T01:52:26Z

Note: I only tested sage-the-library. Also, I don't build optional packages (not even io, I patched atlasrep so it doesn't attempt to hit the network). I thought 2 reviews requested mean we only give positive_review when both accept.

mkoeppe

Portability tests at https://github.com/mkoeppe/sage/actions/runs/4167928897 seem OK. (Doctest failures come from other upgraded packages on https://github.com/mkoeppe/sage/tree/ci-10.0.beta0%2Bupgrades-2)

mkoeppe · 2023-02-14T20:38:27Z

I thought 2 reviews requested mean we only give positive_review when both accept.

I don't think we have rigid rules about this. If a reviewer approves and is confident to have covered all aspects of the PR, then I'd think it's OK to set positive review in addition to "approving" the PR. Here for this upgrade ticket, I guess we were waiting for the portability tests to finish (= my review), so it may have been better to leave it in needs review status.

Let's get this upgrade in.

tornaria · 2023-02-20T00:34:22Z

This needs rebasing due to #35015 in just one commit.

In case it's useful, my rebase of this PR is at 1aa1d12, and that of #35094 is at 2245773.

tornaria · 2023-02-25T20:21:26Z

ping @dimpase : can you please rebase this and #35094 on top of 10.0.beta2 so it can be merged?

I would do it, but unfortunately I don't know any way in github to change the branch that would be merged like we used to do in trac. Is there a way?

BTW, I was skeptical of the move, but in spite of all the warts github has it's a real improvement in productivity in many, many ways. How everything was moved from trac to github is amazing and the improved search on issues is already worth it. I hope the workflow will evolve over time for good as we learn our way around.

mkoeppe · 2023-02-25T20:31:24Z

I don't know any way in github to change the branch that would be merged like we used to do in trac. Is there a way?

We can promote you to "maintainer". Maintainers can make edits to other people's PRs (unless explicitly disabled when the PR is created) by pushing to the branch.

dimpase · 2023-02-25T21:55:33Z

ping @dimpase : can you please rebase this and #35094 on top of 10.0.beta2 so it can be merged?

done.

I would do it, but unfortunately I don't know any way in github to change the branch that would be merged like we used to do in trac. Is there a way?

you don't have to know if you use web interface - did you try hitting "Resolve conficts" button?
It's self-evident what to do there: edit the files with conflicts in the web editor that will pop up
(it will even tell you how many conflicts you have in each file), click "mark as resolved", click "commit"...

BTW, I was skeptical of the move, but in spite of all the warts github has it's a real improvement in productivity in many, many ways. How everything was moved from trac to github is amazing and the improved search on issues is already worth it. I hope the workflow will evolve over time for good as we learn our way around.

tornaria · 2023-02-25T22:44:43Z

ping @dimpase : can you please rebase this and #35094 on top of 10.0.beta2 so it can be merged?

done.

Thanks! I hope this can be merged soon since it's a blocker for me to use the development branch on my box (i.e. every PR I want to try, I have to merge with this branch). The other not-yet merged PR cause very localized test failures, but without this sage is unusable for me.

Also because I think changing such a core package is better done early in the release cycle so it gets to be tested thoroughly.

you don't have to know if you use web interface - did you try hitting "Resolve conficts" button? It's self-evident what to do there: edit the files with conflicts in the web editor that will pop up (it will even tell you how many conflicts you have in each file), click "mark as resolved", click "commit"...

I know how to resolve conflicts on the CLI, and I guess I would know how to do it on GH web. But the point is that:

dimpase wants to merge 24 commits into sagemath:develop from dimpase:u/dimpase/packages/gap/4_12_2

On the one hand I cannot change the branch dimpase:u/dimpase/packages/gap/4_12_2 since it's yours. This is the same as in trac. On the other hand, I cannot change the PR so that it "'wants to merge" a different branch (say, under my control). This was possible on trac (we had the "Branch:" field and we could change it).

I tried doing a PR against your branch in your repo as the documentation somewhere suggests, but I don't know if I did that correctly.

dimpase · 2023-02-26T00:20:48Z

I tried doing a PR against your branch in your repo as the documentation somewhere suggests, but I don't know if I did that correctly.

you did these PRs, they are there - but for some reason I didn't see notifications, sorry - it's not that I deliberately ignored them.

actually, I just checked, and indeed, notificaions for PRs on my repo were off for me. Perhaps it's the GitHub's default.
We need to mention this in the docs, that on Sage's forks PRs should be enabled and notifications for them, too. @mkoeppe

adjust doctests

also add a workaround for Semigroups (see comment in the code)

`make install` installs files in both paths

The 'packagemanager' package does not load without it

dimpase · 2023-03-17T17:01:45Z

Could you try patching gap with atlasrep-dont_use_network_by_default.patch which changes the default for atlasrep to not download data files? The actual computations don't take long (at least the ones done by sage doctests).

OK, done. @vbraun - could you please check on the bots if it helps.

github-actions · 2023-03-17T18:53:45Z

Documentation preview for this PR is ready! 🎉
Built with commit: 9990163

vbraun · 2023-03-17T21:30:54Z

Yes the buildbots poison the http_proxy environment variables to prevent unwanted network access

tornaria · 2023-03-17T21:37:05Z

Yes the buildbots poison the http_proxy environment variables to prevent unwanted network access

How? So I can try to reproduce this locally.

vbraun · 2023-03-18T10:31:03Z

I'm setting (python syntax)

        offline_env['http_proxy'] = 'http://192.0.2.0:5187/'
        offline_env['https_proxy'] = 'http://192.0.2.0:5187/'
        offline_env['ftp_proxy'] = 'http://192.0.2.0:5187/'
        offline_env['rsync_proxy'] = 'http://192.0.2.0:5187/'

…uld look for libgap.so* ### 📚 Description In short, sometimes there is no `lib**.so` available, but only `lib**.so.*`. This breaks GAP and Singular related things. See #33446 for details. Closes #33446 ### ⌛ Dependencies Depends on #35093 #34391 (GAP update to 4.12.2) URL: #35094 Reported by: Dima Pasechnik Reviewer(s): Matthias Köppe

gh-35114: libgap: Remove some GC hazards ### 📚 Description Trac branch `u/gh-collares/gap-gc` from #34701, now migrated to GitHub. Currently based atop #35093; will rebase once that is merged. The rest of the description below is copied from #34701: A refactor in #27946 introduced "unprotected" (not surrounded by `GAP_Enter`/`GAP_Leave`) `GAP_ValueGlobalVariable` calls. I believe this might be a GC hazard, because after updating to GAP 4.12.1 I started seeing aarch64 crashes on NixOS infrastructure such as: ``` #0 0x0000fffff79740e8 in wait4 () #1 0x0000fffff5dc6b78 in print_enhanced_backtrace () #2 0x0000fffff5dc8190 in sigdie () #3 0x0000fffff5dcb1c0 in cysigs_signal_handler () #4 0x0000fffff7ffb7cc in __kernel_rt_sigreturn () #5 0x0000ffff99a0bf28 in ConvString () #6 0x0000000000000000 in ?? () #7 0x0000000000000000 in ?? () #8 0x0000000000000000 in ?? () #9 0x0000ffff99989930 in Pr () #10 0x0000ffff9998aa18 in CloseOutput () #11 0x0000ffff99884828 in capture_stdout () at /build/sage- src-9.7/pkgs/sagemath-standard/sage/libs/gap/element.pyx:154 ... ``` I also see cases where `capture_stdout` throws errors such as `sage.libs.gap.util.GAPError: Error, Length: <list> must be a list (not the integer 255)` and then crashes. Both types of errors are fixed by this ticket. Note that I am nesting `GAP_Enter`/`GAP_Leave` calls because I didn't remove the preexisting calls inside `capture_stdout`. That's because I feared removing the innermost calls might create a new footgun (and I believe nested `GAP_Enter`/`GAP_Leave` calls are explicitly supported), but removing them should cause no problem. Removing them might even be preferable for performance reasons, I don't know. Fixes #34701 ### 📝 Checklist    - [x] I have made sure that the title is self-explanatory and the description concisely explains the PR. - [x] I have linked an issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   - #35093: GAP 4.12.2 upgrade, which touches the same function and should land first. URL: #35114 Reported by: Mauricio Collares Reviewer(s): Dima Pasechnik

dimpase · 2023-04-18T11:59:52Z

Could you try patching gap with atlasrep-dont_use_network_by_default.patch which changes the default for atlasrep to not download data files? The actual computations don't take long (at least the ones done by sage doctests).

just in case, to revert the effect of this patch at the start of Sage session, one can do

libgap.SetUserPreference("AtlasRep", "AtlasRepAccessRemoteFiles", true )

this has to happen at the startup - after AtlasRep is loaded, it has no effect.

dimpase mentioned this pull request Feb 12, 2023

To determine GAP_SO sage.env looks for libgap.so but it should look for libgap.so* #35094

Merged

dimpase added the s: needs review label Feb 12, 2023

dimpase requested review from mkoeppe and tornaria February 12, 2023 20:42

collares mentioned this pull request Feb 13, 2023

libgap: Remove some GC hazards #35114

Merged

4 tasks

tornaria approved these changes Feb 14, 2023

View reviewed changes

dimpase added s: positive review and removed s: needs review labels Feb 14, 2023

mkoeppe approved these changes Feb 14, 2023

View reviewed changes

This was referenced Feb 20, 2023

Rebase gap PR #35093 on top of 10.0.beta1 dimpase/sage#1

Closed

Rebase gap PR #35094 on top of #35093 dimpase/sage#2

Closed

dimpase and others added 16 commits March 17, 2023 16:40

upgrade libsemigroup and gap_packages

21babff

adjust doctests

add GAP upstream PR 5077

1865dd3

also, fix the package names etc

9b1798a

update GAP to 4.12.1

23b6e3f

install GAP package singular - new dependency of liepring

7cb0473

moved install of radiroot to gap from gap_packages

566625d

moved install of autodoc and utils to gap from gap_packages

cecae60

don't use SAGE_LOCAL/share/gap, switch to GAP's "make install"

bde3df7

also add a workaround for Semigroups (see comment in the code)

Search for files in both lib/gap and share/gap

5e3c4ec

`make install` installs files in both paths

Create lib/gap/bin on install

494eaeb

The 'packagemanager' package does not load without it

ensure make gap-clean still works

038a4c7

with SAGE_CHECK=yes, one needs io installed to prevent hangs

d0e122f

backport GAP PR 5235

8db52f7

update GAP to 4.12.2

ab138e1

bump libsemigroups to v2.3.2

411450a

tell atlastrep do not use internet by default

9990163

dimpase force-pushed the u/dimpase/packages/gap/4_12_2 branch from d22bdee to 9990163 Compare March 17, 2023 17:00

vbraun added s: positive review and removed s: needs work labels Mar 17, 2023

vbraun merged commit b0728b0 into sagemath:develop Mar 19, 2023

This was referenced Mar 22, 2023

gap v4.12.2 conda-forge/gap-feedstock#68

Merged

Add minimum version of conda gap package #35351

Merged

Uh oh!

Conversation

dimpase commented Feb 12, 2023 • edited by mkoeppe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Feb 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mkoeppe commented Feb 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tornaria commented Feb 13, 2023

Uh oh!

tornaria commented Feb 13, 2023

Uh oh!

kiwifb commented Feb 13, 2023

Uh oh!

kiwifb commented Feb 13, 2023

Uh oh!

dimpase commented Feb 13, 2023

Uh oh!

dimpase commented Feb 13, 2023

Uh oh!

kiwifb commented Feb 13, 2023

Uh oh!

mkoeppe commented Feb 13, 2023

Uh oh!

tornaria commented Feb 14, 2023

Uh oh!

tornaria commented Feb 14, 2023

Uh oh!

mkoeppe left a comment

Choose a reason for hiding this comment

Uh oh!

mkoeppe commented Feb 14, 2023

Uh oh!

tornaria commented Feb 20, 2023

Uh oh!

tornaria commented Feb 25, 2023

Uh oh!

mkoeppe commented Feb 25, 2023

Uh oh!

dimpase commented Feb 25, 2023

Uh oh!

tornaria commented Feb 25, 2023

Uh oh!

dimpase commented Feb 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimpase commented Mar 17, 2023

Uh oh!

github-actions bot commented Mar 17, 2023

Uh oh!

vbraun commented Mar 17, 2023

Uh oh!

tornaria commented Mar 17, 2023

Uh oh!

vbraun commented Mar 18, 2023

Uh oh!

dimpase commented Apr 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dimpase commented Feb 12, 2023 •

edited by mkoeppe

Loading

codecov-commenter commented Feb 12, 2023 •

edited

Loading

mkoeppe commented Feb 12, 2023 •

edited

Loading

dimpase commented Feb 26, 2023 •

edited

Loading