GAP 4.13.1 #38804
Merged
GAP 4.13.1 #38804
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
orlitzky
requested review from
kiwifb,
dimpase,
tornaria,
antonio-rojas and
enriqueartal
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.
…ests Co-authored-by: Max Horn <max@quendi.de>
…ose test suite is disabled
…ced to dummy from list of packages whose test suite is disabled
There are a few simplicial set tests that require the polenta package with gap-4.13.x. We declare a new feature to support the corresponding "needs" tag.
…ackage Three tests in this file need the GAP package "polenta" to be installed; otherwise, the output (although perhaps not _wrong_) is not quite what we expect.
Fixes gap-system/gap#5796 This is the patch I'm using on Gentoo, it combines the upstream fix and one of its dependencies (without which the desired fix does not apply). Without it, you'll get some test failures with gap-4.13.x when GRAPE happens to be installed.
I've left the upper bound in there, but set to "5.0.0" which should have no real effect. But if 4.14.0 comes out and breaks somes tests, it's a lot easier to tweak the existing upper bound than it is to add one (again) and reindent everything.
|
Locally, I've still got the The CI is unhappy about the upgrade path, but I'm not sure why exactly. This is the failure... it looks like sagelib wasn't rebuilt after GAP was upgraded. The |
|
I tried to remove Tomorrow I'll test the SPKG upgrade path myself. |
It's not proprietary: you can either attach your runner to GitHub CI, or run tests fully locally: https://nektosact.com/ (you'd need Docker and go installed, though) |
|
For what it is worth in a test installation of Fedora 40, with this branch using spkg gap and installing gap_packages and sirocco, no gap-related test fails. |
|
|
|
If I understand correctly, in several local installation the branch is successful, but not in CI, because some files are not rebuilt. What can be done to make possible a positive review? |
|
It's indeed not ideal but the PR is already in Volker's release branch, so I'll try to prepare the necessary conda updates and mark them as ci-fix as soon as the new beta is out. |
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Oct 23, 2024
Follow-up to: * sagemath#37884 * sagemath#38169 With the four additional work items I mentioned in a comment on the latter: 1. Everything has been rebased 2. There's a new feature to detect the polenta GAP package 3. The three failing simplicial sets tests have been marked `# needs gap_package_polenta` 4. I backported gap-system/gap#5796 to the GAP spkg so that the optional GRAPE tests will pass (untested). Let's see what the CI has to say... URL: sagemath#38804 Reported by: Michael Orlitzky Reviewer(s): Dima Pasechnik, Enrique Manuel Artal Bartolo
|
This has completely missed the needed for gap_packages bump to libsemigroups - they should be done in lockstep. @orlitzky |
This was referenced Oct 28, 2024
Sorry, it simply never occurred to me. |
|
I've opened a PR to upgrade - see #38875 |
2 tasks
|
Unfortunately, the gap_packages-4.13.1 spkg does not build with XCode 16 on Intel macOS Sequoia 15.0 using the libsemigroups package added in PR #38875. I tried with both XCode 16.0 and 16.1 (released 2 days ago). The build works with XCode 16.1 on macOS 15.1 with the M1 arm cpu. The log file is attached. The relevant part is: |
|
This is certainly an upstream issue. Please report it to By the wat, gap 4.14 is due to be released soon. |
|
I did some experiments to see what is going on here. It turns out that the -no-avx flag causes the static assertion to fail. If that flag is removed from the g++ command then the static assertion succeeds. The flag is there because we were trying to support all Intel CPUs that are compatible with macOS 10.12. Essentially we are targeting the Core-2 Duo chip, which did not support avx. Maybe I will declare that SageMath-10-5.app requires at least a Sandy Bridge CPU. I doubt that libsemigroups wants to do any extra work in order to support older CPUs than Sandy Bridge. That means Mac models released before 2010. |
|
My old core2 duo macAir stopped being useful 5 or more years ago. I suppose we can drop supporting these configurations in Sage in general |
|
after all, macOS 12 has reached EOL last month. |
|
I am talking about macOS 10.12, which is 9 years old, not macOS 12 which is
only 3 years old. I am certainly not dropping support for 3 year old
operating systems. But it isn't the macOS version that matters, it is the
Intel CPU model.
How long we will support Intel is another question.
…On Wed, Oct 30, 2024 at 5:10 PM Dima Pasechnik ***@***.***> wrote:
after all, macOS 12 has reached EOL last month.
Let's drop support for it.
https://endoflife.date/macos
—
Reply to this email directly, view it on GitHub
<#38804 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ6CP25J353R35NMMA7DDLZ6FKO3AVCNFSM6AAAAABPZ465JKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBYGU2DMMJYHE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
OK, sorry, I messed up with digits here... |
|
As I said, the OS version is irrelevant. It is the CPU model that
matters. For SageMath-10-5.app I am setting the target OS version to 10.12
and disabling avx2 and bmi2 but not avx. I think that makes sense. As far
as I know there is no issue with that. There is no point in setting the
target OS version higher than necessary. It does not cost anything to set
it at the oldest version that builds. And that is also the right thing to
do.
Dropping support for macOS 10 or 11 would be completely counter-productive
in my opinion, saving us nothing but definitely cutting off some users who
have machines which are updated to the newest macOS version that runs on
their hardware.
Also, this is not an issue for Sage developers. It only matters for people
who are making binary Sage packages.
…On Wed, Oct 30, 2024 at 5:55 PM Dima Pasechnik ***@***.***> wrote:
I am talking about macOS 10.12, which is 9 years old, not macOS 12 which is
only 3 years old. I am certainly not dropping support for 3 year old
operating systems.
OK, I messed up with digita here...
How about macOS 10?
And 11 too? @vbraun <https://github.com/vbraun> - any opinion on this?
https://endoflife.date/macos
—
Reply to this email directly, view it on GitHub
<#38804 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ6CPYDIPLTWXJCE6OUZOLZ6FPUNAVCNFSM6AAAAABPZ465JKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBYGYYTKNZQGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Afaik the oldest hardware that Apple supports is a 2017 iMac (running Ventura). Thats 7-th Gen Intel (Kaby Lake). That definitely runs AVX. We shouldn't try to build new versions for hardware that the manufacturer abandoned. |
|
On Wed, Oct 30, 2024 at 7:05 PM Volker Braun ***@***.***> wrote:
Afaik the oldest hardware that Apple supports is a 2017 iMac (running
Ventura). Thats 7-th Gen Intel (Kaby Lake). That definitely runs AVX. We
shouldn't try to build new versions for hardware that the manufacturer
abandoned.
Nobody is proposing to build any new versions of anything. Certainly I am
not. I just said that I plan to set my OS target version and the relevant
compiler flags to match the oldest OS version and oldest CPU for which
everything builds. That seems like a no-brainer, and it is just a matter
of how I set the CFLAGS in my build script; It demands nothing of anyone
else. I think we can stop wasting our time on this discussion, please.
- Marc
… —
Reply to this email directly, view it on GitHub
<#38804 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ6CP25I7CSPNVZCBAN4OTZ6FX3TAVCNFSM6AAAAABPZ465JKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBYG4YDGMJQGY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
This discussion comes from the fact you stumbled upon a macOS specific issue, which originated from trying to support museum-grade hardware. Which I would classify as a Sage bug. Thanks for digging it up. If Sage didn't support the aforementioned in the first place, we would not be having this discussion in the first place. Now we have uncovered this bug, and we shall proceed to fix it. Right, @vbraun ? |
|
There is no Sage bug involved. I removed the -no-avx flag from my CFLAGS
and everything built with no issue, including libsemigroups-2.7.3 and
gap_packages-4.13.1. We learned that those two packages don't build
correctly if you set the -no-avx flag. Removing that flag means that a
small number of very old macs will not be supported by SageMath-10-5.app,
and that is fine with me. Nothing else needs to be done by me and nothing
at all needs to be done by Sage.
I think it was appropriate for me to report the error that I saw, before I
understood its cause. Once I figured out what the cause was I resolved the
issue by myself. That should be the end of this story.
I will be blunt about this: *This discussion is over.*
…On Wed, Oct 30, 2024 at 8:16 PM Dima Pasechnik ***@***.***> wrote:
This discussion comes from the fact you stumbled upon a macOS specific
issue, which originated from trying to support museum-grade hardware. Which
I would classify as a Sage bug. Thanks for digging it up.
If Sage didn't support the aforementioned in the first place, we would not
be having this discussion in the first place. Now we have uncovered this
bug, and we shall proceed to fix it. Right, @vbraun
<https://github.com/vbraun> ?
—
Reply to this email directly, view it on GitHub
<#38804 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ6CPZOLLEI3N4B7RDXB6LZ6GAHVAVCNFSM6AAAAABPZ465JKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBYHAYDOMRWG4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Nov 5, 2024
necessary for GAP 4.13.1 This was forgotten in sagemath#38804 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ x] The title is concise and informative. - [ x] 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: ... --> URL: sagemath#38875 Reported by: Dima Pasechnik Reviewer(s): Marc Culler
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Nov 6, 2024
necessary for GAP 4.13.1 This was forgotten in sagemath#38804 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ x] The title is concise and informative. - [ x] 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: ... --> URL: sagemath#38875 Reported by: Dima Pasechnik Reviewer(s): Marc Culler
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Nov 7, 2024
necessary for GAP 4.13.1 This was forgotten in sagemath#38804 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ x] The title is concise and informative. - [ x] 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: ... --> URL: sagemath#38875 Reported by: Dima Pasechnik Reviewer(s): Marc Culler
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Nov 8, 2024
necessary for GAP 4.13.1 This was forgotten in sagemath#38804 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ x] The title is concise and informative. - [ x] 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: ... --> URL: sagemath#38875 Reported by: Dima Pasechnik Reviewer(s): Marc Culler
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Nov 9, 2024
necessary for GAP 4.13.1 This was forgotten in sagemath#38804 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ x] The title is concise and informative. - [ x] 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: ... --> URL: sagemath#38875 Reported by: Dima Pasechnik Reviewer(s): Marc Culler
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Nov 13, 2024
necessary for GAP 4.13.1 This was forgotten in sagemath#38804 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ x] The title is concise and informative. - [ x] 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: ... --> URL: sagemath#38875 Reported by: Dima Pasechnik Reviewer(s): Marc Culler
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Nov 14, 2024
necessary for GAP 4.13.1 This was forgotten in sagemath#38804 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ x] The title is concise and informative. - [ x] 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: ... --> URL: sagemath#38875 Reported by: Dima Pasechnik Reviewer(s): Marc Culler
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up to:
build/pkgs/gap: Update to 4.13.1, require >= 4.13 #38169With the four additional work items I mentioned in a comment on the latter:
# needs gap_package_polentaLet's see what the CI has to say...