sagemath / sage

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

R breaks if SAGE_LOCAL undefined #10967

Closed ff2dc20c-251e-477e-a72c-eab37f55928e closed 12 years ago

ff2dc20c-251e-477e-a72c-eab37f55928e commented 13 years ago

horatio@havelock ~ $ R /Users/horatio/bin/R: line 212: /lib/R//etc/ldpaths: No such file or directory

This is because on line 31 of the script someone rewrote $R_HOME_DIR in terms of $SAGE_LOCAL. When SAGE_LOCAL is missing from the environment of the running shell, the R script breaks. The fix is to test for empty SAGE_LOCAL before the rewrite.

This is from Sage version 4.6.2.

Depends on #9668

CC: @jasongrout @williamstein @nexttime

Component: packages: standard

Keywords: R script SAGE_LOCAL R.sh.in spkg local/bin r-project

Reviewer: Jeroen Demeyer

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

ff2dc20c-251e-477e-a72c-eab37f55928e commented 13 years ago

patch to fix

ff2dc20c-251e-477e-a72c-eab37f55928e commented 13 years ago
comment:1

Attachment: trac_10967_R_sage_local_fix.patch.gz

ff2dc20c-251e-477e-a72c-eab37f55928e commented 13 years ago

Description changed:

--- 
+++ 
@@ -3,3 +3,5 @@

 This is because on line 31 of the script someone rewrote $R_HOME_DIR in terms of $SAGE_LOCAL. When SAGE_LOCAL is missing from the environment of the running shell, the R script breaks. The fix is to test for empty SAGE_LOCAL before the rewrite.

+This is from Sage version 4.6.2.
+
7c09a680-e216-4024-bb8e-9bfd4aa7f313 commented 13 years ago
comment:2

Is this ready for review?

ff2dc20c-251e-477e-a72c-eab37f55928e commented 13 years ago
comment:3

Replying to @sagetrac-mvngu:

Is this ready for review?

The fix is. It's not a mercurial-generated patch and there don't seem to be any developer guidelines for shell scripts.

7c09a680-e216-4024-bb8e-9bfd4aa7f313 commented 13 years ago
comment:4

Which file in the Sage distribution are you patching? Provide the exact path to the file from SAGE_ROOT.

ff2dc20c-251e-477e-a72c-eab37f55928e commented 13 years ago
comment:5

$SAGE_ROOT/local/bin/R

7c09a680-e216-4024-bb8e-9bfd4aa7f313 commented 13 years ago
comment:6

Replying to @sagetrac-ahd:

$SAGE_ROOT/local/bin/R

That's a wrong file to patch in order to get any changes into the R spkg. The file SAGE_ROOT/local/bin/R is part of the R spkg, so you should patch the R spkg. Get the latest Sage development release, unpack it, and look under SAGE_ROOT/spkg/standard/ for the R spkg. For more information on patching an spkg, see this page in the Sage Developer's Guide.

kcrisman commented 13 years ago
comment:7

Does this actually make the problem worse? The way I read the patch, before one did check for SAGE_LOCAL and afterwards one doesn't. Is this patch backwards?

I can't do it right now, but I should be able to produce an updated R package within a few days if ahd isn't able to.

Changing component, since the problem isn't the statistics functionality but rather the way the package is bundled.

ff2dc20c-251e-477e-a72c-eab37f55928e commented 13 years ago
comment:8

Replying to @kcrisman:

Does this actually make the problem worse? The way I read the patch, before one did check for SAGE_LOCAL and afterwards one doesn't. Is this patch backwards?

I can't do it right now, but I should be able to produce an updated R package within a few days if ahd isn't able to.

Changing component, since the problem isn't the statistics functionality but rather the way the package is bundled.

Probably backwards from your point of view; since I was only lodging this issue as a courtesy, I just did a diff -u between my version of the R script (which doesn't break when SAGE_LOCAL is undefined) and the version that is in my sage installation (which does).

So the best fix from my point of view is to define SAGE_LOCAL in my .profile and leave you all to do what you want, yes?

kcrisman commented 13 years ago
comment:9

Replying to @sagetrac-ahd:

Replying to @kcrisman:

Does this actually make the problem worse? The way I read the patch, before one did check for SAGE_LOCAL and afterwards one doesn't. Is this patch backwards?

I can't do it right now, but I should be able to produce an updated R package within a few days if ahd isn't able to.

Probably backwards from your point of view; since I was only lodging this issue as a courtesy, I just did a diff -u between my version of the R script (which doesn't break when SAGE_LOCAL is undefined) and the version that is in my sage installation (which does).

Okay, that explains it. I didn't have a copy of the R script on hand.

So the best fix from my point of view is to define SAGE_LOCAL in my .profile and leave you all to do what you want, yes?

Is there a reason why sage -R isn't as good of an option? But yes, for now that would be a workaround for you. But you are right that we should fix this; no reason for the R to be unusable outside of Sage.

cc:ing the "package maintainers" as well. R is at 2.12, nearly 2.13, in the meantime, by the way. Some changes might affect build process - now needs 'popen' and a C99 compiler - if we upgrade. Probably those are standard, but I don't know if they are or not.

kcrisman commented 13 years ago
comment:10

Okay, it turns out that this is done in spkg-install in an interesting way - by calling a Python script called fix_hardcode:

#!/usr/bin/env python

import os

def fix_hardcode(file):
    r = open(file).read().replace('#SAGEHACK#','R_HOME_DIR="${SAGE_LOCAL}/lib/R/"')
    open(file,'w').write(r)

S = os.environ['SAGE_LOCAL']
fix_hardcode(S + '/bin/R')
fix_hardcode(S + '/lib/R/bin/R')

Which replaces the thing which is actually patched:

# HACK for Sage to avoid hardcoding.  NOthing 
# else has been changed in this file.
#R_HOME_DIR="${SAGE_LOCAL}/lib/R/"  
#SAGEHACK#  

Apparently one can't change this part of the file R.sh.in until after configuration and make install? I think because that file must end up in the binary ahd alludes to.

So we just need to change the hack to

    r = open(file).read().replace('#SAGEHACK#','foo')

where foo is a string version of

if test -n "${SAGE_LOCAL}"; then 
   R_HOME_DIR="${SAGE_LOCAL}/lib/R/"   
fi 

But I don't know exactly how one puts multiple lines properly in a shell script of this kind. I guess it's a Python script, so maybe \n would suffice for that? But of course R.sh.in is not a Python script.

jhpalmieri commented 13 years ago
comment:11

You can use triple quotes in python for multiline strings, so something like this (which is untested) might work:

def fix_hardcode(file):
    r = open(file).read().replace('#SAGEHACK#',
                                  """if test -n "${SAGE_LOCAL}"; then 
   R_HOME_DIR="${SAGE_LOCAL}/lib/R/"   
fi""")
    open(file,'w').write(r)
jhpalmieri commented 13 years ago
comment:12

I don't know anything about R, but this looks more complicated to me. When I look at R.sh.in, right after the #SAGEHACK# part, I see

R_HOME="${R_HOME_DIR}"
export R_HOME
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

When I install the spkg, every instance of R_HOME_DIR gets replaced with the hard-coded full path to SAGE_LOCAL/lib/R, rather than being kept as R_HOME_DIR. Doesn't this defeat the purpose #SAGEHACK#? Won't things go wrong if you move the whole Sage installation?

Anyway, I see the same error "/lib/R//etc/ldpaths: No such file or directory" if I run R. If I apply the patch above, it works on one machine (an Intel Mac running OS X 10.6), but on another (also a mac, same OS) I get an error

dyld: Library not loaded: /usr/local/lib/libgfortran.2.dylib
  Referenced from: /Applications/sage/local/lib/R/lib/libR.dylib
  Reason: image not found
Trace/BPT trap

Indeed, libgfortran is not in /usr/local/lib, I think it's only in /Applications/sage/local/lib.

I'll post the patch anyway, in case it helps. New spkg at http://sage.math.washington.edu/home/palmieri/SPKG/r-2.10.1.p5.spkg. If this works, please mark the ticket as "needs review" and add the URL to the ticket description, so the release manager can find it.

jhpalmieri commented 13 years ago

hg patch for R spkg

kcrisman commented 13 years ago
comment:13

Attachment: trac_10967-R.patch.gz

Ok, I'll look into this eventually. I don't really understand when all this hardcoding happens, because I don't know what R.sh.in does in the build process. My hazy understanding of it would indicate you are right, though. But I guess testing is the key.

kcrisman commented 13 years ago
comment:14

Conceivably related, from a sage-support thread where someone had trouble moving the Mac version. Just for reference.

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). 
kcrisman commented 13 years ago
comment:15

This may be related to #9968 and friends.

kcrisman commented 13 years ago
comment:16

This should also probably have a nonzero exit code if $SAGE_LOCAL is undefined.

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

IIRC I completely dropped the fix_hardcode script, but I'm not sure about whether I'm using SAGE_LOCAL or SAGE_ROOT; using the latter wouldn't be much better though. ;-)

I'll take a look at it, although I think I wasn't expecting someone to run Sage's R from "outside"...

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

Dependencies: #9668

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

Replying to @nexttime:

... although I think I wasn't expecting someone to run Sage's R from "outside"...

Thinking a bit more about this: ;-)

In general, none of the "stand-alone" programs like R, the PARI/GP interpreter, Maxima etc. as shipped with and installed by Sage (and almost none of the scripts) in $SAGE_ROOT/local/bin/ are intended to be called directly from outside the Sage environment, hence there's also no point in putting this directory into one's PATH (i.e., one shouldn't).

Instead, one can do any of the following:

$ ./sage -c "install_scripts('$HOME/bin')"

or

$ sudo ./sage -c "install_scripts('/usr/local/bin')" # needs root privileges

Afterwards, provided that both an instance of $SAGE_ROOT/sage is found along and the directory passed to install_scripts() is contained in PATH, one can start (e.g.) Sage's R again by simply typing R from any shell, i.e., also from "outside the Sage environment".

Note: install_scripts() currently has a flaw in that the created shortcuts do not support spaces in arguments passed to them; this has been fixed at #11602, which has been merged into Sage 4.7.2.alpha2.

Nonetheless, I agree that we should at least add a sanity check to e.g. $SAGE_ROOT/local/bin/R, such that it exits with an appropriate error message in case SAGE_LOCAL isn't defined (like many scripts already do). I guess such a test is currently missing just because nobody thought of someone trying to call this (and some other scripts) directly, since having them in the PATH without a properly set up environment was never intended.

I can (and will perhaps) also make some scripts automatically execute themselves in a Sage environment, since this (to me) seems convenient for e.g. devel/sage/spkg-dist, but most probably not scripts like R in $SAGE_ROOT/local/bin/, because of the alternatives already present as given above.


I'll address the specific issue with the R script(s) at #9668 (not #9968 ;-) ), so I've added that ticket as a dependency.

After I've fixed this there (by simply adding a sanity check), this ticket should IMHO be closed as duplicate (not to say "invalid/won't fix").

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

Changed keywords from R to R script SAGE_LOCAL R.sh.in spkg local/bin

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

Work Issues: Review as soon as #9668 is ready (new R spkg).

kcrisman commented 13 years ago

Changed keywords from R script SAGE_LOCAL R.sh.in spkg local/bin to R script SAGE_LOCAL R.sh.in spkg local/bin r-rproject

kcrisman commented 13 years ago
comment:19

See also #12057.

kcrisman commented 13 years ago

Changed keywords from R script SAGE_LOCAL R.sh.in spkg local/bin r-rproject to R script SAGE_LOCAL R.sh.in spkg local/bin r-project

jdemeyer commented 12 years ago

Changed work issues from Review as soon as #9668 is ready (new R spkg). to none

jdemeyer commented 12 years ago

Reviewer: Jeroen Demeyer