Opened 9 years ago

Closed 7 years ago

Last modified 7 years ago

#9906 closed defect (fixed)

Move the RPy package outside of the R package

Reported by: mpatel Owned by: leif
Priority: major Milestone: sage-5.4
Component: packages: standard Keywords: r-project spkg
Cc: leif, kcrisman, vbraun, kini Merged in: sage-5.4.beta1
Authors: Leif Leonhardy, Jeroen Demeyer Reviewers: Karl-Dieter Crisman, John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13428, #13395 Stopgaps:

Description (last modified by jdemeyer)

We include the RPy spkg in the R spkg and install the former from the latter. It would be less potentially confusing to move RPy outside of R but make it depend on R and any other prerequisites in spkg/standard/deps.

The necessary changes to spkg/install and spkg/standard/deps are already there, but currently commented out. Apply the following patch to the Sage root repository to activate them:

trac_9906-Activate_separate_RPy_spkg.root-repo.patch


spkg:

  1. http://boxen.math.washington.edu/home/jdemeyer/spkg/r-2.14.0.p5.spkg (diff: r-2.14.0.p5.diff)
  2. http://boxen.math.washington.edu/home/jdemeyer/spkg/rpy2-2.0.8.p0.spkg (diff: rpy2-2.0.8.p0.diff)

Related: #3086 (closed as duplicate). See also #9668 and #9847 for what could or should be addressed, too. Previous R upgrade: #12057.

Attachments (3)

trac_9906-Activate_separate_RPy_spkg.root-repo.patch (2.3 KB) - added by jdemeyer 7 years ago.
Apply to SAGE_ROOT. Activates RPY in spkg/install and spkg/standard/deps
r-2.14.0.p5.diff (3.2 KB) - added by jdemeyer 7 years ago.
Diff for the R spkg. For reference / review only.
rpy2-2.0.8.p0.diff (9.3 KB) - added by jdemeyer 7 years ago.
Diff for the rpy2 spkg (w.r.t. to the one in the current R spkg). For reference / review only.

Download all attachments as: .zip

Change History (55)

comment:1 Changed 9 years ago by leif

  • Cc leif added

It would also allow independent updates and potentially improve compression.

comment:2 Changed 9 years ago by leif

When touching the R package, one should also consider #9847 (i.e. R environment variables potentially disturbing the build).

comment:3 Changed 9 years ago by leif

  • Priority changed from minor to critical
  • Type changed from enhancement to defect

See this comment for another important reason.

comment:4 Changed 9 years ago by leif

(Having Rpy in R's spkg is the culprit of this build error.)

comment:5 follow-up: Changed 9 years ago by leif

  • Owner changed from tbd to leif

I've created a separate RPy 2.0.8.p1 spkg and an R 2.10.1.p5 (follow-up of p4 at #10016) spkg with the RPy spkg removed; both coming soon... (including updated deps and spkg/install)

I also cleaned up both packages a little, and tried to address #9847, too.

comment:6 in reply to: ↑ 5 Changed 9 years ago by drkirkby

Replying to leif:

I've created a separate RPy 2.0.8.p1 spkg and an R 2.10.1.p5 (follow-up of p4 at #10016) spkg with the RPy spkg removed; both coming soon...

How soon is soon?

If you can make these available, it will help those that want to update the source code.

Dave

comment:7 Changed 9 years ago by leif

Personal note:

  • Add patching libR.pc to spkg-install, making rhome and rincludedir relative to $SAGE_ROOT. (Currently no prefix=... definition.)

comment:8 Changed 8 years ago by leif

  • Description modified (diff)

comment:9 Changed 8 years ago by kcrisman

  • Cc kcrisman added
  • Priority changed from critical to major

comment:10 Changed 8 years ago by leif

  • Description modified (diff)

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

So... does this activity mean you will be posting something on e.g. spkg-upload?

:)

comment:12 Changed 8 years ago by leif

  • Authors set to Leif Leonhardy
  • Description modified (diff)
  • Status changed from new to needs_review

comment:13 in reply to: ↑ 11 Changed 8 years ago by leif

Replying to kcrisman:

So... does this activity mean you will be posting something on e.g. spkg-upload?

Looks like. Before the spkgs have their first anniversary...

There are (still) some things that could be done, e.g. using patch and patching the pkg-config file (libR.pc) from spkg-install.

We could also perhaps either recompress the contained recommended R packages (.tar.gz) with bzip2 or decompress them to plain tar files, as we compress the whole spkg with bzip2 anyway. Haven't yet tried that though.

RPy (and perhaps R) should IMHO be upgraded on a follow-up ticket.

comment:14 Changed 8 years ago by leif

I see the hardcoding issues shouldn't be hard to fix; the current (more or less superfluous) patch to R.sh.in and the corresponding Python script do only half of the job, namely setting R_HOME_DIR to $SAGE_LOCAL/lib/R/, but not changing R_SHARE_DIR, R_INCLUDE_DIR and R_DOC_DIR.

But before I do so, I think the new spkgs should first be tested as they are.

I'll then make a p6, either here or (preferably) on a follow-up ticket if no other issues arise with the p5.

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

  • Reviewers set to Karl-Dieter Crisman

Yes, a different ticket would be fine. I didn't realize these spkgs were actually ready :)

Comments/questions:

  • The rpy2 patch looks fine, and those are nice additions (the sanity checks, I mean).
  • Really excellent work clarifying things on the r patch, adding all those checks etc. I wouldn't have thought of any of them, but they all make loads of sense, though probably won't affect a million users. I'm probably the one responsible for the .DS* stuff - I always try to check, but sometimes one forgets.
  • Do we need to unset R_PROFILE completely, or can we save that in a temp variable, unset it, then reset it after the spkg installs? (If not because it would mess up RPy, we would have to do that there too.)
  • Do we need an 'echo' regarding the comment about the $MAKE check so that if it does fail, people know what to do?
  • What happened to Dave's favorite "x$SAGE64" = xyes (you removed the 'x's)?
  • Any reason to include the old RPy installation in the if false block? Presumably if one really wanted to, the HG is there - I can't imagine this being very interesting to anyone.

Note I have not actually installed or tested this yet :)

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

Sorry for the delay, no trac notifications... >B-|

Replying to kcrisman:

Yes, a different ticket would be fine.

#9668 if you don't object...


I didn't realize these spkgs were actually ready :)

I didn't know either. ;-)

More or less just completed the Changelog entries.


Comments/questions:

  • Do we need to unset R_PROFILE completely, or can we save that in a temp variable, unset it, then reset it after the spkg installs? (If not because it would mess up RPy, we would have to do that there too.)

It's not fully clear to me what you mean by that. Unsetting R_PROFILE there only lasts until we leave spkg-install, i.e., is only effective within spkg-install and everything that's called from it. (Same applies to spkg-check.)

Actually I apparently didn't think of unsetting (or forgot to unset) R_PROFILE also in RPy's spkg-install; I'm not sure if RPy could break otherwise, but doing so shouldn't hurt.

IIRC there were also problems with R_PROFILE when using Sage's R, which would have to be addressed in e.g. sage-env and/or the Sage library, not [necessarily] the spkg itself.


  • Do we need an 'echo' regarding the comment about the $MAKE check so that if it does fail, people know what to do?

Well, I was expecting testing this being part of the review process, so I just added a comment. I can of course also print an appropriate message in case the (parallel) test should fail, but perhaps in the p6 on another ticket. (Automatically rerunning the test suite sequentially would IMHO be a less good idea.)


  • What happened to Dave's favorite "x$SAGE64" = xyes (you removed the 'x's)?

I get eye cancer from such, it's worse readable and hence error-prone. If test "" = foo would evaluate to true (on some supported systems) Sage would certainly break in many places, not just because we use that all around. Quoting is important.

(The broken instance of test was btw. a shell built-in version [besides some really very old test implementations which were less robust w.r.t. invalid expressions], and we explicitly use bash anyway. Prepending characters to variable expansions can make sense in different contexts, and is sometimes used to save the quotes, though the latter is also quite ugly. Similar applies to not using -o and -a.)

If one wanted to go really safe, one would either explicitly use bash's built-in version of test ([[ "$SAGE64" = yes ]]), use case "$SAGE64" in yes) ..., or perhaps

[ "${SAGE64:-no}" = yes ]

which is the equivalent of Python's

os.environ.get("SAGE64", "no") == "yes"


  • Any reason to include the old RPy installation in the if false block?

Not really.

Presumably if one really wanted to, the HG is there - I can't imagine this being very interesting to anyone.

Sure, though it is buried by lots of other changes in the same changeset. I intended to remove it later, temporarily kept it just for documentation purposes.


Note I have not actually installed or tested this yet :)

Hope you'll do so later. :)

comment:17 in reply to: ↑ 16 ; follow-up: Changed 8 years ago by kcrisman

Yes, a different ticket would be fine.

#9668 if you don't object...

No, that is quite natural.

  • Do we need to unset R_PROFILE completely, or can we save that in a temp variable, unset it, then reset it after the spkg installs? (If not because it would mess up RPy, we would have to do that there too.)

It's not fully clear to me what you mean by that. Unsetting R_PROFILE there only lasts until we leave spkg-install, i.e., is only effective within spkg-install and everything that's called from it. (Same applies to spkg-check.)

I didn't realize that.

Actually I apparently didn't think of unsetting (or forgot to unset) R_PROFILE also in RPy's spkg-install; I'm not sure if RPy could break otherwise, but doing so shouldn't hurt.

So maybe that's a "needs work"?

IIRC there were also problems with R_PROFILE when using Sage's R, which would have to be addressed in e.g. sage-env and/or the Sage library, not [necessarily] the spkg itself.

Yes, that's a different ticket, and notoriously hard to track down at times.

  • Do we need an 'echo' regarding the comment about the $MAKE check so that if it does fail, people know what to do?

Well, I was expecting testing this being part of the review process, so I just added a comment. I can of course also print an appropriate message in case the (parallel) test should fail, but perhaps in the p6 on another ticket. (Automatically rerunning the test suite sequentially would IMHO be a less good idea.)

Ok.

  • Any reason to include the old RPy installation in the if false block?

Not really.

Presumably if one really wanted to, the HG is there - I can't imagine this being very interesting to anyone.

Sure, though it is buried by lots of other changes in the same changeset. I intended to remove it later, temporarily kept it just for documentation purposes.

Well, either way that wouldn't hold up this ticket.

Note I have not actually installed or tested this yet :)

Hope you'll do so later. :)

But not today :(

comment:18 in reply to: ↑ 17 Changed 8 years ago by leif

Replying to kcrisman:

Yes, a different ticket would be fine.

#9668 if you don't object...

No, that is quite natural.

Ok.


Actually I apparently didn't think of unsetting (or forgot to unset) R_PROFILE also in RPy's spkg-install; I'm not sure if RPy could break otherwise, but doing so shouldn't hurt.

So maybe that's a "needs work"?

R_PROFILE doesn't appear in any of RPy's source files, so I don't think so. (I perhaps checked that last year, but forgot about it. :D )

comment:19 Changed 8 years ago by kcrisman

Just a note to self/any other reviewer of what would have to be tested.

  • Upgrading rpy2 by itself.
  • Upgrading r by itself.
  • Then upgrading rpy2 after having upgraded r.
  • And test R and rpy2 after each step.
  • Test with and without R_PROFILE set.
  • Test with and without SAGE_CHECK set.
  • Test with SAGE_CHECK yes with and without parallel make.
  • Test on (at least) Linux and Mac.
  • Test with and without spaces in the path.

Remind me why this is needed again? :-)

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

Here is another somewhat related ticket - #10967. I don't think that is fixed, at least not in the same way, by this p5.

comment:21 Changed 8 years ago by leif

  • Description modified (diff)

comment:22 in reply to: ↑ 20 Changed 8 years ago by leif

  • Description modified (diff)

Replying to kcrisman:

Here is another somewhat related ticket - #10967. I don't think that is fixed, at least not in the same way, by this p5.

For the record: #10967 will be addressed at #9668, by an R 2.10.1.p6 spkg there, so I've made the former ticket depend on the latter, and changed its milestone to "duplicate/invalid/won't fix".

comment:23 Changed 8 years ago by leif

Ping.

I think we should merge this before it further rottens; I'll not be able to finish #9668 in the next days (and right now don't know when I'll do that), but it's not a dependency anyway, just a further improvement...

comment:24 Changed 8 years ago by kcrisman

Sorry, I just won't have time to do that laundry list of testing in the near future. (As you may have noticed from my relative lack of activity elsewhere.) And there isn't really anything breaking because of this weirdness, thankfully... My apologies :(

comment:25 follow-ups: Changed 8 years ago by jason

Wow. I just upgraded my rpy2 spkg to 2.2.4, and was tempted to work on upgrading R to 2.14. But then I saw this ticket. I don't have time either right now to do the extensive testing this ticket appears to need (according to comments above), and it feels like this work should go in before upgrading to 2.14, etc.

comment:26 Changed 8 years ago by kcrisman

  • Keywords r-project added

comment:27 in reply to: ↑ 25 Changed 8 years ago by kcrisman

  • Cc vbraun added

Replying to jason:

Wow. I just upgraded my rpy2 spkg to 2.2.4, and was tempted to work on upgrading R to 2.14. But then I saw this ticket. I don't have time either right now to do the extensive testing this ticket appears to need (according to comments above), and it feels like this work should go in before upgrading to 2.14, etc.

Regarding this, see #12057.

comment:28 in reply to: ↑ 25 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  • Work issues set to Rebase the spkg to #12057

Replying to jason:

Wow. I just upgraded my rpy2 spkg to 2.2.4, and was tempted to work on upgrading R to 2.14. But then I saw this ticket. I don't have time either right now to do the extensive testing this ticket appears to need (according to comments above), and it feels like this work should go in before upgrading to 2.14, etc.

Well, this didn't happen. R will be upgraded to version 2.14 in #12057 which will normally be merged in sage-4.8.alpha4. So, this ticket needs to be rebased...

comment:29 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:30 in reply to: ↑ 25 ; follow-up: Changed 8 years ago by jdemeyer

Replying to jason:

I don't have time either right now to do the extensive testing this ticket appears to need (according to comments above)

Testing is easy, I wouldn't mind doing that.

comment:31 in reply to: ↑ 30 Changed 8 years ago by kcrisman

I don't have time either right now to do the extensive testing this ticket appears to need (according to comments above)

Testing is easy, I wouldn't mind doing that.

I had the same problem, because it would require a lot of manual stuff, but if you can use the buildbot to test a rebased version of this that would be fantastic.

comment:32 Changed 8 years ago by kcrisman

Apparently we also should take care of some references to sage-env or something which are soon to disappear - see #11073, in particular this comment.

comment:33 Changed 8 years ago by leif

  • Work issues changed from Rebase the spkg to #12057 to Rebase the spkg to #12172

Argh!

comment:34 Changed 8 years ago by jdemeyer

  • Dependencies set to #12172, #10492

comment:35 Changed 8 years ago by jdemeyer

  • Dependencies changed from #12172, #10492 to #12172, #10492, #12787

comment:36 Changed 7 years ago by kini

  • Cc kini added

comment:37 Changed 7 years ago by jdemeyer

  • Dependencies changed from #12172, #10492, #12787 to #13428
  • Description modified (diff)
  • Work issues Rebase the spkg to #12172 deleted

comment:38 Changed 7 years ago by jdemeyer

  • Dependencies changed from #13428 to #13428, #13395

comment:39 Changed 7 years ago by jdemeyer

  • Description modified (diff)

Changed 7 years ago by jdemeyer

Apply to SAGE_ROOT. Activates RPY in spkg/install and spkg/standard/deps

comment:40 Changed 7 years ago by jdemeyer

Why should R depend on PYTHON? The old spkg/standard/deps claims "Note that even with a separate RPy spkg (#9906), Sage's R will still depend on "Python, but I see no reason for this. So I removed the explicit PYTHON dependency. But it still indirectly depends on PYTHON via PYTHON->ATLAS->R.

comment:41 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Keywords spkg added
  • Milestone changed from sage-5.3 to sage-5.4

Changed 7 years ago by jdemeyer

Diff for the R spkg. For reference / review only.

comment:42 Changed 7 years ago by jdemeyer

  • Authors changed from Leif Leonhardy to Leif Leonhardy, Jeroen Demeyer
  • Status changed from needs_work to needs_review

comment:43 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:44 follow-up: Changed 7 years ago by jhpalmieri

There is also some general cleanup that could happen; for example:

  • spkg-install

    diff --git a/spkg-install b/spkg-install
    a b if [ "$UNAME" = "SunOS" ]; then 
    6767    SUN_FLAGS="--without-ICU"; export SUN_FLAGS
    6868fi
    6969
    70 # let's remove old install, even the wrongly installed ones
    71 rm -rf "$SAGE_LOCAL"/lib/r
    72 rm -rf "$SAGE_LOCAL"/lib/R
    73 rm -rf "$SAGE_LOCAL"/lib64/R
    74 rm -rf "$SAGE_LOCAL"/lib64/r
    75 # and let's also nuke some leftovers on SAGE_LOCAL/lib
    76 rm -rf "$SAGE_LOCAL"/lib/libRblas.so "$SAGE_LOCAL"/lib/libRlapack.so "$SAGE_LOCAL"/lib/libR.so
    77 rm -rf "$SAGE_LOCAL"/lib/libRblas.dylib "$SAGE_LOCAL"/lib/libRlapack.dylib "$SAGE_LOCAL"/lib/libR.dylib
    78 
    7970cd src
    8071
    8172# Apply patches.  See SPKG.txt for information about what each patch
    if [ $? -ne 0 ]; then 
    131122    exit 1
    132123fi
    133124
     125# Before installing, remove old install, even the wrongly installed ones.
     126rm -rf "$SAGE_LOCAL"/lib/r
     127rm -rf "$SAGE_LOCAL"/lib/R
     128rm -rf "$SAGE_LOCAL"/lib64/R
     129rm -rf "$SAGE_LOCAL"/lib64/r
     130# and let's also nuke some leftovers on SAGE_LOCAL/lib
     131rm -rf "$SAGE_LOCAL"/lib/libRblas.so "$SAGE_LOCAL"/lib/libRlapack.so "$SAGE_LOCAL"/lib/libR.so
     132rm -rf "$SAGE_LOCAL"/lib/libRblas.dylib "$SAGE_LOCAL"/lib/libRlapack.dylib "$SAGE_LOCAL"/lib/libR.dylib
     133
    134134# Disable parallel make install, which is supposedly broken
    135135$MAKE -j1 vignettes  # Needed for help system
    136136$MAKE -j1 install
    if [ $? -ne 0 ] || [ ! -f "$SAGE_ROOT"/s 
    168168fi
    169169
    170170if [ "$UNAME" = "Darwin" ]; then
    171     echo "Copying fake java and javac compiler on OSX"
     171    echo "Removing fake java and javac compiler on OSX."
    172172    rm -f "$SAGE_LOCAL"/bin/java
    173173    rm -f "$SAGE_LOCAL"/bin/javac
    174174fi

Should this happen on another ticket?

comment:45 in reply to: ↑ 44 ; follow-up: Changed 7 years ago by jdemeyer

Replying to jhpalmieri:

Should this happen on another ticket?

Yes please. I specifically didn't do any kind of cleanup just to ensure this would finally be merged.

comment:46 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

rpy2 needs a version bump, otherwise it won't be downloaded when upgrading.

comment:47 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Changed 7 years ago by jdemeyer

Diff for the rpy2 spkg (w.r.t. to the one in the current R spkg). For reference / review only.

comment:48 Changed 7 years ago by jhpalmieri

  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, John Palmieri
  • Status changed from needs_review to positive_review

Looks good to me. Builds and passes tests, and works when upgrading.

comment:49 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.4.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:50 in reply to: ↑ 45 ; follow-up: Changed 7 years ago by kcrisman

Should this happen on another ticket?

Yes please. I specifically didn't do any kind of cleanup just to ensure this would finally be merged.

You can cc: me on this new ticket too when you make it.

comment:51 in reply to: ↑ 50 Changed 7 years ago by jdemeyer

Replying to kcrisman:

You can cc: me on this new ticket too when you make it.

To be honest, this is not something I feel like working on, so I'm going to make that ticket.

comment:52 Changed 7 years ago by jdemeyer

Okay, I'm making the follow-up anyway: #13443.

Note: See TracTickets for help on using tickets.