Opened 8 years ago

Closed 5 years ago

#14268 closed defect (fixed)

Remove zn_poly ulong work-around

Reported by: jdemeyer Owned by: GeorgSWeber
Priority: major Milestone: sage-7.1
Component: packages: standard Keywords:
Cc: jpflori Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 9eacb7c (Commits, GitHub, GitLab) Commit: 9eacb7c9eaf3ff7d6742f2cc333f0a70a6700b1b
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

We no longer need to mess with ulong in zn_poly.

Attachments (3)

flint-1.5.2.p4.diff​ (5.0 KB) - added by jdemeyer 8 years ago.
14268_remove_c99.patch​ (10.6 KB) - added by jdemeyer 8 years ago.
zn_poly-0.9.p11.diff​ (1.3 KB) - added by jdemeyer 8 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 8 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Dependencies set to #14265

comment:2 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:3 follow-up: Changed 8 years ago by leif

IIRC we had trouble with typedefed ulong on Solaris.

For removing -std=c99 from module_list.py, see this comment.

Changed 8 years ago by jdemeyer

comment:4 in reply to: ↑ 3 ; follow-up: Changed 8 years ago by jdemeyer

Replying to leif:

IIRC we had trouble with typedefed ulong on Solaris.

It works correctly on mark at least.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by leif

Replying to jdemeyer:

Replying to leif:

IIRC we had trouble with typedefed ulong on Solaris.

It works correctly on mark at least.

But causes trouble if you include both flint.h and sys/types.h.

Changed 8 years ago by jdemeyer

comment:6 in reply to: ↑ 5 Changed 8 years ago by jdemeyer

Replying to leif:

But causes trouble if you include both flint.h and sys/types.h.

Still works correctly on mark. Do you have a concrete example of a file which fails to compile due to this "issue"?

comment:7 follow-up: Changed 8 years ago by jdemeyer

Replying to ticket:14265:19:

Do env CFLAGS="-DFOO" ./sage -ba and the -std=gnu99 vanishes

It does indeed. I'd say this is a bug.

so we must not remove -std=c99 from the extra_compile_flags (or what it's called).

I disagree with this conclusion, since over-riding CFLAGS also removes other mandatory flags, like -fno-strict-aliasing. So you're basically saying that my patch breaks something which is already broken.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 8 years ago by leif

Replying to jdemeyer:

Replying to ticket:14265:19:

Do env CFLAGS="-DFOO" ./sage -ba and the -std=gnu99 vanishes

It does indeed. I'd say this is a bug.

Well, depends. Probably a bug in Sage, but it always used to be that way. (And I suggested adding -fno-strict-aliasing in setup.py loooooong time ago, but nobody bothered. Only Cython-generated code requires this.)

so we must not remove -std=c99 from the extra_compile_flags (or what it's called).

I disagree with this conclusion, since over-riding CFLAGS also removes other mandatory flags, like -fno-strict-aliasing. So you're basically saying that my patch breaks something which is already broken.

Nope. It further breaks building extension modules, IMHO without any gain.

(Btw., exporting CFLAGS="" is sufficient.)

Changed 8 years ago by jdemeyer

comment:9 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:10 Changed 8 years ago by leif

Slightly related: #7987 (three years bit-rottening)

comment:11 in reply to: ↑ 8 Changed 8 years ago by jdemeyer

Replying to leif:

It further breaks building extension modules

Not really, there's nothing broken which wasn't broken before.

IMHO without any gain.

IMHO, simplification is a gain.

comment:12 Changed 8 years ago by leif

  • Cc jpflori added
  • Status changed from new to needs_info

Just for the record: #14265 no longer compiles Python as C99, but instead #defines __C99FEATURES__ (on Solaris).

comment:13 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_info to needs_review
  • Summary changed from Remove C99 flags in module_list.py to Remove #define ulong

comment:14 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:15 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:16 Changed 8 years ago by jpflori

Could you try the FLINT 2.3 spkg from #12173 and confirm that vanilla FLINT 2.3 still gives trouble with respect to zn_poly and C99? Then we'll know whether we have to include changes similar to the one you included here there or not (and I'll report upstream accordingly)

comment:17 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:18 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:19 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:20 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:21 Changed 5 years ago by aapitzsch

  • Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to needs_review

Is this still an issue with flint 2.5?

comment:22 Changed 5 years ago by jdemeyer

  • Dependencies #14265 deleted
  • Milestone changed from sage-duplicate/invalid/wontfix to sage-7.1
  • Status changed from needs_review to needs_work

Even if it's no longer an issue, we should remove the work-around from zn_poly.

comment:23 Changed 5 years ago by jdemeyer

  • Component changed from build to packages: standard
  • Description modified (diff)
  • Summary changed from Remove #define ulong to Remove zn_poly ulong work-around

comment:24 Changed 5 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/14268

comment:25 Changed 5 years ago by jdemeyer

  • Commit set to 9eacb7c9eaf3ff7d6742f2cc333f0a70a6700b1b
  • Status changed from needs_work to needs_review

New commits:

​9eacb7cRemove ulong work-around for zn_poly

comment:26 Changed 5 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:27 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/14268 to 9eacb7c9eaf3ff7d6742f2cc333f0a70a6700b1b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.