Opened 10 years ago

Closed 8 years ago

#9544 closed enhancement (fixed)

Fix flintqs on FreeBSD

Reported by: pjeremy Owned by: pjeremy
Priority: major Milestone: sage-5.1
Component: porting: BSD Keywords:
Cc: Merged in: sage-5.1.rc0
Authors: Peter Jeremy, Jeroen Demeyer Reviewers: Stephen Montgomery-Smith, Jeroen Demeyer, Karl-Dieter Crisman
Report Upstream: None of the above - read trac for reasoning. Work issues:
Branch: Commit:
Dependencies: #12855 Stopgaps:

Description (last modified by jdemeyer)

TonelliShanks.h references int32_t but does not directly include <stdint.h> (where it is defined). On FreeBSD using gcc45 (but not the base gcc), this causes compilation to fail.

spkg: ​http://boxen.math.washington.edu/home/jdemeyer/spkg/flintqs-20070817.p8.spkg

Attachments (1)

flintqs-20070817.p8.diff​ (1.2 KB) - added by jdemeyer 8 years ago.
Diff for the flintqs spkg. For reference / review only.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 10 years ago by pjeremy

  • Description modified (diff)
  • Milestone set to sage-4.5.1

comment:2 Changed 10 years ago by pjeremy

  • Description modified (diff)

comment:3 Changed 9 years ago by kcrisman

Apparently at least once ​Stephen Montgomery-Smith compiled Sage on a "port" successfully on FreeBSD without this.

comment:4 Changed 9 years ago by stephen

Stephen Montgomery-Smith reports that this patch is absolutely necessary for building under FreeBSD.

comment:5 Changed 9 years ago by leif

Just for the record: There'll be a new FLINTQS spkg at #12855 soon...

comment:6 Changed 8 years ago by kcrisman

  • Authors set to Peter Jeremy
  • Reviewers set to Stephen Montgomery-Smith

The current spkg-install change being used in the port is

  • flintqs-20070817.p6/

    old new  
    77fi
    88
    99cp patches/lanczos.h  src/
     10patch -p0 < patches/TonelliShanks.h.patch
    1011
    1112cd src

but we'll have to check the exit status on that. Or just add this into #12855 or something.

comment:7 Changed 8 years ago by jdemeyer

  • Authors changed from Peter Jeremy to Peter Jeremy, Jeroen Demeyer
  • Dependencies set to #12855
  • Description modified (diff)
  • Milestone changed from sage-5.1 to sage-5.2
  • Status changed from new to needs_review

comment:8 Changed 8 years ago by jdemeyer

Rebased to #12855.

Changed 8 years ago by jdemeyer

Diff for the flintqs spkg. For reference / review only.

comment:9 follow-up: Changed 8 years ago by kcrisman

  • Report Upstream changed from Not yet reported upstream; Will do shortly. to None of the above - read trac for reasoning.

This looks good, but since it's applied universally and not just on FreeBSD I am hesitant to give it positive review. Jeroen, did you try this out on some of the buildbots?

Also, is there even an "upstream"? This is a five-year-old program, and some searching indicates that maybe a newer version is included in flint itself? Anyway, I figure that the age makes reporting upstream inappropriate.

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

Replying to kcrisman:

Jeroen, did you try this out on some of the buildbots?

No, but I don't expect problems. What harm could an additional header file do?

comment:11 in reply to: ↑ 10 ; follow-up: Changed 8 years ago by kcrisman

  • Reviewers changed from Stephen Montgomery-Smith to Stephen Montgomery-Smith, Jeroen Demeyer, Karl-Dieter Crisman

Jeroen, did you try this out on some of the buildbots?

No, but I don't expect problems. What harm could an additional header file do?

I don't know, that's why I asked! Extra imports in Python sometimes cause all kinds of circularity issues or slowdowns, and I don't know enough about C to say either way.

But since you just repackaged this, I think it's appropriate for you to be a reviewer as well. And I checked that the package is well-formed and installs on OS X.

I also notice that you included stdint after stdlib, while Stephen does the opposite in his patch ​here. I assume this also doesn't matter? Sorry for the very stupid questions.

See also #11792?

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

Replying to kcrisman:

Jeroen, did you try this out on some of the buildbots?

No, but I don't expect problems. What harm could an additional header file do?

I don't know, that's why I asked! Extra imports in Python sometimes cause all kinds of circularity issues or slowdowns, and I don't know enough about C to say either way.

In theory this could cause problems, but presumably the C headers are well-designed such that these problems don't occur.

I also notice that you included stdint after stdlib, while Stephen does the opposite in his patch

It also shouldn't matter. The way I did it looked more natural, but there's really no reason.

comment:13 follow-up: Changed 8 years ago by kcrisman

It also shouldn't matter. The way I did it looked more natural, but there's really no reason.

Ok.

I'm even having trouble finding where this is used. sage -grep with various upper/lowercase of flintqs does nothing, it's not in module_list.py, in spkg/standard/deps it only has dependencies... and I'm going to need CPU firepower in a few minutes, can't do a full doctest right now. Weird.

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

Replying to kcrisman:

I'm even having trouble finding where this is used. sage -grep with various upper/lowercase of flintqs does nothing, it's not in module_list.py, in spkg/standard/deps it only has dependencies... and I'm going to need CPU firepower in a few minutes, can't do a full doctest right now. Weird.

The interface is sage/interfaces/qsieve.py.

comment:15 in reply to: ↑ 14 ; follow-up: Changed 8 years ago by kcrisman

  • Milestone changed from sage-5.2 to sage-5.1
  • Status changed from needs_review to positive_review

I'm even having trouble finding where this is used. sage -grep with various upper/lowercase of flintqs does nothing, it's not in module_list.py, in spkg/standard/deps it only has dependencies... and I'm going to need CPU firepower in a few minutes, can't do a full doctest right now. Weird.

The interface is sage/interfaces/qsieve.py.

Great, thanks. That passes long tests, so since there really isn't any reason anything should be bad, we are good to go. (Yes, I did sage -b.)

Since p7 is also 5.1, tentatively putting it there, but feel free to do whatever is convenient.

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

Replying to kcrisman:

Since p7 is also 5.1, tentatively putting it there, but feel free to do whatever is convenient.

.p7 still needs review...

comment:17 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.1.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.