Opened 10 years ago

Closed 10 years ago

#12347 closed defect (fixed)

make install broken in Sage 4.8

Reported by: zimmerma Owned by: GeorgSWeber
Priority: major Milestone: sage-5.0
Component: build Keywords:
Cc: Merged in: sage-5.0.beta14
Authors: Punarbasu Purkayastha Reviewers: Jeroen Demeyer, Paul Zimmermann
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by ppurka)

when I compile Sage 4.8 from source in say /tmp/sage-4.8, then I do make install DESTDIR=/usr/local/sage-4.8, and I run the binary /usr/local/sage-4.8/bin/sage I get:

[zimmerma@coing ~]$ /usr/local/sage-4.8/bin/sage
************************************************************************
You must compile Sage first using 'make' in the Sage root directory.
(If you have already compiled Sage, you must set the SAGE_ROOT variable
in the file '/usr/local/sage-4.8/bin/sage').
************************************************************************

This way of compiling and installing Sage worked before, up to Sage 4.7.2.

A fix is to add the line

SAGE_ROOT="/usr/local/sage-4.8/sage

in the script /usr/local/sage-4.8/bin/sage. This was done automatically by make install up to 4.7.2, and was broken in Sage 4.8.

Paul


Hard coding of SAGE_ROOT is removed from make install now. We simply do a symbolic link.

  1. Apply trac_12347-remove-sage-hardcode.patch to SAGE_ROOT/local/bin
  1. Apply trac_12347.patch to SAGE_ROOT

Attachments (2)

trac_12347-remove-sage-hardcode.patch (600 bytes) - added by ppurka 10 years ago.
Remove unnecessary file. Apply to SAGE_ROOT/local/bin
trac_12347.patch (1.2 KB) - added by ppurka 10 years ago.
Apply to SAGE_ROOT

Download all attachments as: .zip

Change History (37)

comment:1 Changed 10 years ago by ppurka

The sage directory is relocatable and so simply moving the directory from /tmp/sage-4.8 to /usr/local/sage-4.8 will enable you to run sage again. After moving the folder you should also run sage just once ( /usr/local/sage-4.8/sage ) so that it resolves all the paths.

As for make install being broken, it seems to happen because there is no SAGE_ROOT="....." in the file anymore and so that hard coding doesn't get done any more. A simple fix should be this patch to the Makefile in your /tmp/sage-4.8/Makefile:

  • Makefile

    old new  
    149149        mkdir -p $(DESTDIR)/sage
    150150        mkdir -p $(DESTDIR)/bin/
    151151        cp -rpv * $(DESTDIR)/sage/
    152         python local/bin/sage-hardcode_sage_root $(DESTDIR)/sage/sage "$(DESTDIR)"/sage
     152        sed -i -e "s@^#SAGE_ROOT=/path/.*$@SAGE_ROOT=$(DESTDIR)/sage@;q" "$(DESTDIR)/sage/sage"
    153153        cp $(DESTDIR)/sage/sage $(DESTDIR)/bin/
    154154        cd $(DESTDIR)/bin/; ./sage -c
    155155

comment:2 Changed 10 years ago by ppurka

Hmm.. using @ is not a good idea there. The correct patch should be:

  • Makefile

    old new  
    149149        mkdir -p $(DESTDIR)/sage
    150150        mkdir -p $(DESTDIR)/bin/
    151151        cp -rpv * $(DESTDIR)/sage/
    152         python local/bin/sage-hardcode_sage_root $(DESTDIR)/sage/sage "$(DESTDIR)"/sage
     152        sed -i -e "s|^#SAGE_ROOT=/path/.*$|SAGE_ROOT=\"$(DESTDIR)/sage\"|;q" "$(DESTDIR)/sage/sage"
    153153        cp $(DESTDIR)/sage/sage $(DESTDIR)/bin/
    154154        cd $(DESTDIR)/bin/; ./sage -c
    155155

comment:3 Changed 10 years ago by zimmerma

I tried your patch and got:

sed -i -e "s|^#SAGE_ROOT=/path/.*SAGE_ROOT=\"/usr/local/sage-4.8a/sage\"|;q" "/usr/local/sage-4.8a/sage/sage"
sed: -e expression #1, char 62: unterminated `s' command
make: *** [install] Error 1

Did you really test your patch?

[root@coing sage-4.8]# diff Makefile.orig Makefile
152c152
<       python local/bin/sage-hardcode_sage_root $(DESTDIR)/sage/sage "$(DESTDIR)"/sage
---
>       sed -i -e "s|^#SAGE_ROOT=/path/.*$|SAGE_ROOT=\"$(DESTDIR)/sage\"|;q" "$(DESTDIR)/sage/sage"

Paul

comment:4 Changed 10 years ago by ppurka

Sigh. Sorry for the trouble. I don't have sage-4.8 built yet. So, I can't test this as "make install". But I did test this sed expression before putting it here (that's how I caught the @ problem).

I see from

sed -i -e "s|^#SAGE_ROOT=/path/.*SAGE_ROOT=\"/usr/local/sage-4.8a/sage\"|;q"

that the $| was eaten away by the make process (it is not stripped away when run on the terminal). This is why you are getting that sed error. Can you try it without the $? Then the patch will look like this:

  • Makefile

    old new  
    149149        mkdir -p $(DESTDIR)/sage
    150150        mkdir -p $(DESTDIR)/bin/
    151151        cp -rpv * $(DESTDIR)/sage/
    152         python local/bin/sage-hardcode_sage_root $(DESTDIR)/sage/sage "$(DESTDIR)"/sage
     152        sed -i -e "s|^#SAGE_ROOT=/path/.*|SAGE_ROOT=\"$(DESTDIR)/sage\"|;q" "$(DESTDIR)/sage/sage"
    153153        cp $(DESTDIR)/sage/sage $(DESTDIR)/bin/
    154154        cd $(DESTDIR)/bin/; ./sage -c
    155155

comment:5 Changed 10 years ago by zimmerma

it still does not work. With the change of comment 4, the $(DESTDIR)/bin/sage file contains a single line:

tarte% cat /tmp/sage-4.8/bin/sage
#!/usr/bin/env bash

Paul

comment:6 Changed 10 years ago by ppurka

Ok. I will leave it up to someone else to come up with a fix to the python file local/bin/sage-hardcode_sage_root. I will check this ticket again after I have downloaded and built sage-4.8.

Thanks for your patience and feedback.

comment:7 Changed 10 years ago by ppurka

  • Status changed from new to needs_review

Ok. Here is a fix for the hard coding of the path. The simplest fix is to not do any hard coding at all!

Please apply trac_12347.patch to SAGE_ROOT (the directory where the Makefile is present). If the review is positive then we should also remove SAGE_ROOT/local/bin/sage-hardcode_sage_root from the tree since it is no longer needed.

I also ran into some problems with cp, in particular with this line in the Makefile:

cp -rpv * $(DESTDIR)/sage/

On the system that I tried this, cp actually errored out with error status 2. The last few lines of the make output are as follows:

$ make DESTDIR="/tmp/sage-install" install

.... tons of copying stuff ....

`spkg/logs/matplotlib-1.0.1.p0.log' -> `/tmp/sage-install/sage/spkg/logs/matplotlib-1.0.1.p0.log'
`spkg/logs/sqlite-3.7.5.p0.log' -> `/tmp/sage-install/sage/spkg/logs/sqlite-3.7.5.p0.log'
`start.log' -> `/tmp/sage-install/sage/start.log'
`tmp' -> `/tmp/sage-install/sage/tmp'
make: *** [install] Error 1

I can not fathom why it errors out. As a workaround, I changed the cp command to include -u and the cp finishes fine after invoking the make command once again:

$ make DESTDIR="/tmp/sage-install" install
echo "Experimental use only!"
Experimental use only!
if [ "/tmp/sage-install" = "" ]; then \
		echo "Set DESTDIR"; \
		exit 1; \
	fi
mkdir -p /tmp/sage-install
mkdir -p /tmp/sage-install/sage
mkdir -p /tmp/sage-install/bin/
cp -rpuv * /tmp/sage-install/sage/
`Makefile' -> `/tmp/sage-install/sage/Makefile'
cd /tmp/sage-install/bin; ln -sfn ../sage/sage ./sage; ./sage -c
The Sage installation tree may have moved
(from /home/punarbasu/Installations/sage-4.8 to /tmp/sage-install/sage).
Changing various hardcoded paths...
(Please wait at most a few minutes.)
DO NOT INTERRUPT THIS.
Done resetting paths.

comment:8 Changed 10 years ago by jhpalmieri

  • Status changed from needs_review to needs_work

You shouldn't use the options -u, -v, or -R for cp. Re -r, the man page for cp on OS X says

     Historic versions of the cp utility had a -r option.  This implementation supports that
     option; however, its use is strongly discouraged, as it does not correctly copy special
     files, symbolic links, or fifo's.

The OS X version of cp doesn't recognize the -u option at all. The Solaris version of cp doesn't support -u or -v.

Make sure to only use Posix-standard options; I think this web page is accurate.

Along the same lines, on Solaris (and perhaps other platforms), ln -f doesn't behave the way it does on linux; see the comment on line 71 of the file SAGE_ROOT/local/bin/sage-build.

comment:9 follow-up: Changed 10 years ago by ppurka

@jhpalmieri: I added -u to make sure that the second time make install is run, cp does not go around copying the files all over again. The options -r, -v were all present from before. ln -f was needed in case the user ran make install yet again, otherwise ln would give error. If the first invocation of make install doesn't fail, then there is no need to use -f with ln.

If cp is unreliable then what is the alternative (it is actually quite reliable on linux with the -a option, that is, it copies symlinks properly)? Alternatively, can we assume that rsync is available on all platforms and use rsync instead?

comment:10 in reply to: ↑ 9 Changed 10 years ago by jhpalmieri

Replying to ppurka:

@jhpalmieri: I added -u to make sure that the second time make install is run, cp does not go around copying the files all over again.

Solaris is a supported platform for Sage on which the -u option is not recognized, so it doesn't matter too much why you added it: it's not going to work.

The options -r, -v were all present from before.

But while you're fixing other things, you might as well fix this. Just use -R or -pR instead.

ln -f was needed in case the user ran make install yet again, otherwise ln would give error. If the first invocation of make install doesn't fail, then there is no need to use -f with ln.

Same comment as before: it's not going to work this way, so you should do it a different way. (For example, first run 'rm -f ...' on the link, then do 'ln -s ...'.)

If cp is unreliable then what is the alternative (it is actually quite reliable on linux with the -a option, that is, it copies symlinks properly)? Alternatively, can we assume that rsync is available on all platforms and use rsync instead?

I think that 'cp -pR' is reliable on all platforms. Note also that the 'make install' option says "Experimental use only!", so I don't think it's been well-maintained or supported, or even used much, which is why, for example, the -v option for cp wasn't removed earlier. But if you're working on this, you should do it right, so that it works on all supported platforms. Using POSIX-standard commands should be completely reliable; every platform's version of 'cp' should support the POSIX-standard options for cp.

Changed 10 years ago by ppurka

Remove unnecessary file. Apply to SAGE_ROOT/local/bin

comment:11 Changed 10 years ago by ppurka

  • Description modified (diff)

Updated the patches.

Now I see why cp failed:

...stallations/sage-4.8> make DESTDIR="/tmp/sage-xxxx" install |& tee -i -a ~/tmp/log-new.txt
echo "Experimental use only!"
Experimental use only!
if [ "/tmp/sage-xxxx" = "" ]; then \
		echo "Set DESTDIR"; \
		exit 1; \
	fi
mkdir -p /tmp/sage-xxxx
mkdir -p /tmp/sage-xxxx/sage
mkdir -p /tmp/sage-xxxx/bin/
cp -Rp * /tmp/sage-xxxx/sage/
cp: reading `local/lib/libpari.a': Input/output error
make: *** [install] Error 1

Rebuilding pari makes this bug go away

~/Installations/sage-4.8> ./sage -f spkg/standard/pari-2.5.0.p3.spkg >& ~/tmp/pari.txt
~/Installations/sage-4.8> cp local/lib/libpari.a /tmp
~/Installations/sage-4.8> 

comment:12 Changed 10 years ago by zimmerma

is the patch ready for review? No work issues were added in comment 8.

Paul

comment:13 Changed 10 years ago by ppurka

  • Status changed from needs_work to needs_review

Sorry, forgot to set it to needs_review.

comment:14 Changed 10 years ago by jdemeyer

  • Authors set to Punarbasu Purkayastha
  • Status changed from needs_review to needs_work

Replace

cd $(DESTDIR)/bin; ln -s ../sage/sage ./sage; ./sage -c

by

-rm -f $(DESTDIR)/bin/sage
ln -s ../sage/sage $(DESTDIR)/bin/sage
$(DESTDIR)/bin/sage -c   # Need to run sage-location

comment:15 Changed 10 years ago by ppurka

updated.

comment:16 Changed 10 years ago by ppurka

  • Status changed from needs_work to needs_review

comment:17 Changed 10 years ago by jdemeyer

Just one more thing: it would be better to first delete $(DESTDIR)/sage/ for two reasons:

  1. if there already was a previous version, there might be some kind of conflict (although I cannot immediately think of a concrete problem that could occur).
  2. some program in the old version might still be running (considering sage-cleaner, this is less unlikely than you might think at first).

comment:18 Changed 10 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work

comment:19 Changed 10 years ago by ppurka

Updated the patch. I guess we could write some logic to detect if a sage process is running, but it would not be robust enough and would bloat the install part.

I updated the patch to not proceed with the installation if the $(DESTDIR)/sage is a file or symlink. It will (hopefully) prevent accidents. :)

comment:20 Changed 10 years ago by ppurka

  • Status changed from needs_work to needs_review

comment:21 follow-up: Changed 10 years ago by jdemeyer

"[ -e" isn't portable. "[ -f" is portable.

Why do you talk about symlinks in your comment? I don't see that in your code.

I would simply remove the whole elif block. mkdir -p "$(DESTDIR)"/sage should fail anyway if "$(DESTDIR)"/sage is a non-directory.

comment:22 Changed 10 years ago by jdemeyer

Mention in a comment that the "[ -d" test is meant to exclude the case that "$(DESTDIR)"/sage is an existing file, not a directory.

comment:23 in reply to: ↑ 21 Changed 10 years ago by ppurka

Replying to jdemeyer:

Why do you talk about symlinks in your comment? I don't see that in your code.

Well, -e checks for both. I didn't know it wasn't portable. You are correct that mkdir will fail. I hadn't thought of that.

Changed 10 years ago by ppurka

Apply to SAGE_ROOT

comment:24 Changed 10 years ago by ppurka

Updated.

comment:25 follow-up: Changed 10 years ago by ppurka

Actually -e should be portable, going by this page.

comment:26 in reply to: ↑ 25 Changed 10 years ago by jdemeyer

Replying to ppurka:

Actually -e should be portable, going by this page.

Maybe it "should be", but it isn't.

comment:27 Changed 10 years ago by jdemeyer

For shell script portability discussions, I often refer to the GNU autoconf manual: http://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.68/html_node/Portable-Shell.html#Portable-Shell.

Quoting from there:

But occasionally you may find it necessary to check whether some arbitrary file exists. To do so, use ‘test -f’ or ‘test -r’. Do not use ‘test -x’, because 4.3BSD does not have it. Do not use ‘test -e’ either, because Solaris /bin/sh lacks it. To test for symbolic links on systems that have them, use ‘test -h’ rather than ‘test -L’; either form conforms to Posix 1003.1-2001, but older shells like Solaris 8 /bin/sh support only -h.

comment:28 Changed 10 years ago by ppurka

Thanks!

comment:29 follow-up: Changed 10 years ago by zimmerma

the patches apply successfully to sage-5.0.beta13 and make install works. My only comment after looking at the patch: shouldn't we check also if $(DESTDIR)/bin already exists?

Paul

comment:30 in reply to: ↑ 29 Changed 10 years ago by jdemeyer

Replying to zimmerma:

shouldn't we check also if $(DESTDIR)/bin already exists?

Why? What would you propose?

comment:31 follow-up: Changed 10 years ago by zimmerma

if we assume that $(DESTDIR)/sage can already exist, the same can happen for $(DESTDIR)/bin with a previous installation of Sage. What will then happen? I propose to remove $(DESTDIR)/bin if it exists and is a directory.

Paul

comment:32 in reply to: ↑ 31 Changed 10 years ago by jdemeyer

Replying to zimmerma:

if we assume that $(DESTDIR)/sage can already exist, the same can happen for $(DESTDIR)/bin with a previous installation of Sage. What will then happen?

Nothing bad will happen. The file or symbolic link $(DESTDIR)/bin/sage will be deleted and recreated as symbolic link to $(DESTDIR)/sage/sage.

comment:33 follow-up: Changed 10 years ago by zimmerma

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Paul Zimmermann
  • Status changed from needs_review to positive_review

ok, then I give a positive review.

Paul

comment:34 in reply to: ↑ 33 Changed 10 years ago by jdemeyer

Replying to zimmerma:

ok, then I give a positive review.

Agreed.

comment:35 Changed 10 years ago by jdemeyer

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