Opened 10 years ago

Closed 10 years ago

#12821 closed defect (fixed)

testcc.sh: allow $CC to contain multiple words

Reported by: Jeroen Demeyer Owned by: Jeroen Demeyer
Priority: major Milestone: sage-5.1
Component: build Keywords: sd40.5
Cc: Merged in: sage-5.1.beta2
Authors: Jeroen Demeyer Reviewers: Leif Leonhardy, R. Andrew Ohana
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

If CC="gcc -m32" (for example), then the following will fail:

$ testcc.sh $CC
Usage: spkg/bin/testcc.sh name_or_path_to_the_C_compiler
[...]

We should allow testcc.sh to accept multiple arguments and use all of them to run the C compiler (similarly for testcxx.sh):
Apply 12821_testcc.patch to the SAGE_ROOT repository.

We also need to fix libm4rie, which does testcc.sh "$CC":
spkg: http://boxen.math.washington.edu/home/jdemeyer/spkg/libm4rie-20111004.p3.spkg (diff: libm4rie-20111004.p3.diff)

Attachments (2)

libm4rie-20111004.p3.diff (1.6 KB) - added by Jeroen Demeyer 10 years ago.
12821_testcc.patch (8.8 KB) - added by Jeroen Demeyer 10 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)

comment:2 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)

comment:3 Changed 10 years ago by Jeroen Demeyer

Authors: Jeroen Demeyer
Description: modified (diff)
Status: newneeds_review

comment:4 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)

comment:5 Changed 10 years ago by Leif Leonhardy

Reviewers: Leif Leonhardy

Stylewise, I'd use [ $# -lt 1 ].

The usage should say "$0 name_or_path_to_the_C_compiler [optional arguments]".

I'd also use $@ instead of "$@", then quoting $CC doesn't hurt (so you wouldn't have to change the M4RIE spkg either), and the latter won't work as probably expected.

It's also a bit funny to use grep ... | sed ...; a single invocation of sed is sufficient. (At least he didn't use ... | cat | ... ;-) )

comment:6 in reply to:  5 Changed 10 years ago by Jeroen Demeyer

Replying to leif:

Stylewise, I'd use [ $# -lt 1 ].

In case the user supplies -2 arguments :-)

comment:7 in reply to:  5 ; Changed 10 years ago by Jeroen Demeyer

Replying to leif:

I'd also use $@ instead of "$@", then quoting $CC doesn't hurt (so you wouldn't have to change the M4RIE spkg either), and the latter won't work as probably expected.

I think quoting $CC should hurt because it's wrong.

One must not run

"$CC" foo.c -o foo

so I think one should also not run

testcc.sh "$CC"

comment:8 Changed 10 years ago by Leif Leonhardy

Replying to leif:

I'd also use $@ instead of "$@", then quoting $CC doesn't hurt (so you wouldn't have to change the M4RIE spkg either), and the latter won't work as probably expected.

It's also a bit funny to use grep ... | sed ...; a single invocation of sed is sufficient.

I.e.,

$@ -E $TESTFILE | sed -n '/^[A-Z]/s/ //gp'

comment:9 Changed 10 years ago by Jeroen Demeyer

Owner: changed from Georg S. Weber to Jeroen Demeyer

New version, needs review.

comment:10 in reply to:  7 Changed 10 years ago by Leif Leonhardy

Replying to jdemeyer:

Replying to leif:

I'd also use $@ instead of "$@", then quoting $CC doesn't hurt (so you wouldn't have to change the M4RIE spkg either), and the latter won't work as probably expected.

I think quoting $CC should hurt because it's wrong.

You can't say it's wrong. But using "$@" would only make sense if one e.g. did

testcc.sh "$CC" "some argument with spaces" otherargument
# (if $CC is actually a single pathname with spaces in it)

# or, likewise
testcc.sh $CC "some argument with spaces" otherargument

which I think nobody will do.

comment:11 Changed 10 years ago by Leif Leonhardy

Positive review for the patch to testcc.sh if you remove the extra grep... ;-)


The (patch to the) M4RIE spkg needs work anyway:

if [ "$COMPILER"  = "GNU" ] ; then 
    ...

(testcc.sh never returns GNU, at most GCC.)

comment:12 in reply to:  11 Changed 10 years ago by Leif Leonhardy

Replying to leif:

The (patch to the) M4RIE spkg needs work anyway:

if [ "$COMPILER"  = "GNU" ] ; then 
    ...

(testcc.sh never returns GNU, at most GCC.)

And wouldn't using testcflags.sh -fPIC -Wall -pedantic there be more appropriate anyway?

comment:13 Changed 10 years ago by Leif Leonhardy

Status: needs_reviewneeds_work
Work issues: Fix comparison to `GNU` in the M4RIE spkg.

... if you change the spkg here.

(And the changelog entry should say "... when calling testcc.sh.", provided you don't change spkg-install to use testcflags.sh of course, the latter being much better.)

comment:14 Changed 10 years ago by Leif Leonhardy

Btw., testcflags.sh also needs work, as I already reported on sage-devel a while ago:

...
outfile=testcflags-$$
cfile=$outfile.c
...
rm -f $TESTFILE $OUTFILE
...

(And there are a few other issues with it. Wonder who reviewed such, as the temporary files will always remain, while completely unrelated files might get deleted(!).)

comment:15 Changed 10 years ago by Jeroen Demeyer

Added a fix also for testcflags.sh.

Changed 10 years ago by Jeroen Demeyer

Attachment: libm4rie-20111004.p3.diff added

comment:16 Changed 10 years ago by Jeroen Demeyer

Status: needs_workneeds_review
Work issues: Fix comparison to `GNU` in the M4RIE spkg.

comment:17 Changed 10 years ago by Leif Leonhardy

The test for the compiler used in M4RIE's spkg-install to add specific flags is still a bit weird; you could just use testcflags.sh for all of this, although + z is currently a bit problematic (in case the space is really required), because testcflags.sh tests the flags one by one, and passing "+ z" as a single parameter to it might not work either, since the compiler most probably doesn't expect it to be passed as a single argument.

But the quoting in testcflags.sh wouldn't work as expected anyway, so you could just omit the quotes there when calling the compiler with previous and the currently to be tested flags.


If SAGE_DEBUG=yes, -O0 should be appended to CFLAGS, as it was.

comment:18 in reply to:  17 Changed 10 years ago by Leif Leonhardy

Replying to leif:

But the quoting in testcflags.sh wouldn't work as expected anyway, so you could just omit the quotes there when calling the compiler with previous and the currently to be tested flags.

Oh I see, you've meanwhile fixed that, such that we could now use (e.g.) testcflags.sh -fPIC -Kpic "+ z".

comment:19 Changed 10 years ago by Leif Leonhardy

W.r.t. testcflags.sh:

I'd move the usage instructions from the comment into the error message given if CC isn't set, and add $# -eq 0 (or $# -lt 1) to the test, such that calling testcflags.sh without arguments outputs its usage.

comment:20 Changed 10 years ago by Leif Leonhardy

A few more comments:

  • The scripts could respect TMP and TMPDIR if set.
  • On Cygwin -o foo will create foo.exe, so that should (also) be removed.
  • Using sed -n '/^[A-Z]/s/ //gp' is much faster than sed -n 's/ //g;/^[A-Z]/p'.
  • In spkg/install, there should be a way to bypass the installation of GCC (if the compiler isn't GCC), e.g. if SAGE_PORT=yes (?). Or is there already such?

comment:21 in reply to:  20 ; Changed 10 years ago by Leif Leonhardy

Replying to leif:

  • In spkg/install, there should be a way to bypass the installation of GCC (if the compiler isn't GCC), e.g. if SAGE_PORT=yes (?). Or is there already such?

Doesn't look like. (Although one can set SAGE_INSTALL_GCC=no.)

Quite unrelated, but from spkg/install (i.e., while we're at it):

if [ -z "$SAGE_FORTRAN" ]; then
    if command -v gfortran >/dev/null 2>/dev/null; then
        SAGE_FORTRAN=gfortran
    elif command -v g95 >/dev/null 2>/dev/null; then
        SAGE_FORTRAN=g95
    elif command -v g77 >/dev/null 2>/dev/null; then
        SAGE_FORTRAN=g77
    fi
fi

ignores settings of FC and F77, the standard variables containing the Fortran compiler.

The created sage_fortran script hardcodes -m64 (and ignores FFLAGS, FCFLAGS, F77FLAGS etc.).

Don't know whether that's really intentional, but also the current setting of SAGE_FORTRAN is hardcoded into the script, such that later changing SAGE_FORTRAN has no effect.

comment:22 Changed 10 years ago by Leif Leonhardy

More on spkg/install: :-)

grep '^\($\|[0-3]\.\|4\.[0-3]\|4\.6\.[01]$\)'

is GNUism and non-portable; either use

egrep '^($|[0-3]\.|4\.[0-3]|4\.6\.[01]$)'

or, preferably, shell pattern matching, which is reliable, and in which case you could also more easily handle e.g. a 4.6.1 version on Ubuntu.

comment:23 in reply to:  22 Changed 10 years ago by Jeroen Demeyer

Replying to leif:

More on spkg/install: :-)

grep '^\($\|[0-3]\.\|4\.[0-3]\|4\.6\.[01]$\)'

is GNUism and non-portable; either use

egrep '^($|[0-3]\.|4\.[0-3]|4\.6\.[01]$)'

or, preferably, shell pattern matching, which is reliable, and in which case you could also more easily handle e.g. a 4.6.1 version on Ubuntu.

See #12825.

comment:24 in reply to:  21 Changed 10 years ago by Jeroen Demeyer

Replying to leif:

Quite unrelated, but from spkg/install (i.e., while we're at it):

if [ -z "$SAGE_FORTRAN" ]; then
    if command -v gfortran >/dev/null 2>/dev/null; then
        SAGE_FORTRAN=gfortran
    elif command -v g95 >/dev/null 2>/dev/null; then
        SAGE_FORTRAN=g95
    elif command -v g77 >/dev/null 2>/dev/null; then
        SAGE_FORTRAN=g77
    fi
fi

ignores settings of FC and F77, the standard variables containing the Fortran compiler.

The created sage_fortran script hardcodes -m64 (and ignores FFLAGS, FCFLAGS, F77FLAGS etc.).

Don't know whether that's really intentional, but also the current setting of SAGE_FORTRAN is hardcoded into the script, such that later changing SAGE_FORTRAN has no effect.

You're certainly right that there are some issues here, but that's totally outside of the scope of this ticket.

comment:25 in reply to:  20 ; Changed 10 years ago by Jeroen Demeyer

Replying to leif:

The scripts could respect TMP and TMPDIR if set.

What's TMP? I agree on TMPDIR, which is the standard way of specifying a temporary directory.

On Cygwin -o foo will create foo.exe, so that should (also) be removed.

Fair enough.

Using sed -n '/^[A-Z]/s/ //gp' is much faster than sed -n 's/ //g;/^[A-Z]/p'.

Those two commands aren't equivalent.

In spkg/install, there should be a way to bypass the installation of GCC.

You can set SAGE_INSTALL_GCC=no.

comment:26 in reply to:  25 ; Changed 10 years ago by Leif Leonhardy

Replying to jdemeyer:

Replying to leif:

Using sed -n '/^[A-Z]/s/ //gp' is much faster than sed -n 's/ //g;/^[A-Z]/p'.

Thoase two commands aren't equivalent.

They of course aren't, but in our case have the same (desired) effect; mine is equivalent to what Dave did, while yours isn't.

comment:27 in reply to:  26 Changed 10 years ago by Jeroen Demeyer

Replying to leif:

Replying to jdemeyer:

Replying to leif:

Using sed -n '/^[A-Z]/s/ //gp' is much faster than sed -n 's/ //g;/^[A-Z]/p'.

Thoase two commands aren't equivalent.

They of course aren't, but in our case have the same (desired) effect; mine is equivalent to what Dave did, while yours isn't.

Did you even try those commands before saying such non-sense?

comment:28 Changed 10 years ago by Leif Leonhardy

Replying to jdemeyer:

Replying to leif:

Replying to jdemeyer:

Replying to leif:

Using sed -n '/^[A-Z]/s/ //gp' is much faster than sed -n 's/ //g;/^[A-Z]/p'.

Those two commands aren't equivalent.

They of course aren't, but in our case have the same (desired) effect; mine is equivalent to what Dave did, while yours isn't.

Did you even try those commands before saying such non-sense?

I apparently had trailing spaces on the lines with compiler names...

sed -n '/^[A-Z]/s/[ ]*//gp'

does it, although we shouldn't have spaces on these lines anyway (in which case the substitution of course would be superfluous).

comment:29 in reply to:  17 Changed 10 years ago by Leif Leonhardy

Replying to leif:

If SAGE_DEBUG=yes, -O0 should be appended to CFLAGS, as it was.

This is still in the diff; haven't downloaded the new spkg (yet).

comment:30 Changed 10 years ago by Martin Albrecht

I have based the new SPKG at #12841 on http://boxen.math.washington.edu/home/jdemeyer/spkg/libm4rie-20111004.p3.spkg. Additionally, I changed spkg-install to append not prepend -O0.

comment:31 Changed 10 years ago by Jeroen Demeyer

New version of 12821_testcc.patch needs review.

I disagree about appending -O0. I think the user's CFLAGS should always override the SPKG's CFLAGS without exceptions. What if the user for some reason wants --enable-debug but still -O2?

In any case, the following would be very inconsistent:

if [ "x$SAGE_DEBUG" = "xyes" ]; then
   CFLAGS="$CFLAGS -O0"
   ENABLE_DEBUG="--enable-debug"
else
   CFLAGS="-O2 $CFLAGS"
   ENABLE_DEBUG=""
fi

comment:32 Changed 10 years ago by Jeroen Demeyer

Milestone: sage-5.0sage-5.1

Still needs review...

comment:33 Changed 10 years ago by R. Andrew Ohana

Status: needs_reviewneeds_work

The patched version of testcflags.sh always produces an empty list of cflags for me. My guess is that it is due to d being undefined in the following block of code:

unsigned long double_to_ulong() 
{ 
    return (unsigned long)d; 
}

Changed 10 years ago by Jeroen Demeyer

Attachment: 12821_testcc.patch added

comment:34 Changed 10 years ago by Jeroen Demeyer

Status: needs_workneeds_review

Of course, I don't know how I missed that...

Fixed.

comment:35 Changed 10 years ago by R. Andrew Ohana

Keywords: sd40.5 added
Reviewers: Leif LeonhardyLeif Leonhardy, R. Andrew Ohana
Status: needs_reviewpositive_review

Ok, looks good and works well.

To note, this is not a great test for cflags. For example, clang only throws warnings to bad compiler flags, it does not error.

Last edited 10 years ago by R. Andrew Ohana (previous) (diff)

comment:36 in reply to:  35 Changed 10 years ago by Jeroen Demeyer

Replying to ohanar:

For example, clang only throws warnings to bad compiler flags, it does not error.

That's stupid:

bsd:~ jdemeyer$ clang -march=foobar foo.c -c -o foo.o
'foobar' is not a recognized processor for this target (ignoring processor)
bsd:~ jdemeyer$ echo $?
0

This could mean that MPIR for example might produce very suboptimal code because it thinks that -march=foo is supported when it's not.

comment:37 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.1.beta2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.