Opened 10 years ago

Closed 10 years ago

#11246 closed defect (fixed)

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

Reported by: dimpase Owned by: tbd
Priority: major Milestone: sage-4.7.2
Component: packages: standard Keywords: cygwin
Cc: drkirkby, leif Merged in: sage-4.7.2.alpha0
Authors: Dima Pasechnik, Jeroen Demeyer Reviewers: Karl-Dieter Crisman, Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

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

Attachments (1)

flint-1.5.0.p6-p7.diff (58.3 KB) - added by jdemeyer 10 years ago.
Diff between the .p6 and .p7 versions, for review only

Download all attachments as: .zip

Change History (80)

comment:1 Changed 10 years ago by dimpase

  • Cc drkirkby added
  • Description modified (diff)
  • Status changed from new to needs_review
  • Summary changed from flint-1.5.0.p5 defines ulong on a system with ulong a type (e.g. Cygwin) to flint-1.5.0.p5's extraneous #includes break typedef ulong in sys/types.h

comment:2 follow-ups: Changed 10 years ago by 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

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.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 10 years ago by dimpase

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.

comment:4 follow-up: Changed 10 years ago by 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.

Dave

comment:5 in reply to: ↑ 3 Changed 10 years ago by dimpase

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?

comment:6 in reply to: ↑ 4 Changed 10 years ago by dimpase

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

comment:7 in reply to: ↑ 2 Changed 10 years ago by dimpase

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

comment:8 follow-up: Changed 10 years ago by 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.

comment:9 in reply to: ↑ 8 Changed 10 years ago by dimpase

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

comment:10 Changed 10 years ago by kcrisman

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

comment:11 follow-up: Changed 10 years ago by kcrisman

  • Authors set to Dima Pasechnik

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 in reply to: ↑ 11 Changed 10 years ago by dimpase

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

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

  • Reviewers set to Karl-Dieter Crisman

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 in reply to: ↑ 13 ; follow-up: Changed 10 years ago by 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.

comment:15 in reply to: ↑ 14 Changed 10 years ago by kcrisman

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.

comment:16 Changed 10 years ago by kcrisman

  • Status changed from needs_review to positive_review

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:17 Changed 10 years ago by leif

  • Cc leif added

comment:18 follow-up: Changed 10 years ago by leif

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.

comment:19 in reply to: ↑ 18 ; follow-ups: Changed 10 years ago by kcrisman

Replying to leif:

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?

comment:20 in reply to: ↑ 19 ; follow-up: Changed 10 years ago by dimpase

Replying to kcrisman:

Replying to leif:

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

comment:21 in reply to: ↑ 20 ; follow-up: Changed 10 years ago by leif

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.

comment:22 Changed 10 years ago by jdemeyer

  • Keywords cygwin added
  • Milestone changed from sage-4.7.1 to sage-4.7.2

comment:23 in reply to: ↑ 19 ; follow-up: Changed 10 years ago by leif

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

comment:24 Changed 10 years ago by leif

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

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

Replying to leif:

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.

comment:26 in reply to: ↑ 23 Changed 10 years ago by kcrisman

Replying to leif:

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 :)

comment:27 in reply to: ↑ 25 ; follow-up: Changed 10 years ago by leif

Replying to kcrisman:

Replying to leif:

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.

comment:28 in reply to: ↑ 27 Changed 10 years ago by dimpase

Replying to leif:

Replying to kcrisman:

Replying to leif:

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

comment:29 Changed 10 years ago by jdemeyer

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.

comment:30 Changed 10 years ago by leif

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

comment:31 follow-up: Changed 10 years ago by leif

Finally: ;-)

Replying to kcrisman:

Replying to leif: 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 fortunatelyTM lack suitable Windows versions to run a recent Cygwin on. ;-)

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

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 fortunatelyTM lack suitable Windows versions to run a recent Cygwin on. ;-)

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

comment:33 in reply to: ↑ 32 ; follow-up: Changed 10 years ago by leif

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 fortunatelyTM 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?

comment:34 in reply to: ↑ 33 Changed 10 years ago by kcrisman

I would if I only could, but I fortunatelyTM 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.

comment:35 Changed 10 years ago by jdemeyer

  • Status changed from positive_review to needs_work

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

comment:36 Changed 10 years ago by jdemeyer

  • Description modified (diff)

comment:37 Changed 10 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:38 follow-ups: Changed 10 years ago by leif

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

comment:39 in reply to: ↑ 38 Changed 10 years ago by leif

Replying to leif:

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.

comment:40 follow-up: Changed 10 years ago by kcrisman

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

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

comment:41 Changed 10 years ago by kcrisman

  • Authors changed from Dima Pasechnik to Dima Pasechnik, Jeroen Demeyer
  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Leif Leonhardy

comment:42 follow-up: Changed 10 years ago by kcrisman

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

comment:43 in reply to: ↑ 42 Changed 10 years ago by leif

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
}

comment:44 Changed 10 years ago by jdemeyer

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

comment:45 in reply to: ↑ 40 ; follow-up: Changed 10 years ago by 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.

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

Replying to leif:

If you quote $p

Done

and commit the changes

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

comment:47 in reply to: ↑ 45 Changed 10 years ago by leif

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.

comment:48 in reply to: ↑ 46 ; follow-ups: Changed 10 years ago by leif

Replying to jdemeyer:

Replying to leif:

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.

comment:49 in reply to: ↑ 48 Changed 10 years ago by dimpase

Replying to leif:

Replying to jdemeyer:

Replying to leif:

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

comment:50 in reply to: ↑ 48 Changed 10 years ago by jdemeyer

Replying to leif:

Replying to jdemeyer:

Replying to leif:

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.

comment:51 follow-up: Changed 10 years ago by leif

  • Status changed from needs_review to positive_review

Except for not having committed the changes (1 rubber point deduction), positive review from me.

As I mentioned, I'll make further changes on a follow-up for FLINT 1.5.2.

comment:52 follow-up: Changed 10 years ago by jdemeyer

  • Status changed from positive_review to needs_work

There was a problem with Unix vs. DOS newlines in patches/mpn_extras.h.patch. New version up for review, same location.

comment:53 Changed 10 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Changed 10 years ago by jdemeyer

Diff between the .p6 and .p7 versions, for review only

comment:54 Changed 10 years ago by leif

Replying to jdemeyer:

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.

Well, as I said, I just wouldn't encourage others to proceed like that.

(I know it might sometimes be tedious to undo some changes, but any spkgs advertised on a ticket should IMHO have already all changes committed, at least if it's "needs review". And having multiple, independent patches to an spkg on the ticket for review can be advantageous.)

comment:55 in reply to: ↑ 52 Changed 10 years ago by leif

  • Status changed from needs_review to positive_review

Replying to jdemeyer:

There was a problem with Unix vs. DOS newlines in patches/mpn_extras.h.patch.

Sorry, missed that... ;-)

(Fortunately, we now check the exit code of patch. :-) )

comment:56 Changed 10 years ago by leif

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 in reply to: ↑ 51 ; follow-up: Changed 10 years ago by kcrisman

Replying to leif:

Except for not having committed the changes (1 rubber point deduction), positive review from me.

As I mentioned, I'll make further changes on a follow-up for FLINT 1.5.2.

Can you take into account the (very minor) change for #11547 in that? The only substantive change is

if [ $UNAME = "CYGWIN" ]; then 
    <snip>
   CP -p libflint.so libflint.dll 

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 in reply to: ↑ 57 ; follow-ups: Changed 10 years ago by leif

Replying to kcrisman:

Can you take into account the (very minor) change for #11547 in that? The only substantive change is

if [ $UNAME = "CYGWIN" ]; then 
    <snip>
   CP -p libflint.so libflint.dll 

Thanks for pointing me to that one. Of course I can include that (though either $CP or cp); I'll have to rebase the changes I made so far anyway.

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:59 in reply to: ↑ 58 Changed 10 years ago by kcrisman

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.

No, I haven't, but if I would, it would be on #11547, where you have added yourself.

comment:60 in reply to: ↑ 58 Changed 10 years ago by kcrisman

Thanks for pointing me to that one. Of course I can include that (though either $CP or cp);

Good point, but luckily the patch has $CP; I just cut and pasted it inaccurately here.

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

Replying to leif:

Thanks for pointing me to that one. Of course I can include that (though either $CP or cp);

I prefer cp, I see no reason to use $CP. There was a similar discussion about $RM before in which it was decided that $RM would not be used anymore. Now $RM has been replaced by rm (or rm -f) everywhere.

comment:62 in reply to: ↑ 61 Changed 10 years ago by leif

Replying to jdemeyer:

Replying to leif:

Thanks for pointing me to that one. Of course I can include that (though either $CP or cp);

I prefer cp, I see no reason to use $CP.

Maybe some Cygwin -- more precisely Windoofs -- flavour... :D

There was a similar discussion about $RM before in which it was decided that $RM would not be used anymore. Now $RM has been replaced by rm (or rm -f) everywhere.

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 PATHs to build or use Sage. I'm btw. still in favour of supporting ~/.sagerc and perhaps SAGERC, not to mention using SAGE_CC, SAGE_CFLAGS etc. instead of CC, CFLAGS, since one never knows where the latter come from, meaning if they've intentionally been set [for Sage], and -- more important -- whether they've been set by the user or by some Sage script...)

comment:63 follow-up: Changed 10 years ago by leif

Replying to dimpase:

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.

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 (src/) and ship them as separate tarballs (perhaps just repackaged with tar and bzip2), along with cut-down Sage-source only .spkg files (preferably with a new extension, say .spi).

sage-spkg then could take care of the upstream packages, putting their appropriate, corresponding versions into place before calling spkg-install, i.e. extracting them into src/, such that we would not even have to change anything in the existing spkg-install and spkg-check files, the Sage part of spkgs.

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. foobar-x.y.z.pN.spkg, we would have foobar-x.y.z.tar.bz2 (vanilla upstream), perhaps in $SAGE_ROOT/spkg/standard/upstream, optionally but preferably a Mercurial repository in $SAGE_ROOT/spkg/standard (ignoring upstream/), and the stripped-down foobar-x.y.z.pN.spi files in $SAGE_ROOT/spkg/standard, or even just foobar-x.y.z.spi if they are under central revision control.

comment:64 in reply to: ↑ 63 ; follow-up: Changed 10 years ago by dimpase

Replying to leif:

Replying to dimpase:

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.

If I understand you correctly (having the whole upstream tree under revision control), that would heavily increase their sizes.

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 in reply to: ↑ 64 ; follow-up: Changed 10 years ago by leif

Replying to dimpase:

Replying to leif:

If I understand you correctly (having the whole upstream tree under revision control), that would heavily increase their sizes.

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.

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 spkg-install rather than redundantly shipping them already checked out along with the repository), and second, presumably more important, you'd then in addition carry all the upstream history, which of course would grow the packages' sizes in comparison to what we have now.

comment:66 in reply to: ↑ 65 ; follow-ups: Changed 10 years ago by dimpase

Replying to leif:

Replying to dimpase:

Replying to leif:

If I understand you correctly (having the whole upstream tree under revision control), that would heavily increase their sizes.

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.

I don't know which files' sizes you've added up there,

I just ran 'du' command on '$SAGEROOT/devel/sage/', and on '$SAGEROOT/devel/sage/.hg'.

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 spkg-install rather than redundantly shipping them already checked out along with the repository), and second, presumably more important, you'd then in addition carry all the upstream history, which of course would grow the packages' sizes in comparison to what we have now.

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 in reply to: ↑ 66 Changed 10 years ago by leif

Replying to dimpase:

Replying to leif:

I don't know which files' sizes you've added up there,

I just ran 'du' command on '$SAGEROOT/devel/sage/', and on '$SAGEROOT/devel/sage/.hg'.

Well, then you've counted also all generated files (*.{pyc,pyo,o,so,whatever}, most of *.{c,cpp}), the complete build/ tree, and all of Sphinx's generated files beneath doc/. (Just take a look at .hgignore.)

Also, since Mercurial stores its files compressed, for a fair comparison you should have compared a compressed version of .hg/ to an equally compressed tarball or whatever of the plain / checked-out source files.

comment:68 in reply to: ↑ 66 Changed 10 years ago by leif

Replying to dimpase:

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.

Either you put src/ under revision control, or you don't. If you do, then copying a fresh, upgraded upstream tree into src/, Mercurial will record the changes relative to the previous version in Sage, whichever that was. And you'd have the same problems with possibly rebasing patches that Sage made to the previous upstream release.

Or did you mean putting src/ into a separate Mercurial repository, to be thrown away upon every upstream upgrade?

I cannot really see an advantage in either way.

comment:69 follow-up: Changed 10 years ago by kcrisman

I suggest this discussion belongs on sage-devel :-) as I'm sure others have opinions as well.

comment:70 in reply to: ↑ 69 Changed 10 years ago by leif

Replying to kcrisman:

I suggest this discussion belongs on sage-devel :-) as I'm sure others have opinions as well.

Just post a link to this ticket there. XD

(A BBS would IMHO be better for such discussions. The only advantage of Ggle groups over trac is that the former supports concurrent posting.)

comment:71 Changed 10 years ago by leif

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 Changed 10 years ago by leif

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 .svn folders [still] included, which shouldn't matter here. FLINT is quite small compared to other spkgs anyway, so the absolute difference should be much? larger on average.)

comment:73 Changed 10 years ago by leif

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 Changed 10 years ago by jdemeyer

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 follow-up: Changed 10 years ago by kcrisman

Unfortunately, because of the sage.math cluster failure, this is not currently accessible. Given that the actual relevant part of http://trac.sagemath.org/sage_trac/attachment/ticket/11547/trac_11547-flint.diff is

=== flint-1.5.0.p7 (Karl-Dieter Crisman, 28th June 2011) === 
 * Enable both libflint.dll and .so on Cygwin (see Trac 11547) 

and

-     # also name flint library correctly for MS Windows (an so won't work). 
-     mv libflint.so libflint.dll 
+    # make both kinds of dynamic libraries available for Windows 
+    $CP -p libflint.so libflint.dll

(where the $CP could change to cp)

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 in reply to: ↑ 75 ; follow-up: Changed 10 years ago by leif

Replying to kcrisman:

Unfortunately, because of the sage.math cluster failure, this is not currently accessible. Given that the actual relevant part of http://trac.sagemath.org/sage_trac/attachment/ticket/11547/trac_11547-flint.diff is

=== flint-1.5.0.p7 (Karl-Dieter Crisman, 28th June 2011) === 
 * Enable both libflint.dll and .so on Cygwin (see Trac 11547) 

and

-     # also name flint library correctly for MS Windows (an so won't work). 
-     mv libflint.so libflint.dll 
+    # make both kinds of dynamic libraries available for Windows 
+    $CP -p libflint.so libflint.dll

(where the $CP could change to cp)

then perhaps someone could be kind when this comes back online and make the p8 there.

I've made a p8 with (only) these changes based on the latest p7 from here and uploaded it onto Ggle code. See #11547 for details in a few minutes; I'll also update #6743 accordingly.


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

Well, being offline isn't always unfortunate. ;-)

Have a good time then.

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

I've made a p8 with (only) these changes based on the latest p7 from here and uploaded it onto Ggle code. See #11547 for details in a few minutes; I'll also update #6743 accordingly.

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 G**gle? Does this confuse them - and to what end? It's not like there aren't other references to said company on the Sage trac, and you are already quite good at covering your tracks - I can't find any references to you outside of Sage and a couple computer newsgroups and of course this site ;-)

Well, being offline isn't always unfortunate. ;-)

Good point. Besonders wenn man in die Heimat fliegt.

comment:78 in reply to: ↑ 77 Changed 10 years ago by leif

Replying to kcrisman:

By the way, what's up with the G**gle? Does this confuse them - and to what end?

Don't know. I just can't spell it.


Well, being offline isn't always unfortunate. ;-)

Good point. Besonders wenn man in die Heimat fliegt.

Na denn guten Flug. Internet gibt's hier allerdings mancherorts. :-)

comment:79 Changed 10 years ago by jdemeyer

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