dimpase commented 13 years ago

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

gcc -std=c99 -I/home/dima/sage-4.7.alpha5/local/include/ -I/home/dima/sage-4.7.a
lpha5/local/include  -fPIC -funroll-loops   -O2 -c ZmodF_mul.c -o ZmodF_mul.o
ZmodF_mul.c:1: warning: -fPIC ignored for target (all code is position independe
nt)
In file included from /usr/include/stdio.h:46,
                 from ZmodF_poly.h:40,
                 from ZmodF_mul.c:34:
/usr/include/sys/types.h:101: error: duplicate `unsigned'
make[2]: *** [ZmodF_mul.o] Error 1

note that /usr/include/sys/types.h:101 is

typedef unsigned long   ulong;          /* System V compatibility */

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

dimpase commented 13 years ago

Description changed:

--- 
+++ 
@@ -24,6 +24,69 @@
 ulong is a macro in flint, defined in two places, for some reason:
 in flint.h and in profiler.h

-Removing these defines makes the package building OK.
-A crude patch should be trivial.
+The patched spkg is in 
+http://boxen.math.washington.edu/home/dima/packages/flint-1.5.0.p6.spkg

+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
+
+The patches in the spkg are as follows:
+
+```
+--- src/mpn_extras.h    2009-09-23 18:03:27.000000000 +0800
++++ patches/mpn_extras.h        2011-04-25 22:36:33.000000000 +0800
+@@ -22,7 +22,6 @@
+ #ifndef MPN_EXTRAS_H
+ #define MPN_EXTRAS_H
+ 
+-#include "flint.h"
+ #include "ZmodF_poly.h"
+ 
+ #include "longlong_wrapper.h"
+
+--- src/ZmodF_poly.c    2009-09-23 18:03:27.000000000 +0800
++++ patches/ZmodF_poly.c        2011-04-25 22:37:17.000000000 +0800
+@@ -29,8 +29,6 @@
+ 
+ *****************************************************************************/
+ 
+-#include "flint.h"
+-#include "memory-manager.h"
+ #include "ZmodF_poly.h"
+ #include "ZmodF_mul.h"
+ #include "fmpz_poly.h"
+
+--- src/ZmodF_mul.c     2009-09-23 18:03:27.000000000 +0800
++++ patches/ZmodF_mul.c 2011-04-25 22:32:40.000000000 +0800
+@@ -30,7 +30,6 @@
+ ******************************************************************************/
+ 
+ #include <math.h>
+-#include "ZmodF.h"
+ #include "ZmodF_poly.h"
+ #include "ZmodF_mul.h"
+ #include "mpn_extras.h"
+```
+
+tested on Linux (x64-86), MacOSX 10.6, and Cygwin (1.7.9)
+
+

wbhart commented 13 years ago

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

include

define ulong unsigned long

Note that the flint-1.6 release notes say:

Better Cygwin support

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.

dimpase commented 13 years ago

comment:3

Replying to @wbhart:

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
#include <stdlib.h>
#define ulong unsigned long

I tried this and it did not work.

Another option would be to add double inclusion guards.

Note that the flint-1.6 release notes say:

Better Cygwin support

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.

Indeed, it's recent.

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 13 years ago

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

dimpase commented 13 years ago

comment:5

Replying to @dimpase:

I tried this and it did not work.

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 duplicate inclusions from the code, rather than adds more lines. What are objections to it?

dimpase commented 13 years ago

comment:6

Replying to @sagetrac-drkirkby:

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.

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.

dimpase commented 13 years ago

comment:7

Replying to @wbhart:

Note that the flint-1.6 release notes say:

Better Cygwin support

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.

1.6 suffers from the same issue:

...
cc -std=c99 -I -I   -g -O2 -c ZmodF_mul.c -o ZmodF_mul.o
In file included from /usr/include/stdio.h:46,
                 from ZmodF_poly.h:40,
                 from ZmodF_mul.c:34:
/usr/include/sys/types.h:101: error: duplicate `unsigned'
make: *** [ZmodF_mul.o] Error 1

dima@SPMS-DIMA-W7 /tmp/flint-1.6

to get that far when compiling, I needed to remove quite a bit of "error: redeclaration of `i' with no linkage" in QS/mp_linear_algebra.c and QS/mp_sieve.c

E.g:

QS/mp_linear_algebra.c: In function `insert_lp_relation':
QS/mp_linear_algebra.c:334: error: redeclaration of `i' with no linkage
QS/mp_linear_algebra.c:329: error: previous declaration of `i' was here

the reason for that is rather than writing

 for (unsigned long i = 0; ...

which restricts the scope of i to the for loop, you write

 unsigned long i;
 for (i = 0; ...

there, in many places. Well, maybe some weird compiler can accept this, but gcc does not...

kcrisman commented 13 years ago

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.

dimpase commented 13 years ago

comment:9

Replying to @kcrisman:

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.

Hi, you need a more recent Cygwin! Dima

kcrisman commented 13 years ago

comment:10

I misunderstood - I thought you meant Cygwin 1.6, not flint 1.6. Ok.

kcrisman commented 13 years ago

Author: Dima Pasechnik

kcrisman commented 13 years ago

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.

dimpase commented 13 years ago

comment:12

Replying to @kcrisman:

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.

indeed, makepatchfiles is a script to create these files, and I use it manually whenever I update the spkg. I have such a file in at least one more package (cvxopt).

kcrisman commented 13 years ago

Reviewer: Karl-Dieter Crisman

kcrisman commented 13 years ago

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?

kcrisman commented 13 years ago

comment:14

Replying to @kcrisman:

This works on XP - Flint builds. Since for now we are only focusing on building :)

Sorry, I meant this builds on Windows 7 in addition to XP where this was not a problem.

kcrisman commented 13 years ago

comment:15

Replying to @kcrisman:

Replying to @kcrisman:

This works on XP - Flint builds. Since for now we are only focusing on building :)

Sorry, I meant this builds on Windows 7 in addition to XP where this was not a problem.

Because of an old Cygwin, presumably.

kcrisman commented 13 years ago

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.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

comment:18

I know the FLINT spkg is currently a mess anyway, but

(in general) please add the ticket number to commit messages, and limit SPKG.txt lines to 80 columns;
the patches are applied without any error checking.

kcrisman commented 13 years ago

comment:19

Replying to @nexttime:

I know the FLINT spkg is currently a mess anyway, but

(in general) please add the ticket number to commit messages, and limit SPKG.txt lines to 80 columns;

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.

the patches are applied without any error checking.

You mean

+for j in `ls patches/*.patch` ; do  
+   patch -p0 < $j 
+done

Do we ever do error checking for these? Most spkg-install files seem to have just "cp" or "patch". What should be added?

dimpase commented 13 years ago

comment:20

Replying to @kcrisman:

Replying to @nexttime:

I know the FLINT spkg is currently a mess anyway, but

(in general) please add the ticket number to commit messages, and limit SPKG.txt lines to 80 columns;

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.

the patches are applied without any error checking.

You mean
+for j in `ls patches/*.patch` ; do  
+   patch -p0 < $j 
+done 
Do we ever do error checking for these? Most spkg-install files seem to have just "cp" or "patch". What should be added?

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

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

comment:21

Replying to @dimpase:

Replying to @kcrisman:

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.

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 SPKG.txt we came across in the past in case it was necessary. Once overlong lines are eliminated, it's much more likely people will intuitively adhere without ever looking at their editor's status bar.

the patches are applied without any error checking.

You mean

+for j in `ls patches/*.patch` ; do  
+   patch -p0 < $j 
+done

Do we ever do error checking for these? Most spkg-install files seem to have just "cp" or "patch". What should be added?

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

We do of course check exit codes (or indirectly the success) of tar and other commands that can spectacularly fail...

Fortunately, we now use patch rather than cp, which at least spits out warning or error messages in case upstream and patches/* get out of sync (or someone made another mistake), but these messages are easily missed - even by a reviewer, which can lead to all kinds of obscure errors or misbehavior any time later, so we should IMHO check $? there.

bash's not Python, where we would (hopefully) get a nice, instructive traceback.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

comment:23

Replying to @kcrisman:

Do we ever do error checking for these? Most spkg-install files seem to have just "cp" or "patch". What should be added?

(cp will almost always succeed, so that's more or less irrelevant, meaning a different situation.)

From the "proof-of-concept" spkg (Sphinx):

# Apply patches
cd "$CUR/src"
echo "Patching Sphinx..."
for p in ../patches/*.patch; do
        patch -p1 <$p
        success "Error applying patch $p"
done

(The success function exits spkg-install whenever $? != 0.)

It would IMHO also be valid to just accumulate errors and do one check after the loop, e.g.:

patched_ok=true

for p in patches/*.patch; do
  patch -p0 < "$p" || patched_ok=false  # -p0 is also superfluous btw.
done

if ! $patched_ok; then
  echo >&2 "Error applying the patches to upstream source" # or whatever
  exit 1
fi

(Note that doing ls patches/*.patch is superfluous and not even robust.)

You can of course at your taste "invert" the logic, i.e. start with patch_error=false and set it to true in case patch returns a non-zero value, then test if $patch_error; then ....

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

comment:24

For the record:

9419 ("Update Developers Guide to state how patches should be made.") - intended to explain how to use `patch` in spkgs - is unfortunately still "new"...

kcrisman commented 13 years ago

comment:25

Replying to @nexttime:

Replying to @dimpase:

Replying to @kcrisman:

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.

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

This is exactly what I did, and was rebuked.

kcrisman commented 13 years ago

comment:26

Replying to @nexttime:

Replying to @kcrisman:

Do we ever do error checking for these? Most spkg-install files seem to have just "cp" or "patch". What should be added?

(cp will almost always succeed, so that's more or less irrelevant, meaning a different situation.)

From the "proof-of-concept" spkg (Sphinx):
# Apply patches
cd "$CUR/src"
echo "Patching Sphinx..."
for p in ../patches/*.patch; do
        patch -p1 <$p
        success "Error applying patch $p"
done
(The success function exits spkg-install whenever $? != 0.)

This sounds like a great idea on a different ticket. Getting the Cygwin things in is going to be hard enough!

Unless you are volunteering to test the rest of them! That would be very convenient :)

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

comment:27

Replying to @kcrisman:

Replying to @nexttime:

Replying to @dimpase:

Replying to @kcrisman:

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.

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

This is exactly what I did, and was rebuked.

Where? By whom? D-:<

No, honestly, if someone refused what others consider a common agreement this should IMHO be discussed on sage-devel.

dimpase commented 13 years ago

comment:28

Replying to @nexttime:

Replying to @kcrisman:

Replying to @nexttime:

Replying to @dimpase:

Replying to @kcrisman:

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.

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

This is exactly what I did, and was rebuked.

Where? By whom? D-:<

No, honestly, if someone refused what others consider a common agreement this should IMHO be discussed on sage-devel.

probably Karl-Dieter means my comment somewhere, which was more in the spirit of "please do not reformat the files unnecessarily, to decrease lengths of patches"; I actually wasn't aware of the 80-characters rule...

jdemeyer commented 13 years ago

comment:29

To clear some things up:

It is not needed anymore to add ticket numbers to commit messages (this used to be the case but this was changed in sage-4.7.alpha5)
Lines in SPKG.txt and commit messages should not be too long. There is no hard 80 characters limit but more of a subjective "not too long" limit. In practice I would say <= 120 characters is safe.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

comment:30

Replying to @dimpase:

probably Karl-Dieter means my comment somewhere, which was more in the spirit of "please do not reformat the files unnecessarily, to decrease lengths of patches"; I actually wasn't aware of the 80-characters rule...

The ugly thing with reformatting text is that the diffs usually get hard to read.

I therefore prefer splitting off such changes from code changes, to be reviewed separately and independently, since the latter are much more important.

Nevertheless, both kinds of patches can (and IMHO should) live on the same ticket, because documentation tends to get neglected.

Replying to @jdemeyer:

Lines in SPKG.txt and commit messages should not be too long. There is no hard 80 characters limit but more of a subjective "not too long" limit. In practice I would say <= 120 characters is safe.

I would agree in the case of source code (which you read with your favourite editor), but not documentation that's often just dumped to a terminal (sage -info ..., hg log -v, or simply sage: function?).

Of course one can usually resize the terminal window (perhaps at the expense of redoing the command), but that's quite inconvenient, and there's a reason e.g. newspapers -- and even a lot of blogs -- are typeset as they are: in multiple, rather narrow columns.

Code is somewhat different, also depending on the language and coding style.

It is not needed anymore to add ticket numbers to commit messages (this used to be the case but this was changed in sage-4.7.alpha5)

Ooops, there's not just a technical reason for doing so. That's why I insist on having them in spkg commit messages as well (alternatively, or in addition, the full spkg version).

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

comment:31

Finally: ;-)

Replying to @kcrisman:

Replying to @nexttime: From the "proof-of-concept" spkg (Sphinx):

# Apply patches
cd "$CUR/src"
echo "Patching Sphinx..."
for p in ../patches/*.patch; do
        patch -p1 <$p
        success "Error applying patch $p"
done

(The success function exits spkg-install whenever $? != 0.)

This sounds like a great idea on a different ticket. Getting the Cygwin things in is going to be hard enough!

<scnr><flame> Well, I didn't set the ticket to "needs work", though the more I think about it, the more I'm convinced we shouldn't start(?) accepting code that literally sets up traps the next ones touching the spkg can easily step into, with the potential to cause virtually any kind of obscure, hardly backtraceable error or malfunction.

And perhaps thereby encouraging others in both also omitting checks, and letting others do so.

Murphy's Law tells us it will happen, sooner or later... And all just because someone omitted -- knowingly -- a few simple error checks... :D (The "redundant" for j in `ls patches/*.patch` btw has also a nice side-effect; try it with some patch with spaces in its filename... Ok, always a bad idea, wasn't it?)

Ok, we can always "fix" it later, on another ticket. Although this ticket has meanwhile already been postponed to another milestone. And the changes would be almost trivial.

Procrastination makes things never happen. </flame></scnr>

Sorry, had to let the words flow, get rid of some frustration.

Unless you are volunteering to test the rest of them! That would be very convenient :)

I would if I only could, but I fortunately^TM lack suitable Windows versions to run a recent Cygwin on. ;-)

kcrisman commented 13 years ago

comment:32

Well, I didn't set the ticket to "needs work", though the more I think about it, the more I'm convinced we shouldn't start(?) accepting code that literally sets up traps the next ones touching the spkg can easily step into, with the potential to cause virtually any kind of obscure, hardly backtraceable error or malfunction.

You may be right.

Murphy's Law tells us it will happen, sooner or later... And all just because someone omitted -- knowingly -- a few simple error checks... :D

Except for the fact that in this case the reviewer at least did it unknowingly. Which is the fundamental difference between Sage and most other OS projects - the developers are mostly mathematicians first, programmers second (or third, or nth). My apologies for not being a shell script ninja ;)

Procrastination makes things never happen.

Precisely! That is why I reviewed this ticket positively - if I'd waited for someone who knew enough about shell to check whether it did more than work correctly it would have never been reviewed :)

I would if I only could, but I fortunately^TM lack suitable Windows versions to run a recent Cygwin on. ;-)

Ah, another chicken-and-egg problem...

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

comment:33

Replying to @kcrisman:

Except for the fact that in this case the reviewer at least did it unknowingly.

I primarily meant merging tickets with known newly introduced flaws or deficiencies, which of course requires some positive review.

(Not fixing old flaws on a ticket is another issue, and heavily depends on the situation, especially the effort to do so, and the cost of delaying other changes already made. Also, spkgs are IMHO different to patches to the rest of Sage.)

Which is the fundamental difference between Sage and most other OS projects - the developers are mostly mathematicians first, programmers second (or third, or nth). My apologies for not being a shell script ninja ;)

Ninjas operate silently.

If I'd waited for someone who knew enough about shell to check whether it did more than work correctly it would have never been reviewed :)

Hmmm, I saw Dave was cc'ed.

I would if I only could, but I fortunately^TM lack suitable Windows versions to run a recent Cygwin on. ;-)

Ah, another chicken-and-egg problem...

Chicken and egg? So Sage on Cygwin will replace Windows, or is Microsoft going to rewrite it in Sage when the Cygwin port is ready?

kcrisman commented 13 years ago

comment:34

I would if I only could, but I fortunately^TM lack suitable Windows versions to run a recent Cygwin on. ;-)

Ah, another chicken-and-egg problem...

Chicken and egg? So Sage on Cygwin will replace Windows, or is Microsoft going to rewrite it in Sage when the Cygwin port is ready?

No, that Sage on Windows (Cygwin or otherwise) won't happen until we have enough Windows-adept users, but we are unlikely to have many of them until we have a good Sage on Windows that isn't a VM/VB solution, and so on.

jdemeyer commented 13 years ago

comment:35

I will have a look at spkg-install and clean up where needed...

jdemeyer commented 13 years ago

Description changed:

--- 
+++ 
@@ -24,9 +24,6 @@
 ulong is a macro in flint, defined in two places, for some reason:
 in flint.h and in profiler.h

-The patched spkg is in 
-http://boxen.math.washington.edu/home/dima/packages/flint-1.5.0.p6.spkg
-
 Cause:

 The header inclusion order in ZmodF_mul.c 
@@ -49,44 +46,4 @@

 See http://groups.google.com/group/sage-windows/browse_thread/thread/294895626ba6faf1

-The patches in the spkg are as follows:
-
-```
---- src/mpn_extras.h    2009-09-23 18:03:27.000000000 +0800
-+++ patches/mpn_extras.h        2011-04-25 22:36:33.000000000 +0800
-@@ -22,7 +22,6 @@
- #ifndef MPN_EXTRAS_H
- #define MPN_EXTRAS_H
- 
--#include "flint.h"
- #include "ZmodF_poly.h"
- 
- #include "longlong_wrapper.h"
-
---- src/ZmodF_poly.c    2009-09-23 18:03:27.000000000 +0800
-+++ patches/ZmodF_poly.c        2011-04-25 22:37:17.000000000 +0800
-@@ -29,8 +29,6 @@
- 
- *****************************************************************************/
- 
--#include "flint.h"
--#include "memory-manager.h"
- #include "ZmodF_poly.h"
- #include "ZmodF_mul.h"
- #include "fmpz_poly.h"
-
---- src/ZmodF_mul.c     2009-09-23 18:03:27.000000000 +0800
-+++ patches/ZmodF_mul.c 2011-04-25 22:32:40.000000000 +0800
-@@ -30,7 +30,6 @@
- ******************************************************************************/
- 
- #include <math.h>
--#include "ZmodF.h"
- #include "ZmodF_poly.h"
- #include "ZmodF_mul.h"
- #include "mpn_extras.h"
-```
-
-tested on Linux (x64-86), MacOSX 10.6, and Cygwin (1.7.9)
-
-
+New spkg: [http://boxen.math.washington.edu/home/jdemeyer/spkg/flint-1.5.0.p7.spkg](http://boxen.math.washington.edu/home/jdemeyer/spkg/flint-1.5.0.p7.spkg)

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

comment:38

If you quote $p and commit the changes ;-) I'll give it positive review...

I wouldn't have removed the comment on linking in the NTL interface though (which is a very bad thing btw.).

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

comment:39

Replying to @nexttime:

I wouldn't have removed the comment on linking in the NTL interface though (which is a very bad thing btw.).

Otherwise we should add the "Patches" subsection to SPKG.txt.

I'm going to provide a FLINT 1.5.2 spkg anyway soon (cf. #9858), already made some changes months ago.

kcrisman commented 13 years ago

comment:40

Yeah, I couldn't find anything in the log.

Leif, he moved that comment to the SPKG.txt instructions, so it should be okay.

kcrisman commented 13 years ago

Changed author from Dima Pasechnik to Dima Pasechnik, Jeroen Demeyer

kcrisman commented 13 years ago

Changed reviewer from Karl-Dieter Crisman to Karl-Dieter Crisman, Leif Leonhardy

kcrisman commented 13 years ago

comment:42

Here's another question: where is this "success" function? Is it defined elsewhere in the Sphinx spkg?

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

comment:43

Replying to @kcrisman:

Here's another question: where is this "success" function? Is it defined elsewhere in the Sphinx spkg?

At the beginning of spkg-install:

# Helper functions
success() {
    if [ $? -ne 0 ]; then
        echo "Error building Sphinx: '$1'"
        exit 1
    fi
}

jdemeyer commented 13 years ago

comment:44

New version up at same location. Also removes obsolete dist/debian directory.

jdemeyer commented 13 years ago

comment:45

Replying to @kcrisman:

Leif, he moved that comment to the SPKG.txt instructions, so it should be okay.

Indeed, I think such comments belong in SPKG.txt, not in spkg-install.

jdemeyer commented 13 years ago

comment:46

Replying to @nexttime:

If you quote $p

Done

and commit the changes

This is no longer necessary, the changes will be committed when the spkg is merged.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

comment:47

Replying to @jdemeyer:

Replying to @kcrisman:

Leif, he moved that comment to the SPKG.txt instructions, so it should be okay.

Indeed, I think such comments belong in SPKG.txt, not in spkg-install.

The Patches section...

I think if you apply them "manually" (i.e. not in a loop, which would be necessary if order matters), there should also be a comment (in spkg-install) which issue a specific patch commands fixes.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

comment:48

Replying to @jdemeyer:

Replying to @nexttime:

and commit the changes

This is no longer necessary, the changes will be committed when the spkg is merged.

WTF? In whose name, and what will be the commit message?

I'm strongly against "generic" commit messages, even if the tags (with the spkg version / patch level, perhaps ticket number) were complete, i.e. referring to the Changelog. Each Changelog entry is somewhat cumulative, and usually more high-level, while commit messages may be more specific (especially if they consist of more than one line).

So I wouldn't encourage people to not commit their changes.

Also, having multiple commits in the same spkg version is useful (and not uncommon, even by a single developer) if the changes are (perhaps bigger and) rather unrelated.

Then having the last change uncommitted would be a bit inconsistent.

Also, I think sage -pkg ... (does anybody use that? :-) ) would at least currently reject an spkg with uncommitted changes, which isn't all bad.

dimpase commented 13 years ago

comment:49

Replying to @nexttime:

Replying to @jdemeyer:

Replying to @nexttime:

and commit the changes

This is no longer necessary, the changes will be committed when the spkg is merged.

WTF? In whose name, and what will be the commit message?

I'm strongly against "generic" commit messages, even if the tags (with the spkg version / patch level, perhaps ticket number) were complete, i.e. referring to the Changelog. Each Changelog entry is somewhat cumulative, and usually more high-level, while commit messages may be more specific (especially if they consist of more than one line).

So I wouldn't encourage people to not commit their changes.

IMHO, this discussion just highlights a need to revamp the whole spkg system, for 99% of the standard spkgs, at least. It would be much better if the whole non-building part of the spkg install is done in a uniform way, by hg, say, rather than handwritten scripts...

The need for the current spkg system is dictated by the need of some optional parts of Sage, that demand, say, a distribution in an unmodified form. Yet, putting the spkg source under hg is not a modification. Sage can distribute spkg source with the hg tree, and Sage-needed patches as patches to be applied by an spkg-install.

Also, having multiple commits in the same spkg version is useful (and not uncommon, even by a single developer) if the changes are (perhaps bigger and) rather unrelated.

Then having the last change uncommitted would be a bit inconsistent.

Also, I think sage -pkg ... (does anybody use that? :-) ) would at least currently reject an spkg with uncommitted changes, which isn't all bad.

no, that's not true. And in fact sage -pkg should not reject such an spkg, as you want to debug your spkg before committing changes.

Dima

jdemeyer commented 13 years ago

comment:50

Replying to @nexttime:

Replying to @jdemeyer:

Replying to @nexttime:

and commit the changes

This is no longer necessary, the changes will be committed when the spkg is merged.

what will be the commit message?

The commit message will be the complete SPKG.txt entry, so in this case:

=== flint-1.5.0.p7 (Jeroen Demeyer, 6 July 2011) ===
 * Trac #11246: remove check for gcc version since we require gcc >= 4.0.1
   for Sage anyway.
 * Use `patch` instead of `cp` for patching the makefile
 * Remove test_gcc_version.sh and the horrible makepatchfiles
 * Check that `patch` succeeded in spkg-install, apply patches at -p1 level
 * Remove obsolete dist/debian directory

Then having the last change uncommitted would be a bit inconsistent.

Well, you can always commit all changes yourself, there is no harm. I personally prefer not to commit any changes until the spkg is really ready to be shipped.

Also, I think sage -pkg ... would at least currently reject an spkg with uncommitted changes, which isn't all bad.

It does not reject it, it merely notifies the user that there are uncomitted changes.

sagemath / sage

flint-1.5.0.p5's extraneous #includes break typedef ulong in sys/types.h #11246

undef ulong

include

define ulong unsigned long

9419 ("Update Developers Guide to state how patches should be made.") - intended to explain how to use `patch` in spkgs - is unfortunately still "new"...

sagemath / sage

flint-1.5.0.p5's extraneous #includes break typedef ulong in sys/types.h #11246

undef ulong

include

define ulong unsigned long

9419 ("Update Developers Guide to state how patches should be made.") - intended to explain how to use patch in spkgs - is unfortunately still "new"...

9419 ("Update Developers Guide to state how patches should be made.") - intended to explain how to use `patch` in spkgs - is unfortunately still "new"...