Opened 12 years ago

Closed 9 years ago

#7071 closed defect (fixed)

palp spkg ignores global CC and CFLAGS variables

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

Status badges

Description (last modified by jdemeyer)

Using

  • Solaris 10 update 7 on SPARC
  • sage-4.1.2.alpha4
  • Sun Studio 12.1
  • An updated configure script to allow the Sun compiler to be used #7021

This is one of the many packages that ignore the setting of the variable CC.

palp-1.1.p1/src/GNUmakefile
palp-1.1.p1/src/mori.c
Finished extraction
****************************************************
Host system
uname -a:
SunOS swan 5.10 Generic_139555-08 sun4u sparc SUNW,Sun-Blade-1000
****************************************************
****************************************************
CC Version
/opt/xxxsunstudio12.1/bin/cc -v
usage: cc [ options] files.  Use 'cc -flags' for details
****************************************************
make[2]: Entering directory `/export/home/drkirkby/sage/sage-4.1.2.alpha4/spkg/build/palp-1.1.p1/src'
gcc -O3 -g -W -Wall -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE   -c -o poly.o poly.c
gcc -O3 -g -W -Wall -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE   -c -o Coord.o Coord.c
gcc -O3 -g -W -Wall -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE   -c -o Rat.o Rat.c
gcc -O3 -g -W -Wall -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE   -c -o Vertex.o Vertex.c

To update:

Attachments (3)

palp-1.1.p4.patch (789 bytes) - added by ohanar 9 years ago.
for review purposes
palp-2.0.p1.patch (1.8 KB) - added by ohanar 9 years ago.
for review purposes
trac-12055-SAGELOCAL_BIN.patch (614 bytes) - added by vbraun 9 years ago.
Patch from #12055 needs to be applied

Download all attachments as: .zip

Change History (14)

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

Changed 9 years ago by ohanar

for review purposes

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

  • Cc mjo added
  • Status changed from needs_review to needs_work
  • Work issues set to respect CFLAGS too

Can we pass CFLAGS, too? That will allow us to get rid of that horrible sed. Might as well fix the "xyes" test also

comment:3 Changed 9 years ago by vbraun

Palp 2 is out and I have a preliminary spkg at #12055. Please base your work on the new version.

comment:4 Changed 9 years ago by ohanar

  • Dependencies set to #12055
  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Summary changed from palp-1.1.p1 ignores CC variable and uses gcc, so fails with Sun Studio. to palp spkg ignores global CC and CFLAGS variables

ok, I've based my changes off of the new version at #12055 and have made CFLAGS respected as well.

Changed 9 years ago by ohanar

for review purposes

comment:5 Changed 9 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review
  • Work issues respect CFLAGS too deleted

Looks good!

Changed 9 years ago by vbraun

Patch from #12055 needs to be applied

comment:6 Changed 9 years ago by vbraun

  • Description modified (diff)

I've switched the old ticket #12055 to invalid to not make Jeroen replace the spkg twice. But we still need the patch to the Sage library from there.

comment:7 in reply to: ↑ 2 Changed 9 years ago by drkirkby

Replying to mjo:

Can we pass CFLAGS, too? That will allow us to get rid of that horrible sed. Might as well fix the "xyes" test also

The "xyes" test, as it is called above, is the safest, most portable way to test for a string, as other methods, like the proposed change, can fail under obscure conditions. One might argue they don't fail with modern versions of bash, but IMHO is it worthwhile to write scripts which will always work under all conditions. The original code will always work - the proposed change is less portable. I suggest you take a look at the scripts created by autoconf. You will find they use a similar method to what was in Sage, as it is known to always work.

As such, I believe the change is a retrograde step.

Dave

comment:8 Changed 9 years ago by drkirkby

  • Status changed from positive_review to needs_work

comment:9 Changed 9 years ago by vbraun

  • Status changed from needs_work to positive_review

I disagree. The xblah test makes it more difficult to read the test, which both increases the chance for errors as well as the long-term maintenance effort.

A quick survey shows that at least half of the Sage spkgs use the simplified test, so clearly nobody has yet encountered a shell ancient enough to not work. I'm pretty sure many upstream sources use simplified tests, too, so there is basically no hope of ever compiling Sage on such a system without installing a shell that isn't from the middle ages. Autotools output really is a totally different issue, since their scripts are autogenerated readability is not an issue (and usually is pretty bad, in fact).

If you disagree we can discuss this on the sage-devel, but its such a pervasive issue throughout Sage that it doesn't really matter if we use the simplified test in one spkg more or less.

comment:10 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:11 Changed 9 years ago by jdemeyer

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