gap: support gap 4.13 in sagelib (does NOT touch sage-the-distro) #37884
Conversation
tornaria
requested review from
enriqueartal,
miguelmarco,
tscrim and
antonio-rojas
|
Documentation preview for this PR (built with commit c49562e; changes) is ready! 🎉 |
|
The failures here are real: and the reason why this PR can't be merged if gap is not updated to 4.13. But really, either output (the one with gap 4.12 or the one with 4.13) is just fine. Hence, updating gap to 4.13 in sage-the-distro and apply this PR at the same time, is a pain for using system gap (either we restrict the version of gap that is allowed from system, or we end up with failing doctests on some systems). We really need to come up with a system to be able to support different versions of dependencies in doctests... at this time we have a similar problem with the difference between singular 4.3 and 4.4 (which we worked around with an ugly hack of patching the output of singular when running via doctests!). |
|
We had specific tags for Python 2 and 3 differences. Since there are only a few differences (some of which are really not easy to rectify by changing the doctests), one option might be to introduce such tags for GAP versions. Or we just tell people that some tests might not pass for trivial reasons if you do not have a sufficiently updated version of GAP... |
Here's one idea to think about:
A quick example with one test (very crude first try): In The traslation file for gap has something like: I'm not sure if the doctest label has to include the gap version or even the program name (maybe just have a common tag to mark the test to be applied this mechanism). |
|
Here's another (maybe simpler?) idea. See src/sage/arith/long.pxd where we have this doctest: so the output is labeled here... If we could have labels e.g "gap < 4.13" and "gap >= 4.13" or something like this... Would that we workable? |
In gap 4.13 there are some improvements, e.g. converting fp groups to permutation groups, computing abelianization of fp groups, which lead to different generators. This commit fixes doctests so they pass using gap 4.13.
|
Sorry for loosing track of this. Yes, I think having something as a tag on the output with, e.g., |
|
Or, for simplicity, how about we just update gap to 4.13 and require the same version from the system. RIght now, only >= 4.12.2 is accepted, but there's not really anything relevant that actually has 4.12.2 but not 4.13 - https://repology.org/project/gap/versions |
|
I've opened #38169 for the update, based on this PR. |
|
Let's close this in favor of #38169 |
<!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> - Based on sagemath#37884 by @tornaria - Fixes sagemath#37616 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> - Depends on sagemath#38144 (merged here for testing) URL: sagemath#38169 Reported by: Matthias Köppe Reviewer(s):
|
Then perhaps for the upcoming release we should reject system gap 4.13 |
Happening in #38222 |
|
I don't see any problem with system gap 4.13.0, but nobody here has given any concrete problem. If you have an issue, please document it properly. and: |
|
In a virtual machine with fedora 40 and gap 4.13.1 installed I got plenty of errors, some of them for change of order in some outputs, but many timed out. This is the I do not know if they may have other origins. |
|
yes, that's roughly what I see with GAP 4.13, too. tips on how to debug this are most welcome |
I build gap 4.13 straight from source, no relevant patch. Maybe your gap 4.13 package is broken? Do they run gap tests on build? The log above doesn't show what is failing. Can you actually show the code that is failing / timing out, and track back to a failure in gap 4.13? Then we can compare a "good" 4.13 (mine) with a "bad" 4.13 (yours). And maybe test for "bad" gap by failure, not by version. Because why would sage force me to build gap if mine is working properly? Next thing we know, sage is shipping gcc because some os has a broken toolchain... /s |
|
GAP 4.13.1 causes two additional failures: The first one is the usual change of generators, but the second one looks broken |
|
test22062024.log |
|
I have reinstalled gap (with dnf) recompiled from source (from src rpm) and recompile sage. I have tested |
This is the |
|
Sorry for the newbie question. I compiled gap from the source and installed it in |
there is more info in the top-level config.log |
I attach the whole config23062024.log but maybe the relevant lines are: I missed them. |
|
Hopefully solved. Missed non recommended |
In gap 4.13 there are some improvements, e.g. converting fp groups to permutation groups, computing abelianization of fp groups, which lead to different generators.
This PR fixes doctests so they pass using gap 4.13.
I don't see a way to have this work with gap 4.12.2 at the same time, so this is not in principle ready to merge.
📝 Checklist
⌛ Dependencies
#37883: don't use deprecated
LaTeX()gap function.