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: |
Description (last modified by )
We no longer need to mess with ulong
in zn_poly
.
Attachments (3)
Change History (30)
comment:1 Changed 8 years ago by
- Dependencies set to #14265
comment:2 Changed 8 years ago by
- Description modified (diff)
comment:3 follow-up: ↓ 4 Changed 8 years ago by
Changed 8 years ago by
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 8 years ago by
Replying to leif:
IIRC we had trouble with
typedef
edulong
on Solaris.
It works correctly on mark
at least.
comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 8 years ago by
Changed 8 years ago by
comment:6 in reply to: ↑ 5 Changed 8 years ago by
Replying to leif:
But causes trouble if you include both
flint.h
andsys/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: ↓ 8 Changed 8 years ago by
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 theextra_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: ↓ 11 Changed 8 years ago by
Replying to jdemeyer:
Replying to ticket:14265:19:
Do
env CFLAGS="-DFOO" ./sage -ba
and the-std=gnu99
vanishesIt 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 theextra_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
comment:9 Changed 8 years ago by
- Description modified (diff)
comment:10 Changed 8 years ago by
Slightly related: #7987 (three years bit-rottening)
comment:11 in reply to: ↑ 8 Changed 8 years ago by
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
- Cc jpflori added
- Status changed from new to needs_info
Just for the record: #14265 no longer compiles Python as C99, but instead #define
s __C99FEATURES__
(on Solaris).
comment:13 Changed 8 years ago by
- 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
- Status changed from needs_review to needs_work
comment:15 Changed 8 years ago by
- Description modified (diff)
comment:16 Changed 8 years ago by
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
- Milestone changed from sage-5.11 to sage-5.12
comment:18 Changed 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:19 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:20 Changed 7 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:21 Changed 5 years ago by
- 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
- 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
- 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
- Branch set to u/jdemeyer/ticket/14268
comment:25 Changed 5 years ago by
- Commit set to 9eacb7c9eaf3ff7d6742f2cc333f0a70a6700b1b
- Status changed from needs_work to needs_review
New commits:
9eacb7c | Remove ulong work-around for zn_poly
|
comment:26 Changed 5 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
LGTM.
comment:27 Changed 5 years ago by
- Branch changed from u/jdemeyer/ticket/14268 to 9eacb7c9eaf3ff7d6742f2cc333f0a70a6700b1b
- Resolution set to fixed
- Status changed from positive_review to closed
IIRC we had trouble with
typedef
edulong
on Solaris.For removing
-std=c99
frommodule_list.py
, see this comment.