Opened 8 years ago

Closed 7 years ago

#12821 closed defect (fixed)

testcc.sh: allow $CC to contain multiple words

Reported by: jdemeyer Owned by: jdemeyer
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:

Description (last modified by jdemeyer)

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 jdemeyer 8 years ago.
12821_testcc.patch (8.8 KB) - added by jdemeyer 7 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 8 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)
  • Status changed from new to needs_review

comment:4 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:5 follow-ups: Changed 8 years ago by leif

  • Reviewers set to 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 8 years ago by jdemeyer

Replying to leif:

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

In case the user supplies -2 arguments :-)

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

One must not run

"$CC" foo.c -o foo

so I think one should also not run

testcc.sh "$CC"

comment:8 Changed 8 years ago by leif

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

  • Owner changed from GeorgSWeber to jdemeyer

New version, needs review.

comment:10 in reply to: ↑ 7 Changed 8 years ago by leif

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

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

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

  • Status changed from needs_review to needs_work
  • Work issues set to 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 8 years ago by leif

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

Added a fix also for testcflags.sh.

Changed 8 years ago by jdemeyer

comment:16 Changed 8 years ago by jdemeyer

  • Status changed from needs_work to needs_review
  • Work issues Fix comparison to `GNU` in the M4RIE spkg. deleted

comment:17 follow-ups: Changed 8 years ago by leif

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

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

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 follow-ups: Changed 8 years ago by leif

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 ; follow-up: Changed 8 years ago by leif

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

comment:23 in reply to: ↑ 22 Changed 8 years ago by jdemeyer

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

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 ; follow-up: Changed 8 years ago by jdemeyer

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

comment:27 in reply to: ↑ 26 Changed 8 years ago by 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'.

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

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

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 8 years ago by malb

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

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

  • Milestone changed from sage-5.0 to sage-5.1

Still needs review...

comment:33 Changed 7 years ago by ohanar

  • Status changed from needs_review to needs_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 7 years ago by jdemeyer

comment:34 Changed 7 years ago by jdemeyer

  • Status changed from needs_work to needs_review

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

Fixed.

comment:35 follow-up: Changed 7 years ago by ohanar

  • Keywords sd40.5 added
  • Reviewers changed from Leif Leonhardy to Leif Leonhardy, R. Andrew Ohana
  • Status changed from needs_review to positive_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 7 years ago by ohanar (previous) (diff)

comment:36 in reply to: ↑ 35 Changed 7 years ago by jdemeyer

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

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