Opened 9 years ago

Closed 6 years ago

#9668 closed defect (fixed)

Fix hardcoding of paths in R binary

Reported by: kcrisman Owned by: leif
Priority: major Milestone: sage-5.9
Component: packages: standard Keywords: R spkg R.sh.in libR.pc pkg-config hard-coded package installation R_HOME_DIR sd32 r-project
Cc: leif, jhpalmieri Merged in: sage-5.9.rc0
Authors: John Palmieri Reviewers: Karl-Dieter Crisman, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jhpalmieri)

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

Attachments (3)

trac_9668-r.patch (1.5 KB) - added by jhpalmieri 6 years ago.
patch for R spkg; for review only
trac_9668-r.v2.patch (3.9 KB) - added by jhpalmieri 6 years ago.
patch for R spkg; for review only
trac_9668-r.v3.patch (2.4 KB) - added by jhpalmieri 6 years ago.

Download all attachments as: .zip

Change History (53)

comment:1 Changed 9 years ago by leif

  • Cc leif added

comment:2 Changed 8 years ago by leif

  • Description modified (diff)

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

comment:3 follow-up: Changed 8 years ago by kcrisman

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?

comment:4 in reply to: ↑ 3 ; follow-up: Changed 8 years ago by leif

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.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by leif

Replying to leif:

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.

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

Replying to leif:

Replying to leif:

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.

comment:7 follow-up: Changed 8 years ago by kcrisman

  • Description modified (diff)

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

comment:8 in reply to: ↑ 7 ; follow-up: Changed 8 years ago by leif

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

comment:9 in reply to: ↑ 8 ; follow-ups: Changed 8 years ago by leif

  • Dependencies set to #9906
  • Keywords R spkg R.sh.in libR.pc pkg-config hard-coded package installation R_HOME_DIR added
  • Owner changed from tbd to leif
  • Work issues set to Provide an R 2.10.1.p6 spkg.

Replying to leif:

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

  • Address / check the following (copied from 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). 
    
  • 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.)

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

Replying to leif:

  • 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:

  • Change libR.pc to use either ${pc_top_builddir} or $${SAGE_ROOT}. (In the former case, set PKG_CONFIG_TOP_BUILD_DIR to $SAGE_ROOT unless it is already, perhaps issuing a warning in case it isn't, or especially if its setting differs.)
    Also prepend $SAGE_ROOT/local/lib/pkgconfig to PKG_CONFIG_PATH, unless it is already contained. Warn if PKG_CONFIG_PATH is empty, then set it to $SAGE_ROOT/local/lib/pkgconfig.

comment:11 Changed 8 years ago by was

  • Keywords sd32 added

comment:12 Changed 7 years ago by kcrisman

  • Keywords r-project added

comment:13 Changed 6 years ago by jhpalmieri

  • Cc jhpalmieri added

comment:14 Changed 6 years ago by jhpalmieri

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.

comment:15 Changed 6 years ago by leif

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

comment:16 Changed 6 years ago by jhpalmieri

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?

comment:17 Changed 6 years ago by jhpalmieri

  • Authors set to John Palmieri
  • Description modified (diff)
  • Status changed from new to needs_review

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

comment:18 Changed 6 years ago by jhpalmieri

  • Work issues Provide an R 2.10.1.p6 spkg. deleted

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

Replying to leif:

  • Address / check the following (copied from 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?

Changed 6 years ago by jhpalmieri

patch for R spkg; for review only

comment:20 Changed 6 years ago by kcrisman

  • Dependencies #9906 deleted

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?

comment:21 Changed 6 years ago by jhpalmieri

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.

comment:22 Changed 6 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to positive_review

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.

comment:23 Changed 6 years ago by jhpalmieri

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.

comment:24 Changed 6 years ago by kcrisman

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

comment:25 follow-up: Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work

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.

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:26 in reply to: ↑ 25 ; follow-up: Changed 6 years ago by kcrisman

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?

comment:27 in reply to: ↑ 26 ; follow-up: Changed 6 years ago by jdemeyer

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)

comment:28 in reply to: ↑ 27 Changed 6 years ago by 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)

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

Last edited 6 years ago by kcrisman (previous) (diff)

Changed 6 years ago by jhpalmieri

patch for R spkg; for review only

comment:29 Changed 6 years ago by jhpalmieri

  • Status changed from needs_work to positive_review

Here is another attempt at fixing this.

comment:30 Changed 6 years ago by jhpalmieri

  • Status changed from positive_review to needs_work

comment:31 Changed 6 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

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

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

comment:32 follow-up: Changed 6 years ago by 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?

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

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

comment:33 in reply to: ↑ 32 ; follow-up: Changed 6 years ago by jhpalmieri

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?

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

Replying to jhpalmieri:

Why don't you like it?

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

comment:35 follow-up: Changed 6 years ago by jhpalmieri

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.

comment:36 in reply to: ↑ 35 Changed 6 years ago by jdemeyer

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.

Changed 6 years ago by jhpalmieri

comment:37 follow-up: Changed 6 years ago by jhpalmieri

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.

comment:38 Changed 6 years ago by jhpalmieri

  • Description modified (diff)

New spkg posted.

comment:39 in reply to: ↑ 37 ; follow-up: Changed 6 years ago by leif

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

comment:40 in reply to: ↑ 39 Changed 6 years ago by jhpalmieri

Replying to leif:

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

comment:41 Changed 6 years ago by jhpalmieri

For what it's worth, someone reported on ask.sagemath.org that this spkg fixed their problem with installing an R package.

comment:42 Changed 6 years ago by kcrisman

I think that sounds like a positive review, combined with Jeroen's good comments... What do you think?

comment:43 follow-up: Changed 6 years ago by jhpalmieri

I think that someone should confirm that with the new spkg, in the script local/bin/R, the variables R_SHARE_DIR, R_INCLUDE_DIR, and R_DOC_DIR are now defined in terms of R_HOME_DIR rather than being hard-coded paths as they are with the current spkg.

comment:44 in reply to: ↑ 43 Changed 6 years ago by kcrisman

I think that someone should confirm that with the new spkg, in the script local/bin/R, the variables R_SHARE_DIR, R_INCLUDE_DIR, and R_DOC_DIR are now defined in terms of R_HOME_DIR rather than being hard-coded paths as they are with the current spkg.

I can do this.

comment:45 Changed 6 years ago by kcrisman

  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Jeroen Demeyer
  • Status changed from needs_review to positive_review

I confirmed that moving a Sage install (not just binary) caused installing an R package to fail with precisely the problems one would expect if these were incorrectly defined (e.g.

Warning: R include directory is empty -- perhaps need to install R-devel.rpm or similar

with appropriate nonexistent directory referenced). The script local/bin/R had the (now incorrect) paths.

Then installing this spkg and retrying caused success, and local/bin/R looks right now too. Nice work!

comment:46 Changed 6 years ago by kcrisman

  • Status changed from positive_review to needs_info

One question, though... when I move Sage back and run Sage, it doesn't change R_HOME_DIR back.

R_HOME_DIR=/Users/.../Downloads/tempR/sage-5.9.beta5/local/lib/R

when it should be

R_HOME_DIR=/Users/.../Downloads/sage-5.9.beta5/local/lib/R

So this was changed, presumably, when I reinstalled the spkg. It doesn't impact installing new R packages, by the way, nor functionality of R.

In particular, moving a different Sage installation and starting Sage changes some things, but doesn't change the location of R_HOME_DIR in local/bin/R. I'm not sure why that doesn't affect functionality, but presumably this should somehow be taken care of. On this ticket?

comment:47 Changed 6 years ago by jhpalmieri

It took me a little while to understand this, too. Right before the lines defining R_SHARE_DIR, etc., there are lines

if test x$SAGE_BUILDING_R = x; then
    R_HOME_DIR="$SAGE_LOCAL/lib/R/"
fi

If you're not building the R spkg, then this will be executed, overriding the hard-coded path earlier in the script, and setting R_HOME_DIR to the desired portable setting. I don't know R, so I don't know how to test this: if you run sage -R, can you execute some R command to tell you the current setting of R_HOME_DIR?

comment:48 Changed 6 years ago by kcrisman

  • Status changed from needs_info to needs_review

Well, that is a Sage-specific thing, I think, but we have

R_HOME="${R_HOME_DIR}"

later on and also

$ ./sage -R RHOME
/Users/.../Downloads/sage-5.9.beta5/local/lib/R/

So all is well, I think.

comment:49 Changed 6 years ago by kcrisman

  • Status changed from needs_review to positive_review

comment:50 Changed 6 years ago by jdemeyer

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