Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#15697 closed enhancement (fixed)

Upgrade MPC to latest upstream

Reported by: jdemeyer Owned by:
Priority: minor Milestone: sage-6.2
Component: packages: standard Keywords:
Cc: zimmerma, thevenyp Merged in:
Authors: Jeroen Demeyer Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: u/jdemeyer/ticket/15697 (Commits, GitHub, GitLab) Commit: 00a5937b33cd8ad6f1eafbccfbab1d4f73321bf3
Dependencies: Stopgaps:

Status badges

Change History (24)

comment:1 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 8 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/15697
  • Created changed from 01/19/14 22:47:03 to 01/19/14 22:47:03
  • Modified changed from 01/19/14 23:02:21 to 01/19/14 23:02:21

comment:3 Changed 8 years ago by jdemeyer

  • Cc zimmerma added
  • Commit set to 45ae5fcec6efaf8374166477f701203d6a450c31

New commits:

45ae5fcUpgrade MPC to version 1.0.2

comment:4 Changed 8 years ago by fbissey

Is bz2 rather than gz if it is the only thing provided by upstream a hard requirement?

comment:5 Changed 8 years ago by jdemeyer

Given that every other package in Sage (except for bzip2 obviously) uses .bz2, I decided to recompress to .bz2. But I personally don't care much...

comment:6 follow-up: Changed 8 years ago by fbissey

I guess my concern is can the sage build system cope with gz. I might be a purist but I think we should as much as possible include stuff from upstream as is. It make things easier to include and baring incident like flint 2.4.1 we have the checksum from the upstream tarball rather than the one produced by a sage dev. This is stuff for sage-devel really.

I have otherwise no concern with this upgrade.

comment:7 in reply to: ↑ 6 Changed 8 years ago by jdemeyer

Replying to fbissey:

I guess my concern is can the sage build system cope with gz.

Yes, it can.

comment:8 Changed 8 years ago by zimmerma

  • Cc thevenyp added

comment:9 Changed 8 years ago by jdemeyer

François, for me it's fine to take the pure upstream .gz. It only requires changing the tarball= line in build/pkgs/mpc/checksums.ini.

comment:10 Changed 8 years ago by fbissey

While I raised the point I suddenly feel that it is not my place to make that decision. There should be guidelines agreed with other in this case.

comment:11 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_review to needs_work

comment:12 Changed 8 years ago by git

  • Commit changed from 45ae5fcec6efaf8374166477f701203d6a450c31 to 00a5937b33cd8ad6f1eafbccfbab1d4f73321bf3

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

00a5937Upgrade MPC to version 1.0.2

comment:13 Changed 8 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:14 Changed 8 years ago by fbissey

  • Status changed from needs_review to positive_review

OK I said I would review packages :) The sheer reduction of patches alone justify this one.

comment:15 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:16 Changed 8 years ago by vbraun

  • Reviewers set to François Bissey

comment:17 Changed 8 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:18 Changed 8 years ago by leif

Just for the record:

configure is still slightly broken w.r.t. CC and CFLAGS, since the "hack" currently ignores the setting of CPP (and instead tries /lib/cpp with CPPFLAGS first, which may not work, resulting in empty CC and CFLAGS settings from gmp.h, such that finally gcc is used, which may of course also fail).

I'll one dayTM report this upstream.

comment:19 Changed 8 years ago by leif

P.S.: From m4/mpc.m4:

#
# SYNOPSIS
#
#
# MPC_GMP_CC_CFLAGS
#
# DESCRIPTION
#
# Checks if CC and CFLAGS can be extracted from gmp.h
# essentially copied from mpfr
#
AC_DEFUN([MPC_GMP_CC_CFLAGS], [
   AC_MSG_CHECKING(for CC and CFLAGS in gmp.h)
   # AC_PROG_CPP triggers the search for a C compiler; use hack instead
   for cpp in /lib/cpp gcc cc c99
   do
      test $cpp = /lib/cpp || cpp="$cpp -E"
      echo foo > conftest.c
      if $cpp $CPPFLAGS conftest.c > /dev/null 2> /dev/null ; then
         # Get CC
         echo "#include \"gmp.h\"" >  conftest.c
         echo "MPFR_OPTION __GMP_CC"           >> conftest.c
         GMP_CC=`$cpp $CPPFLAGS conftest.c 2> /dev/null | $EGREP MPFR_OPTION | $SED -e 's/MPFR_OPTION //g;s/ *" *//g'`
         # Get CFLAGS
         echo "#include \"gmp.h\"" >  conftest.c
         echo "MPFR_OPTION __GMP_CFLAGS"           >> conftest.c
         GMP_CFLAGS=`$cpp $CPPFLAGS conftest.c 2> /dev/null | $EGREP MPFR_OPTION | $SED -e 's/MPFR_OPTION //g;s/ *" *//g'`
         break
      fi
   done

   if test "x$GMP_CFLAGS" = "x__GMP_CFLAGS" -o "x$GMP_CC" = "x__GMP_CC" ; then
      AC_MSG_RESULT(no)
      GMP_CC=
      GMP_CFLAGS=
   else
      AC_MSG_RESULT(yes [CC=$GMP_CC CFLAGS=$GMP_CFLAGS])
   fi

   # Check for validity of CC and CFLAGS obtained from gmp.h
   if test -n "$GMP_CC$GMP_CFLAGS" ; then
      AC_MSG_CHECKING(for CC=$GMP_CC and CFLAGS=$GMP_CFLAGS)
      echo "int main (void) { return 0; }" > conftest.c
      if $GMP_CC $GMP_CFLAGS -o conftest conftest.c 2> /dev/null ; then
         AC_MSG_RESULT(yes)
         CC=$GMP_CC
         CFLAGS=$GMP_CFLAGS
      else
         AC_MSG_RESULT(no)
      fi
   fi

   rm -f conftest*
])

I don't exactly recall what went wrong, but configure gave me something like

checking for CC and CFLAGS in gmp.h ... yes CC= CFLAGS=

and finally gcc was used (instead of $CC, while the same setting was recorded in gmp.h).

comment:20 Changed 8 years ago by leif

Ah, the problem arises when none of /lib/cpp gcc cc c99 work (with $CPPFLAGS), since then GMP_CC and GMP_CFLAGS will remain empty, and / but

test "x$GMP_CFLAGS" = "x__GMP_CFLAGS" -o "x$GMP_CC" = "x__GMP_CC"

yields false, and

      AC_MSG_RESULT(yes [CC=$GMP_CC CFLAGS=$GMP_CFLAGS])

is executed, but not what follows (since test -n "$GMP_CC$GMP_CFLAGS" yields false):

   # Check for validity of CC and CFLAGS obtained from gmp.h
   if test -n "$GMP_CC$GMP_CFLAGS" ; then
      AC_MSG_CHECKING(for CC=$GMP_CC and CFLAGS=$GMP_CFLAGS)
      echo "int main (void) { return 0; }" > conftest.c
      if $GMP_CC $GMP_CFLAGS -o conftest conftest.c 2> /dev/null ; then
         AC_MSG_RESULT(yes)
         CC=$GMP_CC
         CFLAGS=$GMP_CFLAGS
      else
         AC_MSG_RESULT(no)
      fi
   fi


I ended up with

...
checking for egrep... /bin/grep -E
checking for a sed that does not truncate output... /bin/sed
checking for CC and CFLAGS in gmp.h... yes CC= CFLAGS=
checking for gcc... gcc
checking whether the C compiler works... no
configure: error: in `$SAGE_BUILD_DIR/mpc-1.0.p0/src':
configure: error: C compiler cannot create executables
See `config.log' for more details
Error configuring MPC.

real	0m1.737s
user	0m0.032s
sys	0m0.068s
************************************************************************
Error installing package mpc-1.0.p0
************************************************************************

(because gcc didn't work with $CFLAGS either, while $CC or __GMP_CC from gmp.h would have worked).

(In the mpc-1.0.p0 spkg, we already use[d] the related upstream patch (included in MPC 1.0.1) fixing a slightly different issue with GMP_CC and GMP_CFLAGS, so the same holds for MPC 1.0.2.)

comment:21 Changed 8 years ago by jdemeyer

I'm posting an answer from Andreas Enge, MPC developer:

Does the following patch solve the problem for you?

  • m4/mpc.m4

     
    11# mpc.m4
    22#
    3 # Copyright (C) 2008, 2009, 2010, 2011, 2012 INRIA
     3# Copyright (C) 2008, 2009, 2010, 2011, 2012, 2014 INRIA
    44#
    55# This file is part of GNU MPC.
    66#
     
    127127AC_DEFUN([MPC_GMP_CC_CFLAGS], [
    128128   AC_MSG_CHECKING(for CC and CFLAGS in gmp.h)
    129129   # AC_PROG_CPP triggers the search for a C compiler; use hack instead
    130    for cpp in /lib/cpp gcc cc c99
     130   for cpp in $CPP cpp gcc /lib/cpp cc c99
    131131   do
    132       test $cpp = /lib/cpp || cpp="$cpp -E"
     132     case $cpp in
     133       *cpp*) ;;
     134       *) cpp="$cpp -E" ;;
     135     esac
    133136      echo foo > conftest.c
    134137      if $cpp $CPPFLAGS conftest.c > /dev/null 2> /dev/null ; then
    135138         # Get CC

It brings things a bit closer to mpfr and adds $CPP to the list of possible preprocessors.

If yes, I can push it to the mpc trunk.

Thanks in advance,

Andreas

comment:22 Changed 8 years ago by leif

Replied off-trac:

...
Should work for me (will test later),
but $cpp should be quoted in 'case $cpp in ...',
and $CPP in the 'for' statement.

You could also go with

    for cpp in "$CPP" cpp "gcc -E" /lib/cpp "cc -E" "c99 -E"

and drop adding '-E' below.
...

comment:23 Changed 8 years ago by leif

P.S.:

   if test "x$GMP_CFLAGS" = "x__GMP_CFLAGS" -o "x$GMP_CC" = "x__GMP_CC" ; then
      AC_MSG_RESULT(no)
      GMP_CC=
      GMP_CFLAGS=
   else
      AC_MSG_RESULT(yes [CC=$GMP_CC CFLAGS=$GMP_CFLAGS])
   fi

could be changed to something like

   if test -z "$GMP_CFLAGS$GMP_CC" ; then
      AC_MSG_RESULT([no, no working preprocessor found; CPP=$CPP CPPFLAGS=$CPPFLAGS])
   elif test "x$GMP_CFLAGS" = "x__GMP_CFLAGS" -o "x$GMP_CC" = "x__GMP_CC" ; then
      AC_MSG_RESULT(no)
      GMP_CC=
      GMP_CFLAGS=
   else
      AC_MSG_RESULT(yes [CC=$GMP_CC CFLAGS=$GMP_CFLAGS])
   fi

comment:24 Changed 8 years ago by leif

The GMP CC / CFLAGS issue with cpp (or rather CPPFLAGS not working with the ones from the list) is now fixed upstream (MPC 1.1 branch); cf. https://gforge.inria.fr/tracker/index.php?func=detail&aid=17410&group_id=131&atid=607.

I'll perhaps open a follow-up ticket including that upstream patch, but it's a minor issue anyway (which apparently only hits me on occasion).

Note: See TracTickets for help on using tickets.