Opened 5 years ago
Closed 5 years ago
#21749 closed enhancement (fixed)
Clean up some # distutils directives
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage7.5 
Component:  cython  Keywords:  
Cc:  fbissey  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  François Bissey 
Report Upstream:  N/A  Work issues:  
Branch:  1109be4 (Commits, GitHub, GitLab)  Commit:  1109be4f888abb8e449bcddb78842ef19736d23f 
Dependencies:  Stopgaps: 
Description
Change History (32)
comment:1 Changed 5 years ago by
 Branch set to u/fbissey/distutils_cleanup
 Commit set to 036da4ed65f6592859d0ef1b14cb790e2433a7db
comment:2 Changed 5 years ago by
 Branch changed from u/fbissey/distutils_cleanup to u/jdemeyer/distutils_cleanup
comment:3 Changed 5 years ago by
 Commit changed from 036da4ed65f6592859d0ef1b14cb790e2433a7db to 5aec722d364b14b35e9c464b676a6c26c9936786
Sorry for overwriting your branch, I was working on it at the same time as you. I haven't looked at src/module_list.py
yet (I see that you did), I'll do that now.
New commits:
5aec722  Clean up some # distutils directives

comment:4 Changed 5 years ago by
 Commit changed from 5aec722d364b14b35e9c464b676a6c26c9936786 to acfc74f0d30014580186f4c683d8a263c04ccf72
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
acfc74f  Clean up some # distutils directives

comment:5 Changed 5 years ago by
 Status changed from new to needs_review
I removed c99
from src/sage/arith/multi_modular.pxd
without moving it to the .pyx
file since I couldn't see why it was needed.
comment:6 Changed 5 years ago by
OK, some of the moving in linbox
from pxd
to pyx
will reduce the number of files that flops from c
to cpp
too I presume. It will be interesting to do a new clang run with this branch.
comment:7 Changed 5 years ago by
I would also like to move some language = 'c++'
from src/module_list.py
to # distutils: language = c++
in .pxd
/.pyx
files. Should I add that to this branch?
comment:8 Changed 5 years ago by
Absolutely. m4ri
could be given the pkgconfig treatment, should we do that too?
comment:9 followup: ↓ 11 Changed 5 years ago by
I was thinking about fixing just a few small things. No big changes like adding pkgconfig
.
Another thing I noticed: many extensions are compiled with gmpxx
but don't seem to use it...
comment:10 Changed 5 years ago by
 Commit changed from acfc74f0d30014580186f4c683d8a263c04ccf72 to 84f7b6e27a302c7c366d6ab7ed235083f2d9bd94
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
84f7b6e  Clean up some # distutils directives

comment:11 in reply to: ↑ 9 Changed 5 years ago by
Replying to jdemeyer:
I was thinking about fixing just a few small things. No big changes like adding
pkgconfig
.
OK, another ticket later then.
Another thing I noticed: many extensions are compiled with
gmpxx
but don't seem to use it...
I'll check that. Wait a minute.
comment:12 Changed 5 years ago by
I'll stop here for this ticket. Please review (and try not to insist on adding more stuff. I know this is far from perfect but at least it's a step).
comment:13 Changed 5 years ago by
Cool, for your question
readelf d `find /usr/lib64/python2.7/sitepackages/sage name \*.so`  grep C 6 libgmpxx File: /usr/lib64/python2.7/sitepackages/sage/modular/arithgroup/farey_symbol.so Dynamic section at offset 0x5bb88 contains 29 entries: Tag Type Name/Value 0x0000000000000001 (NEEDED) Shared library: [libgmp.so.10] 0x0000000000000001 (NEEDED) Shared library: [libgmpxx.so.4] 0x0000000000000001 (NEEDED) Shared library: [libstdc++.so.6] 0x0000000000000001 (NEEDED) Shared library: [libpython2.7.so.1.0] 0x0000000000000001 (NEEDED) Shared library: [libgcc_s.so.1] 0x0000000000000001 (NEEDED) Shared library: [libc.so.6]
That's the only hit with libgmpxx when compiled with asneeded
.
Will review this today.
comment:14 followup: ↓ 15 Changed 5 years ago by
Was NTL
at some point in the past compiled with gmpxx
? That might explain...
comment:15 in reply to: ↑ 14 Changed 5 years ago by
Replying to jdemeyer:
Was
NTL
at some point in the past compiled withgmpxx
? That might explain...
Could, but I cannot answer that without doing sage archeology. Gentoo side as far back as ntl5.5.2, gmpxx
wasn't a requirement in dependency. So if it was in sage, that was probably a mistake.
comment:16 Changed 5 years ago by
 Reviewers set to François Bissey
 Status changed from needs_review to positive_review
comment:17 Changed 5 years ago by
 Status changed from positive_review to needs_work
I'm getting lots of bad stuff like on the patchbot
[sagelib7.5.beta0] /home/kevin/sagepatchbot/local/include/m4ri/mzd.h:1287:3: error: 'for' loop initial declarations are only allowed in C99 or C11 mode
comment:18 Changed 5 years ago by
Looks like we should have put back std=c99
in matrix_integer_dense.pyx
. We probably have compilers defaulting to the right standard or higher when we tested. The C/C++ confusion has gone away after moving the linbox stuff from the .pxd to the .pyx.
comment:19 Changed 5 years ago by
 Branch changed from u/jdemeyer/distutils_cleanup to u/fbissey/distutils_cleanup
 Commit changed from 84f7b6e27a302c7c366d6ab7ed235083f2d9bd94 to dd56bb7823f43ebda959c7d5bea02cbe2f98e6d3
 Status changed from needs_work to needs_review
Acting on my comment to see what the bots do.
New commits:
dd56bb7  Readd std=c99 for matrix_integer_dense but in the .pyx file this time.

comment:20 Changed 5 years ago by
 Status changed from needs_review to positive_review
Seems like the latest patchbot failures are unrelated to this ticket.
comment:21 Changed 5 years ago by
 Status changed from positive_review to needs_review
I don't think you should review your own commit (unless it is trivial, which is not the case here).
comment:22 Changed 5 years ago by
Anyway, the right solution would be to add the flags to the file src/sage/libs/m4ri.pxd
. If m4ri needs C99, then everything which uses that header should be compiled as C99.
comment:23 Changed 5 years ago by
 Status changed from needs_review to needs_work
comment:24 followup: ↓ 25 Changed 5 years ago by
OK. We already have
m4ri_extra_compile_args = pkgconfig.cflags('m4ri').split()
if C99 is needed for m4ri
we should get upstream to include it in the CFLAGS it sets, separately from fixing it for now.
comment:25 in reply to: ↑ 24 Changed 5 years ago by
For now, I would suggest to just add std=c99
to src/sage/libs/m4ri.pxd
and postpone more fundamental fixes to a new ticket.
comment:26 Changed 5 years ago by
 Commit changed from dd56bb7823f43ebda959c7d5bea02cbe2f98e6d3 to 19e0bb0857f2f2e2c1faefda31501b07fdbae18f
comment:27 Changed 5 years ago by
 Status changed from needs_work to needs_review
Not suggesting we do something fundamental here, sorry if it wasn't clear. Adding looking at m4ri
.pc file upstream to my TODO list.
comment:28 Changed 5 years ago by
 Branch changed from u/fbissey/distutils_cleanup to u/jdemeyer/distutils_cleanup
comment:29 Changed 5 years ago by
 Commit changed from 19e0bb0857f2f2e2c1faefda31501b07fdbae18f to 1109be4f888abb8e449bcddb78842ef19736d23f
comment:30 Changed 5 years ago by
I would like to see what the patchbot thinks.
comment:31 Changed 5 years ago by
 Status changed from needs_review to positive_review
Seems to work fine on the patchbot.
comment:32 Changed 5 years ago by
 Branch changed from u/jdemeyer/distutils_cleanup to 1109be4f888abb8e449bcddb78842ef19736d23f
 Resolution set to fixed
 Status changed from positive_review to closed
OK first batch as we discussed on #12426. I presume you have some additions to that lot.
New commits:
First batch of clean up as discussed in #12426