-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
Support python 3.12 #138
Support python 3.12 #138
Conversation
See sagemath/sage#36407 (comment) for an explanation of the change. |
rebased |
gppath = shutil.which("gp") | ||
|
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.
I don't know what's "shutil" or "shutils" but I note that you use two different names.
Also: I didn't need this patch to successfully build cypari2 with python 3.12. What I PR is good enough (as in: I can build and fully test sagemath with it). If this is a problem, is not catched by the testsuite of cypari2 or sagemath. I know distutils is not available anymore, so maybe that means this file is not used at all? It's definitely not tested (as all tests pass for me IIRC).
PS: I find it intriguing the asymmetry that some sage developers can modify my PRs but I can't modify any PR not authored by me. Granted, that was a useful feature of trac, but it's not a standard feature of gh afaik. I don't mind you changing my PRs, just something to think about. I'm glad that you are working on getting releases for cypari2 (and the other pkgs we need).
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.
I find it intriguing the asymmetry that some sage developers can modify my PRs but I can't modify any PR not authored by me.
One needs to be in the Maintainer role for the project. https://docs.github.com/en/organizations/managing-user-access-to-your-organizations-repositories/managing-repository-roles/repository-roles-for-an-organization
I am not sure if there is a clear protocol how and when to elevate developers to this role, but an email to sagemath-admins would probably help.
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.
Also, as PR author one can uncheck the box "Maintainers are allowed to edit this pull request." to remove unwelcome interference from maintainers.
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.
I've given sagemath/triage team (with @tornaria there) same rights here (and on few other sagemath/ repos) as on the main Sage repo. Hopefully this gives more rights to fix stuff.
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.
autogen/paths.py
Outdated
import shutils | ||
|
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.
I mean in the import here
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.
Thanks. It should be shutil
.
https://docs.python.org/3/library/shutil.html
I presume the CI errors are not from our code. |
rebased again |
I must say I am bothered by the presence of |
Provided that pplpy and fpylll do not need this stuff as well, could sage make us of a copy coming from cpypari2 here rather than having its own? |
As I mentioned in sagemath/sage#36407 (comment): But python 3.12 is shipping and we need sagemath working. This makes it work, it's needed in cypari2 and in fpylll. I'm also bothered by code "duplication". For now, let's think of this as "vendoring in" the module, that's why I stressed that I pushed "identical" files. The whole thing is structured in such a way that (a) users (cypari2, fpylll, sagemath) only care about the interface as specified in |
+1 on duplicating it for now and trying to push it into Cython as a follow up. -1 on trying to have Sage import it from cypari2; that would be problematic in the context of modularization. |
I am OK with whatever you want to do so long as we have had the conversation. It is a case of not flying blind. |
This was introduced by accident in sagemath#138.
The necessary functions are copied from cpython 3.12
Include/internal/pycore_long.h
, which is private. This should make it easy to copy new versions if cpython internals change. For cpython < 3.12 I implemented equivalent functions.All of this is placed in separate files
pycore_long.pxd
andpycore_long.h
so it's easy to share between projects (fpylll, sagemath).Tested in 64-bit and 32-bit x86.
Fixes: #137