Opened 11 years ago

Closed 11 years ago

#10016 closed defect (fixed)

R's spkg-install calls "sage -f ..." to install the contained Rpy spkg

Reported by: leif Owned by: leif
Priority: critical Milestone: sage-4.6
Component: packages: standard Keywords: sage-spkg deps scripts dependency
Cc: mpatel Merged in: sage-4.6.alpha2
Authors: Leif Leonhardy Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by leif)

This already caused trouble when the Sage scripts spkg wasn't yet installed, and making all standard packages depend on the complete set of Sage scripts (which had actually then been done) is an overkill.

There's a lot wrong with this spkg, and the Rpy spkg should be moved out of R's (#9906), but as a short-term solution, I decided to just replace the call to $SAGE_ROOT/sage (which requires sage-sage) by a direct one to $SAGE_ROOT/local/bin/sage-spkg, which should be present in any case, s.t. we don't have to make the R spkg depend on the Sage scripts spkg.

The new p4 spkg does not address any other issues.


r-2.10.1.p4 (Leif Leonhardy, September 25th 2010)

  • #10016: Don't call "sage -f" on the included Rpy spkg, instead call "sage-spkg -f" directly, since only this is guaranteed to be present. This is just a temporary solution, until the Rpy spkg is removed from this one.

New spkg: http://spkg-upload.googlecode.com/files/r-2.10.1.p4.spkg

md5sum: afb2987172a2d740227bd24227e6546a

Attachments (1)

trac_10016-r-2.10.1.p3-p4.spkg.patch (3.2 KB) - added by leif 11 years ago.
Diff between the p3 and p4 spkg (actually a Mercurial patch). For reference/review.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 11 years ago by leif

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

Changed 11 years ago by leif

Diff between the p3 and p4 spkg (actually a Mercurial patch). For reference/review.

comment:2 Changed 11 years ago by kcrisman

# sage -f "$RPY_VER".spkg # EVIL

I think you have different standards of evil than I do :)

But this sounds like a good intermediate step here. What I understand of the change in the script makes sense. I don't know how tee works, and whether pipestatus really is in the path or what it does, so I sadly can't review this. Maybe you should cc: the maintainers? (Which I think are jason and was.)

comment:3 follow-up: Changed 11 years ago by leif

I just noticed this doesn't fully work with spaces in the path, but since spaces aren't supported yet anyway (and most probably won't be soon), and this is just a temporary solution (cf. #9906), I'm not going to change the fairly large spkg again here...

comment:4 in reply to: ↑ 3 Changed 11 years ago by leif

Replying to leif:

[...] I'm not going to change the fairly large spkg again here...

... unless somebody finds other things that have to be fixed here.

comment:5 follow-up: Changed 11 years ago by jhpalmieri

So because of the tee command, is the install log for rpy going to appear in both r-2.10.1.p4.log and rpy2-2.0.8.log?

I suppose I should try it and see. Should I use the "deps" file from #9896?

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

Just to experiment, I took deps from #9896, took out the dependency of R on sage-scripts, and put in a dependency of sage-scripts on R.

comment:7 in reply to: ↑ 5 Changed 11 years ago by leif

Trac...

Replying to jhpalmieri:

So because of the tee command, is the install log for rpy going to appear in both r-2.10.1.p4.log and rpy2-2.0.8.log?

I will appear in both. Note that previously the ./sage -f ... directly wrote (appended) to $SAGE_ROOT/install.log as well, which I consider also a bug.

I suppose I should try it and see. Should I use the "deps" file from #9896?

This makes no difference, since R still depends on the scripts in both. (But you could remove the dependency of R on sage_scripts in the deps.v2b at #9896.)

comment:8 Changed 11 years ago by leif

LOL, s/I will/It will/.

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

Replying to jhpalmieri:

Just to experiment, I took deps from #9896, took out the dependency of R on sage-scripts, and put in a dependency of sage-scripts on R.

Good idea. But you should (at least) also remove local/bin/sage-sage to make such a test effective. In principle, you could remove everything from local/bin except sage-spkg and sage-env IIRC, but you'd have to install the new R spkg by copying it to spkg/ and then running make (or just make build).

Or rebuild Sage from scratch with the new R spkg and such deps, as I did...

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

Replying to leif:

Replying to jhpalmieri:

Just to experiment, I took deps from #9896, took out the dependency of R on sage-scripts, and put in a dependency of sage-scripts on R.

Good idea. But you should (at least) also remove local/bin/sage-sage to make such a test effective. In principle, you could remove everything from local/bin except sage-spkg and sage-env IIRC, but you'd have to install the new R spkg by copying it to spkg/ and then running make (or just make build).

And of course remove spkg/installed/sage_scripts-* before running make.

Or rebuild Sage from scratch with the new R spkg and such deps, as I did...

comment:11 Changed 11 years ago by leif

Argh, "copying it to spkg/" should read "copying it to spkg/standard/".

comment:12 Changed 11 years ago by jhpalmieri

I wasn't clear: I'm building from scratch with the modified deps and the new R spkg, so I didn't need to remove anything. (I had a few false starts, but "make distclean" fixed those.)

comment:13 follow-up: Changed 11 years ago by jhpalmieri

  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

This looks good to me. With my purely experimental version of deps, it emphasizes the problem on #9434: since sage-scripts is installed very late, there are many warning messages about being unable to find sage-banner.

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

Thanks for reviewing this.

Replying to jhpalmieri:

[...] With my purely experimental version of deps, it emphasizes the problem on #9434: since sage-scripts is installed very late, there are many warning messages about being unable to find sage-banner.

:D

comment:15 Changed 11 years ago by mpatel

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