Opened 12 years ago

Closed 9 years ago

#7027 closed defect (fixed)

clean up f2c spkg

Reported by: drkirkby Owned by: tbd
Priority: major Milestone: sage-5.0
Component: build Keywords:
Cc: mjo Merged in: sage-5.0.beta6
Authors: R. Andrew Ohana Reviewers: Michael Orlitzky
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

f2c ignores CC, CFLAGS, and MAKE variables: On sage.math using

  CC=clang
  CFLAGS=-O3
  MAKE="make -j6"
****************************************************
Host system
uname -a:
Linux sage.math.washington.edu 2.6.24-28-server #1 SMP Fri Feb 11 18:08:32 UTC 2011 x86_64 Intel(R) Xeon(R) CPU X7460 @ 2.66GHz GenuineIntel GNU/Linux
****************************************************
****************************************************
CC Version
clang -v
clang version 3.0 (tags/RELEASE_30/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
****************************************************
gcc -c f77vers.c
gcc -c i77vers.c
gcc -c -DSkip_f2c_Undefs -O -fPIC main.c
ld -r -o main.xxx main.o
mv main.xxx main.o
gcc -c -DSkip_f2c_Undefs -O -fPIC s_rnge.c
ld -r -o s_rnge.xxx s_rnge.o
mv s_rnge.xxx s_rnge.o
gcc -c -DSkip_f2c_Undefs -O -fPIC abort_.c

Also, there are left over files from development on OSX:

hg status
? ._.hg
? ._.hgignore
? ._SPKG.txt
? ._spkg-install
? patches/._f2c.makefile

Finally, there are unnecessary files in the patch directory, we only need one of the patches, and neither of the makefiles.

spkg: http://boxen.math.washington.edu/home/jdemeyer/spkg/f2c-20070816.p3.spkg

Attachments (1)

f2c-20070816.p3.patch (13.2 KB) - added by ohanar 9 years ago.
for review purposes

Download all attachments as: .zip

Change History (25)

comment:1 Changed 9 years ago by ohanar

  • Authors set to R. Andrew Ohana
  • Description modified (diff)
  • Report Upstream set to N/A
  • Status changed from new to needs_review

comment:2 Changed 9 years ago by mjo

  • Cc mjo added

Can you do CFLAGS, too, while you're in there?

comment:3 Changed 9 years ago by mjo

Oh, and $MAKE!

comment:4 Changed 9 years ago by ohanar

Ok, updated with $MAKE and $CFLAGS.

There was a silly patch whose entire purpose was to make the f2c makefile respect global cflags, of course the spkg-install wouldn't respect those cflags :-/. I am now just passing the cflags at the end of the make line in spkg-install, rather than patching the makefile.

comment:5 follow-up: Changed 9 years ago by mjo

I think there are some left-over files in there:

f2c-20070816.p3 $ sage -hg status
? ._.hg
? ._.hgignore
? ._SPKG.txt
? ._spkg-install
? patches/._f2c.makefile

comment:6 follow-up: Changed 9 years ago by mjo

  • Status changed from needs_review to needs_work
  • Work issues set to remove unneeded files; possibly redundant patch

Why do we need both patches/libf2c.makefile and patches/libf2c.makefile.patch? It looks to me like the patch does the same thing as replacing the upstream makefile with libf2c.makefile:

$ diff -u src/libf2c/makefile patches/libf2c.makefile
--- src/libf2c/makefile	2007-08-14 21:26:15.000000000 -0400
+++ patches/libf2c.makefile	2012-02-10 04:31:00.000000000 -0500
@@ -70,10 +70,10 @@
 ### If your system lacks ranlib, you don't need it; see README.
 
 f77vers.o: f77vers.c
-	$(CC) -c f77vers.c
+	$(CC) -c $(CFLAGS) f77vers.c
 
 i77vers.o: i77vers.c
-	$(CC) -c i77vers.c
+	$(CC) -c $(CFLAGS) i77vers.c
 
 # To get an "f2c.h" for use with "f2c -C++", first "make hadd"
 hadd: f2c.h0 f2ch.add
$ cat patches/libf2c.makefile.patch 
--- libf2c.makefile.orig	2009-01-20 00:22:57.000000000 -0800
+++ libf2c.makefile	2009-01-20 00:22:25.000000000 -0800
@@ -70,10 +70,10 @@
 ### If your system lacks ranlib, you don't need it; see README.
 
 f77vers.o: f77vers.c
-	$(CC) -c f77vers.c
+	$(CC) -c $(CFLAGS) f77vers.c
 
 i77vers.o: i77vers.c
-	$(CC) -c i77vers.c
+	$(CC) -c $(CFLAGS) i77vers.c
 
 # To get an "f2c.h" for use with "f2c -C++", first "make hadd"
 hadd: f2c.h0 f2ch.add

Is there a reason to keep both around and not just the patch?

comment:7 in reply to: ↑ 5 Changed 9 years ago by ohanar

Replying to mjo:

I think there are some left-over files in there:

f2c-20070816.p3 $ sage -hg status
? ._.hg
? ._.hgignore
? ._SPKG.txt
? ._spkg-install
? patches/._f2c.makefile

FYI, these files were in the previous package, probably from someone working on OSX (it seems to like creating these files).

comment:8 in reply to: ↑ 6 Changed 9 years ago by ohanar

  • Description modified (diff)
  • Summary changed from f2c ignores CC and uses gcc anyway to clean up f2c spkg

Replying to mjo:

Why do we need both patches/libf2c.makefile and patches/libf2c.makefile.patch? It looks to me like the patch does the same thing as replacing the upstream makefile with libf2c.makefile:

$ diff -u src/libf2c/makefile patches/libf2c.makefile
--- src/libf2c/makefile	2007-08-14 21:26:15.000000000 -0400
+++ patches/libf2c.makefile	2012-02-10 04:31:00.000000000 -0500
@@ -70,10 +70,10 @@
 ### If your system lacks ranlib, you don't need it; see README.
 
 f77vers.o: f77vers.c
-	$(CC) -c f77vers.c
+	$(CC) -c $(CFLAGS) f77vers.c
 
 i77vers.o: i77vers.c
-	$(CC) -c i77vers.c
+	$(CC) -c $(CFLAGS) i77vers.c
 
 # To get an "f2c.h" for use with "f2c -C++", first "make hadd"
 hadd: f2c.h0 f2ch.add
$ cat patches/libf2c.makefile.patch 
--- libf2c.makefile.orig	2009-01-20 00:22:57.000000000 -0800
+++ libf2c.makefile	2009-01-20 00:22:25.000000000 -0800
@@ -70,10 +70,10 @@
 ### If your system lacks ranlib, you don't need it; see README.
 
 f77vers.o: f77vers.c
-	$(CC) -c f77vers.c
+	$(CC) -c $(CFLAGS) f77vers.c
 
 i77vers.o: i77vers.c
-	$(CC) -c i77vers.c
+	$(CC) -c $(CFLAGS) i77vers.c
 
 # To get an "f2c.h" for use with "f2c -C++", first "make hadd"
 hadd: f2c.h0 f2ch.add

Is there a reason to keep both around and not just the patch?

Nope, but that wasn't the initial goal of this ticket, that said I've changed the purpose/description.

comment:9 follow-up: Changed 9 years ago by ohanar

  • Status changed from needs_work to needs_review
  • Work issues remove unneeded files; possibly redundant patch deleted

ok, I've made those changes (plus cleaned up the source directory, which wasn't pristine)

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

  • Reviewers set to Michael Orlitzky
  • Status changed from needs_review to positive_review

Replying to ohanar:

ok, I've made those changes (plus cleaned up the source directory, which wasn't pristine)

Thanks for taking the time to do this, I didn't check the old SPKG for those files.

I've compared the p2 and p3 src directories after patching; they look equivalent given the spkg-install changes.

To test the original issue, I've replaced $CC and $CFLAGS in both makefiles with junk. If I don't set my environment variables, the build crashes. If I export a real $CC and $CFLAGS, it builds fine.

I built with MAKE=make -j40 with no problems.

comment:11 follow-ups: Changed 9 years ago by drkirkby

I would have thought it sensible to check the return value of 'patch'. Any changes in the package are could easily stop the patch applying cleanly, if at all, yet that is not tested.

Dave

comment:12 in reply to: ↑ 11 Changed 9 years ago by ohanar

  • Status changed from positive_review to needs_work

Replying to drkirkby:

I would have thought it sensible to check the return value of 'patch'. Any changes in the package are could easily stop the patch applying cleanly, if at all, yet that is not tested.

Yup, I agree, I've uploaded a new spkg that adds a check.

Dave

comment:13 Changed 9 years ago by ohanar

  • Status changed from needs_work to needs_review

comment:14 in reply to: ↑ 11 ; follow-up: Changed 9 years ago by mjo

  • Status changed from needs_review to needs_work
  • Work issues set to patch success test, immortal dotfiles

Replying to drkirkby:

I would have thought it sensible to check the return value of 'patch'. Any changes in the package are could easily stop the patch applying cleanly, if at all, yet that is not tested.

If we want to go this route, we'll have to set the fuzz factor. Otherwise, GNU patch, at least, will happily return success:

$ emacs makefile
$ patch -p0 < ../../patches/libf2c.makefile.patch 
patching file makefile
Hunk #1 succeeded at 73 with fuzz 1 (offset 3 lines).
$ echo $?
0

Using --fuzz=0 should catch that, but will still miss large offsets. I don't know if there's a way to catch those... patch is returning success here even for huge offsets:

$ patch --fuzz=0 -p0 < ../../patches/libf2c.makefile.patch 
patching file makefile
Hunk #1 succeeded at 111 (offset 41 lines).
$ echo $?
0

Whoever is upgrading the package would hopefully check for this, but maybe there's a smarter way that I haven't found yet.

Anyway, these are back =)

$ sage -hg status
? ._.hg
? ._.hgignore
? ._SPKG.txt
? ._spkg-install
? patches/._f2c.makefile

comment:15 Changed 9 years ago by drkirkby

People updating packages, whilst not taking care of patches, has been a problem several times in Sage. Anything that could be done to reduce that would be good, but maybe it's not possible to do it very well.

Dave

Changed 9 years ago by ohanar

for review purposes

comment:16 Changed 9 years ago by ohanar

  • Status changed from needs_work to needs_review

Ok, new version, got rid of the ._ files again (which was my bad, since I restarted off of the p2 package). I've also made some more changes to spkg-install, added some comments, moved some stuff around, made it more uniform, as well as make a libf2c directory in patches, just to make it easy to add patches in the future, if they are ever needed.

comment:17 follow-up: Changed 9 years ago by mjo

  • Status changed from needs_review to positive_review

This one looks good.

For future reference, we can probably drop the -g (debug) flag, and shouldn't install libf2c if building f2c fails, but those are minor and unrelated to this ticket.

comment:18 in reply to: ↑ 17 Changed 9 years ago by ohanar

Replying to mjo:

For future reference, we can probably drop the -g (debug) flag,

Yeah, I didn't know why we had this, but I didn't want to break anything, so I kept it for safe measure.

comment:19 in reply to: ↑ 14 Changed 9 years ago by jdemeyer

Replying to mjo:

but will still miss large offsets. I don't know if there's a way to catch those... patch is returning success here even for huge offsets

Those are pretty innocent and not a big deal in my opinion. I have never seen a patch applied wrongly because of this.

comment:20 follow-up: Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_work

CC needs quoting:

$MAKE CC="$CC" CFLAGS="${CFLAGS}" 

Also, it would be good to mention the ticket number in SPKG.txt.

(you can set back positive review yourself after making these trivial changes)

comment:21 in reply to: ↑ 20 Changed 9 years ago by ohanar

  • Status changed from needs_work to positive_review

Replying to jdemeyer:

CC needs quoting:

done

Also, it would be good to mention the ticket number in SPKG.txt.

and done

comment:22 Changed 9 years ago by jdemeyer

  • Description modified (diff)

I made spkg-check executable, as it should be.

comment:23 Changed 9 years ago by jdemeyer

  • Work issues patch success test, immortal dotfiles deleted

comment:24 Changed 9 years ago by jdemeyer

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