-
-
Notifications
You must be signed in to change notification settings - Fork 519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
flint-1.5.0.p5's extraneous #includes break typedef ulong in sys/types.h #11246
Comments
This comment has been minimized.
This comment has been minimized.
comment:2
FLINT_ASSERT is defined in flint.h and flint_heap_alloc is defined in memory-manager.h. I think the given fix compiles only because the headers are picked up via other inclusions. A fix that seems to work for me is to guard the inclusion of system headers as follows: #undef ulong Note that the flint-1.6 release notes say:
though I don't know if this particular issue is fixed or not. It may be only be an issue with more recent Cygwin than what I tested with. |
comment:3
Replying to @wbhart:
I tried this and it did not work. Another option would be to add double inclusion guards.
Indeed, it's recent. |
comment:4
It would really help if flint did not use "ulong" and switched to using "unsigned long". The use of "ulong" is likely to cause many problems - it is even a reserved word in C#. Just Google "ulong" and you will find other systems using this, so using "ulong" is likely to cause issues. Dave |
comment:5
Replying to @dimpase:
PS. I must have missed an #include somewhere when I tried this. Taking special precautions before doing an #include<std*.h> appears to be a last resort... Anyhow, the patch I have here works, and it removes unneeded |
comment:6
Replying to @sagetrac-drkirkby:
One would rather rename ulong to Ulong. (or even to oolong :-)) I'd rather ask why a preprocessor macro is used in place of a typedef, which ought to be preferred over #define whenever possible, IMHO. |
comment:7
Replying to @wbhart:
1.6 suffers from the same issue:
to get that far when compiling, I needed to remove quite a bit of E.g:
the reason for that is rather than writing
which restricts the scope of i to the for loop, you write
there, in many places. Well, maybe some weird compiler can accept this, but gcc does not... |
comment:8
Dima, is that a Windows 7 problem only? I am trying on XP with Cygwin 1.7.3 and did not have any problems building flint. |
comment:9
Replying to @kcrisman:
Hi, |
comment:10
I misunderstood - I thought you meant Cygwin 1.6, not flint 1.6. Ok. |
Author: Dima Pasechnik |
comment:11
Just a question about the changes - I think I understand getting rid of the files in patch and substituting the .patch files, but I don't see how makepatchfiles is ever used. Was that just a utility you cooked up to make all the patches? I don't know whether that belongs in the spkg or not - I guess it doesn't hurt, but maybe that should be more universal. Anyway, I'm trying this out soon on W7. |
comment:12
Replying to @kcrisman:
indeed, makepatchfiles is a script to create these files, and I use it manually whenever I update the spkg. I have such a |
Reviewer: Karl-Dieter Crisman |
comment:13
This works on XP - Flint builds. Since for now we are only focusing on building :) then this just needs testing on a few other platforms, though it seems you have already done this. Still Solaris? |
comment:14
Replying to @kcrisman:
Sorry, I meant this builds on Windows 7 in addition to XP where this was not a problem. |
comment:16
I think this deserves positive review. Relevant tests pass on Mac and sage.math after this upgrade. I am unable to test this on Solaris, so this is the only potential problem I see, but I don't see any reason it shouldn't work, if getting rid of a few ulong things is better. |
comment:18
I know the FLINT spkg is currently a mess anyway, but
|
comment:19
Replying to @nexttime:
Interestingly, I was just chewed out on another ticket for having cut some lines in that file down to size! It's true that the new lines are just a little over that, apparently.
You mean
Do we ever do error checking for these? Most spkg-install files seem to have just "cp" or "patch". What should be added? |
comment:20
Replying to @kcrisman:
IMHO there is no need to check in spkg-install that patches apply correctly --- going this way, one woud also could start asking for a check that tar worked correctly, etc... |
comment:21
Replying to @dimpase:
Of course you shouldn't cut them, but reformat the text such that none of the lines will exceed 80 characters (same for longer, i.e. all but the first line of commit messages). ;-) If doing so would dominate other, more important changes in the diff, you can still provide a separate patch for that. (There is no patch or spkg-diff for review attached to this ticket anyway.) Dave, me, and others have reformatted any
We do of course check exit codes (or indirectly the success) of Fortunately, we now use
|
comment:55
Replying to @jdemeyer:
Sorry, missed that... ;-) (Fortunately, we now check the exit code of |
comment:56
P.S.: Latest p7 spkg now also really tested with Sage 4.7.1.alpha4 on Ubuntu 9.04 x86_64. P.P.S.: The md5sum will (hopefully) change after the changes have been committed, so that's another argument... |
comment:57
Replying to @nexttime:
Can you take into account the (very minor) change for #11547 in that? The only substantive change is
And this is necessary for Sage to start on Cygwin. I currently had that be p7, but that could be made p8 instead. I guess the point is that we would want this in any further FLINT upgrade, so that we don't have to keep making new spkgs. |
comment:58
Replying to @kcrisman:
Thanks for pointing me to that one. Of course I can include that (though either Do you create a p8 that's based on the latest p7 here? If so, and if not on #11547, please cc me or let me know by other means. |
comment:60
Good point, but luckily the patch has |
comment:61
Replying to @nexttime:
I prefer |
comment:62
Replying to @jdemeyer:
Maybe some Cygwin -- more precisely Windoofs -- flavour... :D
I know, a long lasting discussion. (And the main reason for unsetting it was just a change in autotools which caused some trivial trouble, otherwise using variables might not be necessary, but is always more flexible, since that way one doesn't have to set up special |
comment:63
Replying to @dimpase:
If I understand you correctly (having the whole upstream tree under revision control), that would heavily increase their sizes. I'd rather vote for splitting off the whole vanilla source trees (
That way, we could even have a single spkg (or "spi") repository, which would greatly ease working on spkgs and merging changes. Also, this would not only decrease the download size upon upgrades (assuming just Sage's patch level changed, which is often the case), but also the overall traffic when reviewing spkgs, and -- last but not least -- the "new" spkgs, which could then be ordinary Mercurial changesets to simply be applied to the spkg or "spi" repository, could be attached to the tickets and merged like any other changes to Sage, i.e. the library, scripts etc.). Instead of e.g. |
comment:64
Replying to @nexttime:
Really? Why? If I look at my current $SAGEROOT/devel/sage/, I have 878Mb, while its .hg is 51Mb. And here the fact that this tree has a long history must be taken into account. A typical spkg tree would be started from a clean history, and keep it for most of the files. So it would be a pretty small increase. |
comment:65
Replying to @dimpase:
I don't know which files' sizes you've added up there, but, first of all, a [compressed] source tree with meta information will obviously always be larger than [a compressed] one without it (even when you check out the current versions before calling |
comment:66
Replying to @nexttime:
I just ran 'du' command on '$SAGEROOT/devel/sage/', and on '$SAGEROOT/devel/sage/.hg'.
I am not talking about carrying around the whole upstream history, which may or may not be available. If upstream upgrades in a clean patch fashion, then it's easy to import their patches and carry them on. Otherwise we basically would throw the history away, just as we do presently, and start anew. |
comment:67
Replying to @dimpase:
Well, then you've counted also all generated files ( Also, since Mercurial stores its files compressed, for a fair comparison you should have compared a compressed version of |
comment:68
Replying to @dimpase:
Either you put Or did you mean putting I cannot really see an advantage in either way. |
comment:69
I suggest this discussion belongs on sage-devel :-) as I'm sure others have opinions as well. |
comment:70
Replying to @kcrisman:
Just post a link to this ticket there. XD (A BBS would IMHO be better for such discussions. The only advantage of G**gle groups over trac is that the former supports concurrent posting.) |
comment:71
Back on topic, for the record: leif@portland:~/Sage/spkgs$ du -sch `find flint-1.5.0.p7/ -name .svn`
140K flint-1.5.0.p7/src/zn_poly/test/.svn
432K flint-1.5.0.p7/src/zn_poly/src/.svn
148K flint-1.5.0.p7/src/zn_poly/include/.svn
76K flint-1.5.0.p7/src/zn_poly/demo/bernoulli/.svn
44K flint-1.5.0.p7/src/zn_poly/demo/.svn
96K flint-1.5.0.p7/src/zn_poly/tune/.svn
140K flint-1.5.0.p7/src/zn_poly/profile/.svn
48K flint-1.5.0.p7/src/zn_poly/doc/.svn
1.1M total
leif@portland:~/Sage/spkgs$ |
comment:72
Again slightly off-topic, just as an example: ~/flint-1.5.0.p7$ mv src/ src-plain
~/flint-1.5.0.p7$ cp -pr src-plain/ src-hg
~/flint-1.5.0.p7$ diff -r src-plain/ src-hg/
~/flint-1.5.0.p7$ (cd src-hg/ && hg init && hg add >/dev/null && hg commit -m "initial revision")
~/flint-1.5.0.p7$ (cd src-hg/ && rm -rf *)
~/flint-1.5.0.p7$ for d in src-*; do echo -n "$d: "; tar cjf - $d | wc -c; done
src-hg: 1015130
src-plain: 754703 (With the |
comment:73
P.S.: For all standard spkgs (327 MB), taking the ratio for FLINT (1.345), this would mean about +113 MB, which IMHO would be an unreasonable increase. (And that doesn't even take into account the history for changes made by Sage to upstream releases.) |
comment:74
New version with fully committed changes, same location: http://boxen.math.washington.edu/home/jdemeyer/spkg/flint-1.5.0.p7.spkg You can use this to base a .p8 on. |
comment:75
Unfortunately, because of the sage.math cluster failure, this is not currently accessible. Given that the actual relevant part of https://github.com/sagemath/sage/files/ticket11547/trac_11547-flint.diff.gz is
and
(where the then perhaps someone could be kind when this comes back online and make the p8 there. It would be very, very good to have all of the stuff at #6743 merged as soon as possible so we don't go backwards. I'm unfortunately about to go largely offline for about two weeks, so I will not be around when p7 becomes available (at least, I don't think I'm going to have good access). |
comment:76
Replying to @kcrisman:
I've made a p8 with (only) these changes based on the latest p7 from here and uploaded it onto G**gle code. See #11547 for details in a few minutes; I'll also update #6743 accordingly.
Well, being offline isn't always unfortunate. ;-) Have a good time then. |
comment:77
Thanks, I appreciate that a lot. I keep all my spkgs on sage.math only, because I don't want to clutter up my computer, but maybe that was a bad decision this time around :) I also could use the spkg-upload code project more often, it's true... By the way, what's up with the
Good point. Besonders wenn man in die Heimat fliegt. |
comment:78
Replying to @kcrisman:
Don't know. I just can't spell it.
Na denn guten Flug. Internet gibt's hier allerdings mancherorts. :-) |
Merged: sage-4.7.2.alpha0 |
flint-1.5.0.p5 does not build on a recent Cygwin (1.7.9)
due to Cygwin having ulong defined as type in
/usr/include/sys/types.h. This results in
note that
/usr/include/sys/types.h:101
is
ulong is a macro in flint, defined in two places, for some reason:
in flint.h and in profiler.h
Cause:
The header inclusion order in ZmodF_mul.c
does #include ZmodF_poly.h (which includes stdio.h, which in turn
includes sys/types.h containing the typedef for ulong)
after ZmodF.h, which includes flint.h containing #define ulong
Thus the typedef is nuked, and there is no way around it with the
given header order.
The 1st bug is in ZmodF_mul.c, which does not need to include ZmodF.h
at all,
as it is included in ZmodF_poly.h
The 2nd bug like this is in mpn_extras.h, where the inclusion of
flint.h is not needed
---and this nukes ZmodF_mul-tuning.c
The 3rd bug like this is in ZmodF_poly.c, which needs to include
neither flint.h nor memory-manager.h
See http://groups.google.com/group/sage-windows/browse_thread/thread/294895626ba6faf1
New spkg: http://boxen.math.washington.edu/home/jdemeyer/spkg/flint-1.5.0.p7.spkg
CC: @sagetrac-drkirkby @nexttime
Component: packages: standard
Author: Dima Pasechnik, Jeroen Demeyer
Reviewer: Karl-Dieter Crisman, Leif Leonhardy
Merged: sage-4.7.2.alpha0
Issue created by migration from https://trac.sagemath.org/ticket/11246
The text was updated successfully, but these errors were encountered: