-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sage: Fix on Darwin #264126
Sage: Fix on Darwin #264126
Conversation
Thanks, this is much appreciated! Just a heads-up: I disabled Singular docbuilding on darwin last year because it was hanging and I didn't have a machine to debug it on. I would expect this to cause a Sage test failure or two, but don't worry about it if it doesn't cause test failures. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xcas works fine
Fixed fpylll by disabling |
Agreed on the Cython3 point! By the way, feel free to ping me when you remove the draft status from the PR (since GitHub does not send notifications about it). |
@7c6f434c Thanks for the heads up — apologies for letting this sit in stasis. The only thing I couldn't fix were the Cython linker warnings (and sympow, which fundamentally cannot work on aarch64-darwin), but I will try to rebase on top of the 10.2 update to fix that. |
Ok, I finally came back to this. There are several more changes that have to be made due to I initially tried compiling with C++14, but that's frankly a band-aid fix; the only packages I still need to fix are Giac and Sage itself (sagemath/sage#36840). A few notes:
Also: I still don't know how to fix the |
no they don't. if the PR is force pushed, then the hash is invalidated. You can fetch the commit of the PR, but even though the commit hash cannot be invalidated, the commit can be gc-ed and disappear if the PR is closed or force-pushed. So for non-merged-yet PRs the only truly future proof way is vendoring the patch. |
you are supposed to add a line in pkgs/top-level/aliases.nix I think. |
If the only modification was deleting the Thanks for the update, by the way! The changes here are looking good. I will probably review them in more detail on the weekend. I also just merged the Sage 10.3 upgrade, which included a Singular bump to 4.3.2p16. |
Good to see 10.3 hot off the press, as I no longer have to backport a lot of package fixes — I'll rebase and share my local changes in a day or two. I've gotten Giac 1.9.0-93 to compile (and fixed the Sage bugs), but test cases are "failing" (expressions are symbolically equivalent but not literally so; presumably not a bug introduced by the update to C++17) and Giac completely fails to work from within Sage. I'm sure Sage expects a decently older version of Giac, so I'll try on an older version at some point. Linker errors are still present: I suspect this Darwin-specific behaviour in |
Ok, I gated the gfan patch behind Clang. As for the Singular
See also this SO answer. Naturally, this will also fail on any platform where sandboxing is disabled. While I agree that this is a hack and not ideal (Singular should be using $TMPDIR), it seems much less intrusive then patching Singular (and someone is already trying to fix this upstream). |
@Feyorsh It looks like the tmp issue with Singular has been stalled a bit. If you're down, maybe we could collab on a PR to submit to them? I'm looking forward to Sage on Darwin through Nix! |
@StefanoDeVuono Admittedly I'm lacking the motivation to see such a PR through — even if the bug with I don't mean to discourage you from fixing it upstream though... maybe reach out to the Gentoo Sage maintainer who opened the issue? (I'm also going to be AFK for the next two weeks, if this PR gets reviewed in that time. Would love to see this land before the upgrade to 10.4!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many apologies for taking so long to review your changes, the last few weeks were quite hectic on my side. Everything looks great! I only found a few minor nits. Happy to merge this PR when they're addressed.
installCheckPhase = '' | ||
export HOME=$TMPDIR | ||
"$out/bin/sympow" -curve "[1,2,3,4,5]" -moddeg | grep 'Modular Degree is 464' | ||
echo "[1,-1,0,-79,289]" | "$out/bin/sympow" -analrank | grep ^"Analytic Rank is 4" | ||
"$out/bin/sympow" -curve "[1,-1,0,-79,289]" -analrank | grep ^"Analytic Rank is 4" | ||
"$out/bin/sympow" -curve "[0,1,1,-2,0]" -analrank | grep ^"Analytic Rank is 2" | ||
'' + lib.optionalString (!stdenv.isAarch64) '' | ||
"$out/bin/sympow" -sp 2p16 -curve "[1,2,3,4,5]" | grep '8.3705' | ||
''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
installCheckPhase = '' | |
export HOME=$TMPDIR | |
"$out/bin/sympow" -curve "[1,2,3,4,5]" -moddeg | grep 'Modular Degree is 464' | |
echo "[1,-1,0,-79,289]" | "$out/bin/sympow" -analrank | grep ^"Analytic Rank is 4" | |
"$out/bin/sympow" -curve "[1,-1,0,-79,289]" -analrank | grep ^"Analytic Rank is 4" | |
"$out/bin/sympow" -curve "[0,1,1,-2,0]" -analrank | grep ^"Analytic Rank is 2" | |
'' + lib.optionalString (!stdenv.isAarch64) '' | |
"$out/bin/sympow" -sp 2p16 -curve "[1,2,3,4,5]" | grep '8.3705' | |
''; | |
installCheckPhase = | |
'' | |
export HOME=$TMPDIR | |
"$out/bin/sympow" -curve "[1,2,3,4,5]" -moddeg | grep 'Modular Degree is 464' | |
echo "[1,-1,0,-79,289]" | "$out/bin/sympow" -analrank | grep ^"Analytic Rank is 4" | |
"$out/bin/sympow" -curve "[1,-1,0,-79,289]" -analrank | grep ^"Analytic Rank is 4" | |
"$out/bin/sympow" -curve "[0,1,1,-2,0]" -analrank | grep ^"Analytic Rank is 2" | |
'' | |
+ lib.optionalString (!stdenv.isAarch64) '' | |
"$out/bin/sympow" -sp 2p16 -curve "[1,2,3,4,5]" | grep '8.3705' | |
''; |
to appease our nixfmt overlords.
@ofborg build sageWithDoc giac giac-with-xcas |
I think the More worrying are the Giac-related sage test failures. For example:
and
It's possible one of the errors is a side-effect of the other, but at least one should be real. In unrelated news, unfortunately Sage recently broke on master due to the Cython 3.0.11 upgrade and I might have to update it to 10.4 to apply sagemath/sage#38500. But if Sage tests pass on this branch, I don't see any problem in merging this PR. Note that you'll have to autosquash the fixup commits, since GitHub does not do that automatically. |
@Feyorsh Turns out the upstreamed Giac patches I mentioned (which you migrated to in 1d1710c) do not exactly match your previous version, and everything works before the change I suggested. Apologies, I should have tested it before making the suggestion. If you reinstate |
Nearly fixed on aarch64-darwin! (Note that this coincides with a release of Xcas for macOS, but this is not using Parisse's tarball).
This issue does not present at build time, but only in sage doctests. See https://www.github.com/sagemath/sage/issues/25118
--enable-sage was removed a while ago linbox-team/linbox#146
This upgrade fixes the issue with drawTropicalCurve docbuilding on Darwin (although said doctest is pretty useless now that Apple has removed postscript from macOS). Removed enableGfanlib; you can still overrideAttrs Added mpfr to be more consistent with upstream packaging recommendations
Also backport a fix that silences `xcode-select: not found` warnings
Tampering with ulimit is not ideal and may not work in the Darwin sandbox, but is necessary due to how Singular handles allocations.
That's on me for not running the Sage tests before pushing. Based on my correspondence with Parisse, it seems my patches were more of a loose inspiration than anything else (let's just hope it works when Giac gets bumped in the future). I'm running a full build currently and I'll let you know if I encounter any issues with the Sage test suite; if not, this should be good to go. |
lgtm @collares |
@ofborg build giac-with-xcas sageWithDoc |
Wonderful, thank you very much! Excited to have Darwin support for Nixpkgs Sage. |
@Feyorsh Unfortunately Maxima failed to build on aarch64-darwin: https://hydra.nixos.org/build/271482748 and https://cache.nixos.org/log/jq3wzld79yzdqaxqlvlsyfa1x7nm8vk0-maxima-5.47.0.drv
|
Looking into it. Can't reproduce the error with |
Are you testing Maxima as it is built for Sage? That is,
In particular, ECL as the lisp compiler seems to always use GCC as part of the Lisp compilation process (as seen below):
If that's not the issue and you're not seeing the error even in this configuration, my guess would be that it is an intermittent GC problem (maybe related to https://gitlab.com/embeddable-common-lisp/ecl/-/issues/718 or ivmai/bdwgc#569 (comment), although in our case it wouldn't be happening at ECL build time.). The reason I'm even suspecting a GC issue is that |
Yes, I tested using 74b34d6 per the Hydra reproduce script, with Unfortunately I need another 5 days until I can give this proper attention; this issue doesn't seem to be a regression caused by this PR, but if these commits need to be reverted, so be it. Additional support should not come at the expense of breaking existing packages. |
Oh, sorry for giving the wrong impression! Fixing the Maxima issue is not urgent, and this PR does not need to be reverted. |
Also not urgent, just for your information: Sage Maxima built OK on staging-next, so it seems like it was just an intermittent failure. But Singular is failing with
This seems less likely to be an intermittent failure, but perhaps we should wait for a rebuild. See https://hydra.nixos.org/build/271681864 for details. |
Seems like the Singular failure was also intermittent! Only the |
Description of changes
There shouldn't be any fundamental blockers to building Sage on aarch64-darwin, given that it builds fine with Homebrew. I am quite inexperienced with Nix, so please let me know if I need to fix my code to be more idiomatic or otherwise correct.
Currently, I am able to build Sage and "pass" all the test cases: some test cases fail because of linker warnings when Cython compiles:
I'm assuming I need to poke around sage-env.nix as suggested here and add these missing dependencies to
PKG_CONFIG_PATH
. Besides that, Sage behaves as you would expect, all functional test cases pass just fine. Disabling tests is obviously not an actual solution, but I'm confident these issues should be somewhat straightforward to fix.108e281: fflas-ffpack seems to build and pass all test cases successfully.
d154d8a32f031930d8f2ba80f9f60866fdf0d142: Interestingly, although m4ri builds fine, m4rie seems to only build properly on Apple Clang, not LLVM Clang. As suggested in an upstream issue, I changed
-O2
to-O0
and it compiles. Please let me know if I should be modifying CFLAGS in a different way or changing hardening flags.98a725a: There seems to be an issue with Apple's
libintl
assuming locale variables being set, which causes most of Giac's test cases to fail. Passing--disable-nls
partially fixes the issue, leading to only 4 test cases failing. I would like to fix this completely, but according to a Sage discussion, it looks like these failures are not new. I also added some Darwin patches for Giac from Sage, but they likely have no bearing on whether or not the above issue gets resolved.aff699e: When running Sage, I received warnings about
awk
not being found. Indeed,gawk
was not an input intosage-env.nix
. It seems surprising that only Darwin would requireawk
so I didn't guard it withlib.optionals stdenv.isDarwin
; is this warning present on Linux without the inclusion ofgawk
?c65031c8be5e03f4a9258c176da990e42e99ddca: I just used
macosx
as the target because it's what Sage uses and it looks like there's some issues with the multithreaded build on aarch64-linux. I haven't testedmacosx-thr
, though.7e419ac: fpylll (and fplll) need to disable
long double
on both Cygwin and aarch64-darwin (x86_64-darwin does supportlong double
and shouldn't be changed).TODO
linbox: I noticed that a test failed when building linbox as part of Sage (i.e.
nix-build -A sage
), but when I built it separately all tests passed. I assumed that was just an issue with a test case running out of memory or timing out, but I don't know how to confirm that that's the case.sympow: Install check fails, almost certainly due to this warning:
Sage packaging does not appear to fix this, so the "fix" might be simply disabling Sympow within Sage.
Giac: Pass remaining test cases (or ignore if infeasible).
Sage: Fix linker warnings as described above.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)