search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.35k stars 460 forks source link

Fix hardcoding of paths in R binary #9668

Closed kcrisman closed 11 years ago

kcrisman commented 14 years ago

See this thread on sage-support.

Here is how I got the optional package automap to install into a 
binary sage R. 
Go into the sage directory and edit the following files: 
local/bin/R and local/lib/R/bin/R 
and change all the hard-set user variables "/scratch/...." to the true 
locations of R_HOME_DIR, R_HOME, R_INCLUDE_DIR, R_SHARE_DIR and for 
good measure, R_DOC_DIR. Replace the default string EVERYWHERE in the 
file. 
I then exported SAGE_HOME as well (Not sure that this is needed.), and 
run local/bin/R 
Inside R, install.packages("automap") 
No more build errors, and when I restart R, automap loads using 
library. Just have to try it out from sage now. 
Any chance there's a script to find all of these hard-set strings and 
change them to correct values?

Related (R package):

New spkg: http://boxen.math.washington.edu/home/palmieri/SPKG/r-2.15.2.p2.spkg

CC: @nexttime @jhpalmieri

Component: packages: standard

Keywords: R spkg R.sh.in libR.pc pkg-config hard-coded package installation R_HOME_DIR sd32 r-project

Author: John Palmieri

Reviewer: Karl-Dieter Crisman, Jeroen Demeyer

Merged: sage-5.9.rc0

Issue created by migration from https://trac.sagemath.org/ticket/9668

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

Description changed:

--- 
+++ 
@@ -17,3 +17,11 @@
 Any chance there's a script to find all of these hard-set strings and 
 change them to correct values?

+ +--- + +Related (R package): + #9906 + #9847 +* #8274 +

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:2

IIRC I have some "almost" ready R spkg for #9906, which also does a lot of clean-up in spkg-install...

kcrisman commented 13 years ago
comment:3

Well, I would certainly welcome and help review this. Anything which cleans up parts of #8274 could be commented there, and perhaps even a new ticket opened if there isn't much left.

But what about #9668 (this ticket) itself? Does the stuff that is "almost" ready at #9906 address that?

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:4

Replying to @kcrisman:

But what about #9668 (this ticket) itself? Does the stuff that is "almost" ready at #9906 address that?

Not yet, i.e. not all of it. See this comment there.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:5

Replying to @nexttime:

Replying to @kcrisman:

But what about #9668 (this ticket) itself? Does the stuff that is "almost" ready at #9906 address that?

Not yet, i.e. not all of it. See this comment there.

Ok, a bit more subtle than I expected (R is quite weird, failed to build inbetween), but I now have a p6 that fixes both the R scripts and the pkg-config file (libR.pc).

There was another issue with R someone reported on sage-devel which was caused by a "complicated" setting of an environment variable, in this case PAGER, but that's certainly an upstream problem and rather exotic.

kcrisman commented 13 years ago
comment:6

Replying to @nexttime:

Replying to @nexttime:

Replying to @kcrisman:

But what about #9668 (this ticket) itself? Does the stuff that is "almost" ready at #9906 address that?

Not yet, i.e. not all of it. See this comment there.

Ok, a bit more subtle than I expected (R is quite weird, failed to build inbetween), but I now have a p6 that fixes both the R scripts and the pkg-config file (libR.pc).

Okay, I'll watch this space.

There was another issue with R someone reported on sage-devel which was caused by a "complicated" setting of an environment variable, in this case PAGER, but that's certainly an upstream problem and rather exotic.

Agreed.

kcrisman commented 13 years ago
comment:7

See also #10967. We should probably either incorporate that here, or make a followup spkg there.

kcrisman commented 13 years ago

Description changed:

--- 
+++ 
@@ -24,4 +24,4 @@
 * #9906
 * #9847
 * #8274
-
+* #10967
83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:8

Replying to @kcrisman:

See also #10967. We should probably either incorporate that here, or make a followup spkg there.

Almost certainly the former.

[Note to myself:]

The problem is that removing any reference to SAGE_ROOT and SAGE_LOCAL disables the script being "automatically" relocated in the first place.

I'll then have to guess SAGE_ROOT if none of Sage's variables are defined. We also have two copies of the R script, one in local/bin/, and one in local/lib/R/bin/.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

Dependencies: #9906

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

Changed keywords from none to R spkg R.sh.in libR.pc pkg-config hard-coded package installation R_HOME_DIR

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:9

Replying to @nexttime:

[Note to myself:]

The problem is that removing any reference to SAGE_ROOT and SAGE_LOCAL disables the script being "automatically" relocated in the first place.

I'll then have to guess SAGE_ROOT if none of Sage's variables are defined. We also have two copies of the R script, one in local/bin/, and one in local/lib/R/bin/.

Guessing SAGE_ROOT there is simply bullshit. Simply add a sanity check, pointing to sage --R, install_scripts() and perhaps sage --sh in case SAGE_LOCAL isn't defined.

Furthermore:

The problem was with hard-coded paths, not the 
permissions.  Anyway, the fix was easy.  I opened all the files listed 
above by George: 
sage/local/lib/R/bin/R 
sage/local/lib/R/bin/libtool 
sage/local/lib/R/etc/Makeconf 
sage/local/lib/R/etc/ldpath 
sage/local/lib/R/etc/Renviron 
sage/local/bin/R 
and edited obvious lines containing hardcoded paths (using find- 
replace-all at once).
83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

Work Issues: Provide an R 2.10.1.p6 spkg.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:10

Replying to @nexttime:

  • Change libR.pc to use either ${pc_top_builddir} or $${SAGE_ROOT}. (In the former case, perhaps also set PC_TOP_BUILD_DIR to $SAGE_ROOT, or prepend $SAGE_ROOT:, unless it is already contained.)

Should read:

williamstein commented 13 years ago

Changed keywords from R spkg R.sh.in libR.pc pkg-config hard-coded package installation R_HOME_DIR to R spkg R.sh.in libR.pc pkg-config hard-coded package installation R_HOME_DIR sd32

kcrisman commented 12 years ago

Changed keywords from R spkg R.sh.in libR.pc pkg-config hard-coded package installation R_HOME_DIR sd32 to R spkg R.sh.in libR.pc pkg-config hard-coded package installation R_HOME_DIR sd32 r-project

jhpalmieri commented 11 years ago
comment:14

This problem prevents Sage from being relocatable on Solaris, or at least on the skynet machines mark and mark2: if I build Sage, then move the entire Sage directory, then run sage so the relocation scripts get executed, and then run sage -R, I get an error:

$ ./sage -R
ld.so.1: R: fatal: libgcc_s.so.1: version `GCC_4.3.0' not found (required by file /usr/local/gcc-4.7.0/sparc-SunOS-ultrasparc3/lib/libgomp.so.1)
ld.so.1: R: fatal: libgcc_s.so.1: open failed: No such file or directory
/home/palmieri/mark2/sage-5.4.rc2-7797/spkg/bin/sage: line 457: 28710 Killed                  "$SAGE_LOCAL/bin/R" "$@"

I tried just modifying local/bin/R and local/lib/R/bin/R, replacing the hard-coded paths with $SAGE_ROOT, but I still got an error.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 11 years ago
comment:15

I doubt this is related to the original topic at all. The setup on skynet's Solaris machines is (or used to be) quite broken, as there are outdated versions of shared libraries left around in typical paths, and some R scripts insist on messing up your paths such that the former gets relevant.

Unfortunately I don't recall what the exact problem was, and how I managed to work around it, just that I had to [and somehow successfully did]; I think this problem appeared with, or was related to, [the installation of] GCC 4.7.0. In case I am right, changing PATH and/or LD_LIBRARY_PATH should "solve" the issue.

Sorry for not being of much help here, at least right now... ;-)

jhpalmieri commented 11 years ago
comment:16

Well, if the problems on mark are not related, then for this ticket, we could just add some lines at the end of spkg-install:

# Make R relocatable by using "$SAGE_ROOT" instead of the hardcoded path.
sed -e "s|$SAGE_ROOT|\$SAGE_ROOT/|" "$SAGE_LOCAL/bin/R" > "$SAGE_LOCAL/bin/R.tmp" && mv "$SAGE_LOCAL/bin/R.tmp" "$SAGE_LOCAL/bin/R" && chmod a+x "$SAGE_LOCAL/bin/R"
sed -e "s|$SAGE_ROOT|\$SAGE_ROOT/|" "$SAGE_LOCAL/lib/R/bin/R" > "$SAGE_LOCAL/lib/R/bin/R.tmp" && mv "$SAGE_LOCAL/lib/R/bin/R.tmp" "$SAGE_LOCAL/lib/R/bin/R" && chmod a+x "$SAGE_LOCAL/lib/R/bin/R"

(It's too bad that the -i flag for sed is not portable.) libR.pc already uses SAGE_ROOT. What else needs to be done?

jhpalmieri commented 11 years ago

Author: John Palmieri

jhpalmieri commented 11 years ago

Description changed:

--- 
+++ 
@@ -25,3 +25,8 @@
 * #9847
 * #8274
 * #10967
+
+---
+
+New spkg: [http://sage.math.washington.edu/home/palmieri/SPKG/r-2.14.0.p7.spkg](http://sage.math.washington.edu/home/palmieri/SPKG/r-2.14.0.p7.spkg)
+
jhpalmieri commented 11 years ago
comment:17

I've posted a new spkg, along with the corresponding patch.

jhpalmieri commented 11 years ago

Changed work issues from Provide an R 2.10.1.p6 spkg. to none

jhpalmieri commented 11 years ago
comment:19

Replying to @nexttime:

  • Address / check the following (copied from [comment:ticket:10967:14]):
The problem was with hard-coded paths, not the 
permissions.  Anyway, the fix was easy.  I opened all the files listed 
above by George: 
sage/local/lib/R/bin/R 
sage/local/lib/R/bin/libtool 
sage/local/lib/R/etc/Makeconf 
sage/local/lib/R/etc/ldpath 
sage/local/lib/R/etc/Renviron 
sage/local/bin/R 
and edited obvious lines containing hardcoded paths (using find- 
replace-all at once).

It's easy enough to add these to the "for" loop that I added to spkg-install, so I might as well do that. For what it's worth, the binary files Rscript in both local/bin and local/lib/R/bin also have the path hard-coded in them, but I don't know how to fix this, or whether it's important. When are those files used?

jhpalmieri commented 11 years ago

Attachment: trac_9668-r.patch.gz

patch for R spkg; for review only

kcrisman commented 11 years ago
comment:20

This patch for the spkg makes sense to me, anyway.

Did you end up changing the libR.pc stuff in comment:10? Or was that done elsewhere?

kcrisman commented 11 years ago

Changed dependencies from #9906 to none

jhpalmieri commented 11 years ago
comment:21

The ".pc" files are handled by the sage-location script, I think. In any case, I believe that if you run make build on a fresh Sage tarball, then libR.pc should be in good shape.

kcrisman commented 11 years ago

Reviewer: Karl-Dieter Crisman

kcrisman commented 11 years ago
comment:22

This looks good, and I now recall the sage-location discussion. I think this is ready to go; it behaves as advertised. I can't check whether this fixes the problems on Solaris directly, but it should fix the problem in the original post too - well, if such a person were to upgrade the spkg without just downloading a new Sage.

jhpalmieri commented 11 years ago
comment:23

A slight correction to what I said before: just doing make build doesn't fix libR.pc, but running Sage once takes care of it.

kcrisman commented 11 years ago
comment:24

Or anything else that triggers sage-location (nowadays, any spkg upgrade even), yes.

jdemeyer commented 11 years ago
comment:25

I think using sed for this is very fragile, as any special characters in $SAGE_ROOT might cause this script to malfunction. A better solution would be to patch the R sources. This is already partially done (see R.sh.patch in the R sources) to allow for basic relocation of R, but apparently installing R packages doesn't work.

kcrisman commented 11 years ago
comment:26

Replying to @jdemeyer:

I think using sed for this is very fragile, as any special characters in $SAGE_ROOT might cause this script to malfunction. A better

Out of curiosity, do we have any current restrictions on the characters in $SAGE_ROOT? I feel like we already don't allow whitespace, though that might be an old memory. Does Sage work in non-ASCII paths as well, say with Cyrillic characters?

jdemeyer commented 11 years ago
comment:27

Replying to @kcrisman:

Out of curiosity, do we have any current restrictions on the characters in $SAGE_ROOT?

We certainly do, although only "no spaces" is documented.

But I doubt Sage will work correctly with a directory like /home/jdemeyer/.#|"+*=@$' (which is a valid UNIX filename)

kcrisman commented 11 years ago
comment:28

Out of curiosity, do we have any current restrictions on the characters in $SAGE_ROOT?

We certainly do, although only "no spaces" is documented.

But I doubt Sage will work correctly with a directory like /home/jdemeyer/.#|"+*=@$' (which is a valid UNIX filename)

You sound like you're arguing that we might as well use sed :-) If I wasn't lazy I'd try to grep through other spkgs to see if it's used elsewhere - I'm almost sure I've seen it in use before...

jhpalmieri commented 11 years ago

Attachment: trac_9668-r.v2.patch.gz

patch for R spkg; for review only

jhpalmieri commented 11 years ago
comment:29

Here is another attempt at fixing this.

jhpalmieri commented 11 years ago
comment:31

Wishful thinking: I accidentally set this to "positive review". Oops.

I think the changes in attachment: trac_9668-r.v2.patch could be polished a bit, but I'm not going to bother until people agree with the general approach.

jdemeyer commented 11 years ago
comment:32

I don't like this at all:

./configure --prefix="\$SAGE_LOCAL"

If it works, you're probably relying on a bug. Why is it needed?

Also, the first hunk in Makefile.in.patch is not needed.

In any case, this needs to be rebased as R has been upgraded.

jhpalmieri commented 11 years ago
comment:33

Replying to @jdemeyer:

I don't like this at all:

./configure --prefix="\$SAGE_LOCAL"

If it works, you're probably relying on a bug. Why is it needed?

If I remember correctly, the prefix directory gets written into various of the files in SAGE_LOCAL/lib/R/bin/, so using \$SAGE_LOCAL instead of $SAGE_LOCAL means that "$SAGE_LOCAL" gets written verbatim to the file, instead of expanded first and then written. This makes those files relocatable.

Why don't you like it?

jdemeyer commented 11 years ago
comment:34

Replying to @jhpalmieri:

Why don't you like it?

Because the fact that it works looks like a bug rather than a feature.

jhpalmieri commented 11 years ago
comment:35

If you look at src/scripts/R.sh.in, it has lines like

        R_HOME_DIR="@prefix@/${libnn}/R"

My understanding is that prefix gets set by the configure script, stored in Makefile.conf, and then read by src/scripts/Makefile so this variable's value can get used when making the R script. In particular, the R people are deliberately using the prefix variable here. So it looks like their design decision, not a bug. But I'm not sure I understand your point.

jdemeyer commented 11 years ago
comment:36

Replying to @jhpalmieri:

If you look at src/scripts/R.sh.in, it has lines like

        R_HOME_DIR="@prefix@/${libnn}/R"

This line is completely irrelevant, as we already patch R.sh.in to overwrite R_HOME_DIR.

jhpalmieri commented 11 years ago

Attachment: trac_9668-r.v3.patch.gz

jhpalmieri commented 11 years ago
comment:37

I'll post a new version of the spkg when sage.math is back up. I've attached the patch. This has the effect of changing lines in local/bin/R (and the corresponding file local/lib/R/bin/R) from

R_SHARE_DIR=.../sage-5.8.beta4/local/lib/R/share
export R_SHARE_DIR
R_INCLUDE_DIR=.../sage-5.8.beta4/local/lib/R/include
export R_INCLUDE_DIR
R_DOC_DIR=.../sage-5.8.beta4/local/lib/R/doc
export R_DOC_DIR

to

R_SHARE_DIR="${R_HOME_DIR}/share"
export R_SHARE_DIR
R_INCLUDE_DIR="${R_HOME_DIR}/include"
export R_INCLUDE_DIR
R_DOC_DIR="${R_HOME_DIR}/doc"
export R_DOC_DIR

With this change, running sage -R and then install.packages("automap") seems to work.

jhpalmieri commented 11 years ago

Description changed:

--- 
+++ 
@@ -28,5 +28,4 @@

 ---

-New spkg: [http://sage.math.washington.edu/home/palmieri/SPKG/r-2.14.0.p7.spkg](http://sage.math.washington.edu/home/palmieri/SPKG/r-2.14.0.p7.spkg)
-
+New spkg: [http://boxen.math.washington.edu/home/palmieri/SPKG/r-2.15.2.p2.spkg](http://boxen.math.washington.edu/home/palmieri/SPKG/r-2.15.2.p2.spkg)
jhpalmieri commented 11 years ago
comment:38

New spkg posted.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 11 years ago
comment:39

Replying to @jhpalmieri:

I'll post a new version of the spkg when sage.math is back up.

Replying to @jhpalmieri:

New spkg posted.

I still cannot log into sage.math... ;-)

jhpalmieri commented 11 years ago
comment:40

Replying to @nexttime:

Replying to @jhpalmieri:

I'll post a new version of the spkg when sage.math is back up.

Replying to @jhpalmieri:

New spkg posted.

I still cannot log into sage.math... ;-)

I did an scp to boxen. If you want, I can try to keep my promise by posting the new version (again) when sage.math comes back up...