-
-
Notifications
You must be signed in to change notification settings - Fork 490
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 a few more Cython system packages #36548
Support a few more Cython system packages #36548
Conversation
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.
looks good.
please put the correct tags up.
@@ -0,0 +1,5 @@ | |||
SAGE_SPKG_CONFIGURE([primecountpy], [ | |||
SAGE_SPKG_DEPCHECK([cysignals primecount], [ |
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.
Not necessarily for this PR, but a possible next refinement:
primecount
is only used via primecountpy
; so instead of listing it in SAGE_SPKG_DEPCHECK
, we could mark primecount
as "not required" when primecountpy
is available -- like we do for virtualenv
as the dependency of only tox
.
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.
how do we do this? where is this "not required" thing should be?
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.
You can see an example in e.g. filelock/spkg-configure.m4
. In theory we could do this for any package but it requires hard-coding the entire dependency tree in m4 and even I am not in a rush to do that. This is a relatively easy case though.
?? what did I miss? |
Best to rebase it on top of #36482 for the fpylll version |
The changes look straightforward, but it's not quite clear what the standard of review should be for this experimental feature because so many tests are failing. In addition to what I reported in #36536, I see:
|
Someone renamed this with a hyphen instead of an underscore a few minutes after I added it.
d1a55d0
to
44dd3c5
Compare
Done |
Does the container need an update? That's my configuration and I don't see any of those failures. I'm sure Francois would have heard about it too. |
These are real bugs being exposed in our build system / metadata, but if we can't guess the cause from the logs and if no one has these old distros to poke around on, then it shouldn't be a blocker IMO. This may still improve somewhat when we announce the feature. I've been putting it off until I was done writing |
One does not need to "have these old distros". This all runs on Docker. |
Documentation preview for this PR (built with commit 44dd3c5; changes) is ready! 🎉 |
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 guess this is fine, as the feature is marked experimental.
The usual stuff, for: - cypari2 - cysignals - fpylll - memory_allocator - pplpy - primecountpy ### Dependencies - sagemath#36482 URL: sagemath#36548 Reported by: Michael Orlitzky Reviewer(s): Dima Pasechnik, Matthias Köppe, Michael Orlitzky
The usual stuff, for: - cypari2 - cysignals - fpylll - memory_allocator - pplpy - primecountpy ### Dependencies - sagemath#36482 URL: sagemath#36548 Reported by: Michael Orlitzky Reviewer(s): Dima Pasechnik, Matthias Köppe, Michael Orlitzky
The usual stuff, for:
Dependencies