Clean up some # distutils directives

[jdemeyer]

CC: @kiwifb

Component: cython

Author: Jeroen Demeyer

Branch/Commit: 1109be4

Reviewer: François Bissey

Issue created by migration from https://trac.sagemath.org/ticket/21749

[kiwifb]

Commit: 036da4e

[kiwifb]

comment:1

OK first batch as we discussed on #12426. I presume you have some additions to that lot.

---

New commits:

036da4e

First batch of clean up as discussed in #12426

[kiwifb]

Branch: u/fbissey/distutils_cleanup

[jdemeyer]

Changed branch from u/fbissey/distutils_cleanup to u/jdemeyer/distutils_cleanup

[jdemeyer]

Changed commit from 036da4e to 5aec722

[jdemeyer]

comment:3

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

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

acfc74f

Clean up some # distutils directives

[sagetrac-git]

Changed commit from 5aec722 to acfc74f

[jdemeyer]

comment:5

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.

[kiwifb]

comment:6

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.

[jdemeyer]

comment:7

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?

[kiwifb]

comment:8

Absolutely. m4ri could be given the pkg-config treatment, should we do that too?

[jdemeyer]

comment:9

I was thinking about fixing just a few small things. No big changes like adding pkg-config.

Another thing I noticed: many extensions are compiled with gmpxx but don't seem to use it...

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

84f7b6e

Clean up some # distutils directives

[sagetrac-git]

Changed commit from acfc74f to 84f7b6e

[kiwifb]

comment:11

Replying to @jdemeyer:

I was thinking about fixing just a few small things. No big changes like adding pkg-config.

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.

[jdemeyer]

comment:12

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).

[kiwifb]

comment:13

Cool, for your question

readelf -d `find /usr/lib64/python2.7/site-packages/sage -name \*.so` | grep -C 6 libgmpxx

File: /usr/lib64/python2.7/site-packages/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 -as-needed.

Will review this today.

[jdemeyer]

comment:14

Was NTL at some point in the past compiled with gmpxx? That might explain...

[kiwifb]

comment:15

Replying to @jdemeyer:

Was NTL at some point in the past compiled with gmpxx? That might explain...

Could, but I cannot answer that without doing sage archeology. Gentoo side as far back as ntl-5.5.2, gmpxx wasn't a requirement in dependency. So if it was in sage, that was probably a mistake.

[kiwifb]

Reviewer: François Bissey

[vbraun]

comment:17

I'm getting lots of bad stuff like on the patchbot

[sagelib-7.5.beta0] /home/kevin/sage-patchbot/local/include/m4ri/mzd.h:1287:3: error: 'for' loop initial declarations are only allowed in C99 or C11 mode

[kiwifb]

Changed branch from u/jdemeyer/distutils_cleanup to u/fbissey/distutils_cleanup

[kiwifb]

comment:19

Acting on my comment to see what the bots do.

---

New commits:

dd56bb7

Re-add -std=c99 for matrix_integer_dense but in the .pyx file this time.

[kiwifb]

comment:20

Seems like the latest patchbot failures are unrelated to this ticket.

[jdemeyer]

comment:21

I don't think you should review your own commit (unless it is trivial, which is not the case here).

[jdemeyer]

comment:22

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.

[kiwifb]

comment:24

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.

[jdemeyer]

comment:25

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.

[sagetrac-git]

Changed commit from dd56bb7 to 19e0bb0

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

339b2cc	Merge branch 'develop' into distutils
808767c	Revert "Re-add -std=c99 for matrix_integer_dense but in the .pyx file this time."
19e0bb0	Adding -std=c99 to m4ri.pxd so it can be inherited by all of its users

[kiwifb]

comment:27

Not suggesting we do something fundamental here, sorry if it wasn't clear. Adding looking at m4ri .pc file upstream to my TODO list.

[jdemeyer]

Changed branch from u/fbissey/distutils_cleanup to u/jdemeyer/distutils_cleanup

[jdemeyer]

comment:29

Cleaned up commits (rewriting git history), no real changes.

---

New commits:

36ae1a5	Clean up some # distutils directives
1109be4	Add -std=c99 to m4ri.pxd so it can be inherited by all of its users

[jdemeyer]

Changed commit from 19e0bb0 to 1109be4

[jdemeyer]

comment:30

I would like to see what the patchbot thinks.

[jdemeyer]

comment:31

Seems to work fine on the patchbot.

[vbraun]

Changed branch from u/jdemeyer/distutils_cleanup to 1109be4