Support a few more Cython system packages#36548
Conversation
dimpase
left a comment
There was a problem hiding this comment.
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.
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.
how do we do this? where is this "not required" thing should be?
There was a problem hiding this comment.
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! 🎉 |
mkoeppe
left a comment
There was a problem hiding this comment.
I guess this is fine, as the feature is marked experimental.
sagemathgh-36548: Support a few more Cython system packages 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
sagemathgh-36548: Support a few more Cython system packages 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