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: sage-7.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:

Status badges

Description


Change History (32)

comment:1 Changed 5 years ago by fbissey

  • Branch set to u/fbissey/distutils_cleanup
  • Commit set to 036da4ed65f6592859d0ef1b14cb790e2433a7db

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


New commits:

036da4eFirst batch of clean up as discussed in #12426

comment:2 Changed 5 years ago by jdemeyer

  • Branch changed from u/fbissey/distutils_cleanup to u/jdemeyer/distutils_cleanup

comment:3 Changed 5 years ago by jdemeyer

  • 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:

5aec722Clean up some # distutils directives

comment:4 Changed 5 years ago by git

  • Commit changed from 5aec722d364b14b35e9c464b676a6c26c9936786 to acfc74f0d30014580186f4c683d8a263c04ccf72

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

acfc74fClean up some # distutils directives

comment:5 Changed 5 years ago by jdemeyer

  • 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 fbissey

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 jdemeyer

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 fbissey

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

comment:9 follow-up: Changed 5 years ago by jdemeyer

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

comment:10 Changed 5 years ago by git

  • Commit changed from acfc74f0d30014580186f4c683d8a263c04ccf72 to 84f7b6e27a302c7c366d6ab7ed235083f2d9bd94

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

84f7b6eClean up some # distutils directives

comment:11 in reply to: ↑ 9 Changed 5 years ago by fbissey

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.

comment:12 Changed 5 years ago by jdemeyer

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 fbissey

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.

comment:14 follow-up: Changed 5 years ago by jdemeyer

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 fbissey

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.

comment:16 Changed 5 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

comment:17 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

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

comment:18 Changed 5 years ago by fbissey

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 fbissey

  • 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:

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

comment:20 Changed 5 years ago by fbissey

  • 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 jdemeyer

  • 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 jdemeyer

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 jdemeyer

  • Status changed from needs_review to needs_work

comment:24 follow-up: Changed 5 years ago by fbissey

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 jdemeyer

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 git

  • Commit changed from dd56bb7823f43ebda959c7d5bea02cbe2f98e6d3 to 19e0bb0857f2f2e2c1faefda31501b07fdbae18f

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

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

comment:27 Changed 5 years ago by fbissey

  • 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 jdemeyer

  • Branch changed from u/fbissey/distutils_cleanup to u/jdemeyer/distutils_cleanup

comment:29 Changed 5 years ago by jdemeyer

  • Commit changed from 19e0bb0857f2f2e2c1faefda31501b07fdbae18f to 1109be4f888abb8e449bcddb78842ef19736d23f

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


New commits:

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

comment:30 Changed 5 years ago by jdemeyer

I would like to see what the patchbot thinks.

comment:31 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to positive_review

Seems to work fine on the patchbot.

comment:32 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/distutils_cleanup to 1109be4f888abb8e449bcddb78842ef19736d23f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.