Opened 8 years ago

Closed 5 years ago

#9493 closed task (fixed)

Remove extra baggage from the ECL spkg

Reported by: leif Owned by: tbd
Priority: major Milestone: sage-6.3
Component: packages: standard Keywords: ecl gmp cygwin spkg
Cc: drkirkby, kcrisman Merged in:
Authors: Leif Leonhardy, Jean-Pierre Flori Reviewers: Peter Bruin
Report Upstream: Reported upstream. No feedback yet. Work issues:
Branch: bec6426 (Commits) Commit: bec64268f80d6a4eb9c89bdbe83452459d665e7c
Dependencies: Stopgaps:

Change History (27)

comment:1 follow-up: Changed 8 years ago by leif

Minimal patch to configure to allow rm -r src/src/gmp:

  • configure

    old new  
    19871987
    19881988
    19891989ac_aux_dir=
    1990 for ac_dir in ${srcdir}/gmp "$srcdir"/${srcdir}/gmp; do
     1990for ac_dir in ${srcdir}/gmp "$srcdir"/${srcdir}/gmp ${srcdir}/gc; do
    19911991  if test -f "$ac_dir/install-sh"; then
    19921992    ac_aux_dir=$ac_dir
    19931993    ac_install_sh="$ac_aux_dir/install-sh -c"

(Tested with 4.5.rc0 on a 32-bit Linux, with --with-gmp-prefix=$SAGE_LOCAL added to ./configure in spkg-install, but should work as fine without that.)

Though we should in mid-term remove (Boehm) gc as well, because Sage ships with its own copy of it. (ECL's boehm_gc is only used on MacOS X, and just because ECL unconditionally thinks an already installed version there can only be Fink's broken one.) But this is worth another ticket.

Another simple solution is just leaving install-sh in src/src/gmp (untested) or just copying it to ${srcdir} and adding that directory to the for list.

comment:2 Changed 8 years ago by leif

Modified spkg-install to allow removal of GMP (suggestion):

  • ecl-10.2.1.p2/spkg-install

    old new  
    154154# We clear MAKEFLAGS to fix building multiple spkgs in parallel on OS X.
    155155export MAKEFLAGS=
    156156
     157if [ -d patches ] && [ `ls patches` != "" ]; then
     158  echo "Applying patches..."
     159
     160    test -f patches/src.src.configure && cp -pv patches/src.src.configure src/src/configure
     161
     162  echo "Finished applying patches."
     163  echo " "
     164fi
     165
    157166set +e
    158167
    159168cd src
     
    165174   # 1) OpenSolaris (SunOS 5.11)
    166175   # 2) Intel or AMD CPU
    167176   # 3) 64-bit build
    168    ./configure --prefix=$SAGE_LOCAL --with-dffi=no
     177   ./configure --prefix=$SAGE_LOCAL --with-dffi=no --with-gmp-prefix=$SAGE_LOCAL
    169178else
    170    ./configure --prefix=$SAGE_LOCAL
     179   ./configure --prefix=$SAGE_LOCAL --with-gmp-prefix=$SAGE_LOCAL
    171180fi
    172181
    173182if [ $? -ne 0 ]; then

(v1's spkg-install is unchanged w.r.t. ECL 10.2.1.p1.)

comment:3 Changed 8 years ago by drkirkby

Get rid of the '-v' option to 'cp'. It's not a POSIX option

http://www.opengroup.org/onlinepubs/009695399/utilities/cp.html

and will certainly fail on Solaris or OpenSolaris

Dave

comment:4 Changed 8 years ago by leif

What about

copy-patch()
{
  if [ -f patches/$1 ] ; then
    echo "  Patching "$2
    cp -p patches/$1 $2
  fi
}

and (e.g.)

if [ -d patches ] && [ `ls patches` != "" ]; then
  echo "Applying patches..." 

  copy-patch "src.src.configure" "src/src/configure"

  echo "Finished applying patches." 
  echo " " 
fi      

?

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

I don't think

if [ -d patches ] && [ `ls patches` != "" ]; then

is safe. I don't think the order is guaranteed, so the second part could be evaluated before the first. Testing on "" is bad practice.

Dave

comment:6 in reply to: ↑ 5 Changed 8 years ago by leif

Replying to drkirkby:

I don't think

if [ -d patches ] && [ `ls patches` != "" ]; then

is safe. I don't think the order is guaranteed, so the second part could be evaluated before the first.

(Even if it was, it doesn't make a difference, but...)

Like in C, the second expression is only evaluated if needed (same for ||), such that

foo && bar || baz

is equivalent to

if foo; then
    bar
else
    baz
fi

However, the [ `ls patches` != "" ] is suboptimal. The whole line could be

if ls patches/* >/dev/null 2>/dev/null; then

(One could substitute ls by e.g. cat, too.) It was just one suggestion anyway.

comment:7 in reply to: ↑ 5 Changed 8 years ago by nbruin

Replying to drkirkby:

I don't think the order is guaranteed, so the second part could be evaluated before the first. Testing on "" is bad practice.

The order of evaluation and the shortcutting is guaranteed by POSIX:

http://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_03

comment:8 follow-up: Changed 8 years ago by drkirkby

I believe this should be closed, and if further cleanups are required, a new ticket is opened.

ECL will be updated in Sage 4.6.1 to 10.4.1 in #10187. Most of the changes on this ticket are incorporated into #10187 anyway.

Dave

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

Replying to drkirkby:

I believe this should be closed, and if further cleanups are required, a new ticket is opened.

ECL will be updated in Sage 4.6.1 to 10.4.1 in #10187. Most of the changes on this ticket are incorporated into #10187 anyway.

Nothing of the TODOs mentioned in the description have been done on #10187; removing the other parts was just the correction of a regression.

So IMHO this ticket should be kept open.

comment:10 Changed 8 years ago by leif

  • Summary changed from Remove extra baggage from ECL 10.2.1.p1 (again) to Remove extra baggage from the ECL spkg

comment:11 Changed 7 years ago by kcrisman

  • Cc kcrisman added

comment:12 in reply to: ↑ 1 Changed 5 years ago by leif

Replying to leif:

Minimal patch to configure to allow rm -r src/src/gmp:

  • configure

    old new  
    19871987
    19881988
    19891989ac_aux_dir=
    1990 for ac_dir in ${srcdir}/gmp "$srcdir"/${srcdir}/gmp; do
     1990for ac_dir in ${srcdir}/gmp "$srcdir"/${srcdir}/gmp ${srcdir}/gc; do
    19911991  if test -f "$ac_dir/install-sh"; then
    19921992    ac_aux_dir=$ac_dir
    19931993    ac_install_sh="$ac_aux_dir/install-sh -c"

No idea whether that's at all still necessary (when removing the unused GMP source tree) -- we're meanwhile at ECL 12.12.1...

Another simple solution is just leaving install-sh in src/src/gmp (untested) or just copying it to ${srcdir} and adding that directory to the for list.

comment:13 Changed 5 years ago by leif

Ooops, just noticed:

$ tar tvjf spkg/standard/ecl-* | grep gmp
tar: Record size = 8 blocks
drwxr-xr-x jdemeyer/jdemeyer      0 2013-04-08 13:05 ecl-12.12.1.p4/src/src/gmp/
-rwxr-xr-x jdemeyer/jdemeyer   4105 2012-12-07 22:01 ecl-12.12.1.p4/src/src/gmp/config.sub
-rwxr-xr-x jdemeyer/jdemeyer  31164 2012-12-07 22:01 ecl-12.12.1.p4/src/src/gmp/configfsf.sub
-rwxr-xr-x jdemeyer/jdemeyer  23041 2012-12-07 22:01 ecl-12.12.1.p4/src/src/gmp/config.guess
-rwxr-xr-x jdemeyer/jdemeyer   9505 2012-12-07 22:01 ecl-12.12.1.p4/src/src/gmp/install-sh
-rwxr-xr-x jdemeyer/jdemeyer  43636 2012-12-07 22:01 ecl-12.12.1.p4/src/src/gmp/configfsf.guess
-rw-r--r-- jdemeyer/jdemeyer  15353 2012-12-07 22:01 ecl-12.12.1.p4/src/src/gmp/config.in

So there's meanwhile not much of GMP left (in our spkg), but its config* files still disturb: #14648

Hence the install-sh fix above should still be valid (and useful) ...

comment:14 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:15 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:16 Changed 5 years ago by jpflori

  • Authors changed from Leif Leonhardy to Leif Leonhardy, Jean-Pierre Flori
  • Branch set to u/jpflori/ticket/9493
  • Commit set to ad497e751543fc52941a8b6affe0a9de55d55043
  • Keywords ecl gmp cygwin spkg added
  • Status changed from new to needs_review

Let's get rid of the gmp subdirectory.


New commits:

ad497e7Strip GMP out of ECL and use up to date config.* scripts.

comment:17 Changed 5 years ago by jpflori

  • Description modified (diff)

I think the previously rmoved encoding stuff is actually planned to be used, so I did not change its inclusion.

comment:18 Changed 5 years ago by git

  • Commit changed from ad497e751543fc52941a8b6affe0a9de55d55043 to bec64268f80d6a4eb9c89bdbe83452459d665e7c

Branch pushed to git repo; I updated commit sha1. New commits:

bec6426Check that FSF config.* scripts are correctly copied from SAGE_ROOT/config.

comment:19 Changed 5 years ago by vbraun

IMHO if we already make our own tarball then we should just regenerate auto-files.

comment:20 Changed 5 years ago by jpflori

I'm not sure that will much cleaner as we patch the auto-files as well, so spkg-src should first patch then regenerate vs copy two files and then patch...

I don't have strong feeling though.

comment:21 Changed 5 years ago by vbraun

Ugh, once again somebody went the extra mile to show that you can't reasonably build shared libraries without libtool.

Since ECL seems to have a maintainer again, did you try to push the implib.patch upstream?

comment:23 Changed 5 years ago by vbraun

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:24 Changed 5 years ago by jpflori

Note that the implib patch is not part of this ticket. But for what it's worth note that I used to communicate with the ECL dev and got a bunch of patches into ECL but not that one and now he's no longer maintaining ECL. Not sure if anyone is actually actively maintaining ECL anymore... Once there is someone clearly identified we could just bump the request to get that in as at least from my point of view it perfectly makes sense to build shared lib this way on Cygwin.

comment:25 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:26 Changed 5 years ago by pbruin

  • Reviewers set to Peter Bruin
  • Status changed from needs_review to positive_review

The changes look good to me, and the new version of the package built without problems. After reinstalling Maxima (I did not use SAGE_UPGRADING), all doctests passed.

comment:27 Changed 5 years ago by vbraun

  • Branch changed from u/jpflori/ticket/9493 to bec64268f80d6a4eb9c89bdbe83452459d665e7c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.