make install broken in Sage 4.8

[zimmerma]

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

2. Apply trac_12347.patch​ to SAGE_ROOT

[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

@@ -149 +149 @@
         mkdir -p $(DESTDIR)/sage
         mkdir -p $(DESTDIR)/bin/
         cp -rpv * $(DESTDIR)/sage/
-        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"
         cp $(DESTDIR)/sage/sage $(DESTDIR)/bin/
         cd $(DESTDIR)/bin/; ./sage -c
 

[ppurka]

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

Makefile

@@ -149 +149 @@
         mkdir -p $(DESTDIR)/sage
         mkdir -p $(DESTDIR)/bin/
         cp -rpv * $(DESTDIR)/sage/
-        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"
         cp $(DESTDIR)/sage/sage $(DESTDIR)/bin/
         cd $(DESTDIR)/bin/; ./sage -c
 

[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

[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

@@ -149 +149 @@
         mkdir -p $(DESTDIR)/sage
         mkdir -p $(DESTDIR)/bin/
         cp -rpv * $(DESTDIR)/sage/
-        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"
         cp $(DESTDIR)/sage/sage $(DESTDIR)/bin/
         cd $(DESTDIR)/bin/; ./sage -c
 

[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

[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.

[ppurka]

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.

[jhpalmieri]

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.

[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?

[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.

[ppurka]

Remove unnecessary file. Apply to SAGE_ROOT/local/bin

[ppurka]

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> 

[zimmerma]

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

Paul

[ppurka]

Sorry, forgot to set it to needs_review.

[jdemeyer]

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

[ppurka]

updated.

[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).

[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. :)

[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.

[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.

[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.

[ppurka]

Apply to SAGE_ROOT

[ppurka]

Updated.

[ppurka]

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

[jdemeyer]

Replying to ppurka:

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

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

[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.

[ppurka]

Thanks!

[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

[jdemeyer]

Replying to zimmerma:

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

Why? What would you propose?

[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

[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.

[zimmerma]

ok, then I give a positive review.

Paul

[jdemeyer]

Replying to zimmerma:

ok, then I give a positive review.

Agreed.