Opened 9 years ago

Closed 8 years ago

#11503 closed task (fixed)

Make new spkg to install Jmol in SAGE_LOCAL/share

Reported by: gutow Owned by: gutow
Priority: major Milestone: sage-5.2
Component: notebook Keywords: sd31 Jmol flask
Cc: rado, fbissey, strogdon Merged in: sage-5.2.beta0
Authors: Jonathan Gutow Reviewers: Dan Drake, Jason Grout, Punarbasu Purkayastha
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11080, #11078, #11874 Stopgaps:

Description (last modified by gutow)

In an effort to remove Jmol from the notebook in the Flask notebook it is being moved to SAGE_LOCAL/share. This is based on the logic that the notebook can run without Jmol, but Sage cannot run without either the notebook or Jmol even when using just the command line.

To achieve this relocation of Jmol:

  1. install the spkg http://www.uwosh.edu/faculty_staff/gutow/jmol-12.2.21.p0.spkg
  1. Apply trac-11503-jmol-spkg.2.patch to the sage root repository.
  1. Apply trac-11503-jmol-commandline.patch to the sage repository (fixes jmol on the command line)

The necessary patch for sagenb has already been merged with the master sagenb branch: https://github.com/sagemath/sagenb/pull/28

Attachments (7)

trac_11503_path_fix.patch (781 bytes) - added by gutow 9 years ago.
path fix for flask
trac_11503_cmd_line_fix.patch (995 bytes) - added by gutow 9 years ago.
plot3d command line fix
trac11503.patch (3.9 KB) - added by ddrake 9 years ago.
changes to spkg-install, SPKG.txt. For review only.
trac-11503-jmol-commandline.patch (1.3 KB) - added by jason 9 years ago.
apply to sage repository
trac_11503-jmol-commandline.rebase.patch (1.2 KB) - added by kini 9 years ago.
apply to $SAGE_ROOT/devel/sage
trac-11503-jmol-spkg.patch (1.2 KB) - added by jdemeyer 9 years ago.
apply to the sage root repository
trac-11503-jmol-spkg.2.patch (1.3 KB) - added by ppurka 8 years ago.
Rebased to sage-5.0beta11

Download all attachments as: .zip

Change History (73)

comment:1 Changed 9 years ago by gutow

  • Owner changed from jason, mpatel, was to gutow

Changed 9 years ago by gutow

path fix for flask

comment:2 Changed 9 years ago by gutow

  • Description modified (diff)

Changed 9 years ago by gutow

plot3d command line fix

comment:3 Changed 9 years ago by gutow

  • Description modified (diff)

comment:4 Changed 9 years ago by fbissey

  • Cc fbissey added

comment:5 Changed 9 years ago by kcrisman

  • Description modified (diff)

comment:6 Changed 9 years ago by gutow

  • Description modified (diff)

spkg refactored to better meet Sage standards and remove dead code from spkg-install.

comment:7 Changed 9 years ago by gutow

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

comment:8 follow-up: Changed 9 years ago by jhpalmieri

  • Status changed from needs_review to needs_work

Several comments:

  • There are some backup files present: SPKG.txt~ and spkg-install~.
  • As I noted on #11496, you should copy the jmol shell script to SAGE_LOCAL/bin as part of spkg-install.
  • It would be helpful if you could prepare the mercurial repository with the version of hg installed with Sage. When I use the version included with Sage (which is the only version on my computer), I get
    $ hg status
    abort: requirement 'dotencode' not supported!
    
    so I can't check the state of the repo without installing a new version of hg.

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

  • Hmm...didn't notice the backup files.  I'll look into that...
  • I could add the Jmol script to the jmol .spkg.  The one that is in there is the generic install not the one for SAGE...It seems reasonable that it should not be tracked by SAGE...I will add the SAGE version as a patch to the .spkg.  Once I get this done, I believe #11496 should be closed, as it will no longer apply...let me know who to notify about that.
  • Not sure if I can manage to successfully backtrack on the Hg repository format.  In order to work with the flask-notebook on my machine, I found I needed a system install of Hg and I got the latest.  Is this going to be a hold up?  Hg has been a real pain compared to the other version control systems I've  used.  Can somebody point out how to do the backtrack cleanly and quickly.  If it takes me a couple of hours to untangle Hg that's more time than it will take me to fix the rest of the .spkg problems and more time than I have for this project at the moment.

Replying to jhpalmieri:

Several comments: - There are some backup files present: SPKG.txt~ and spkg-install~. - As I noted on #11496, you should copy the jmol shell script to SAGE_LOCAL/bin as part of spkg-install. - It would be helpful if you could prepare the mercurial repository with the version of hg installed with Sage. When I use the version included with Sage (which is the only version on my computer), I get $ hg status abort: requirement 'dotencode' not supported! so I can't check the state of the repo without installing a new version of hg.

comment:10 Changed 9 years ago by gutow

  • Description modified (diff)

comment:11 Changed 9 years ago by gutow

  • Status changed from needs_work to needs_review

.p2 addresses the following: * reverted to mercurial version included in Sage (lost all the history in the process).

  • jmol launch script is now included in the patches and copied to SAGE_LOCAL/bin.
  • I think I removed all the backup files.

comment:12 follow-up: Changed 9 years ago by jhpalmieri

  • Status changed from needs_review to needs_work

I upgraded my version of mercurial so I can look at the repo, and I hope that the version included with Sage will be upgraded soon, too (see #10594). Oh, and now I see that I can open this repo with the old version. Anyway, there are some uncommitted files:

$ hg status
? patches/Jmol.js.patch
? patches/jmol.patch

(So you should run "hg add" and then commit again.)

The file spkg-install needs some work. In particular, any variable which refers to a path should be quoted, in case there are spaces in it. (Right now, we don't support having spaces in the path where Sage is, but when it's easy to allow, we should.) I think this should do it:

  • spkg-install

    diff --git a/spkg-install b/spkg-install
    a b if [ $? -ne 0 ]; then 
    2727fi
    2828
    2929# Check for sagenb location
    30 cd $SAGE_ROOT"/devel/sagenb/sagenb/data/jmol"
     30cd "$SAGE_ROOT/devel/sagenb/sagenb/data/jmol"
    3131if [ $? -ne 0 ]; then
    3232    echo "No old Jmol install in notebook. Skipping removal of Jmol from notebook."
    3333else
    3434    echo "Removing Jmol files from the notebook data directory..."
    35     rm -r $SAGE_ROOT"/devel/sagenb/sagenb/data/jmol"
     35    rm -r "$SAGE_ROOT/devel/sagenb/sagenb/data/jmol"
    3636fi
    3737
    3838TEMPDIR=$SPKDIR"/src"
    if [ $? -ne 0 ]; then 
    4747   echo "Directory "$SAGE_LOCAL"/share/jmol does not exist.  Creating Directory..."
    4848else
    4949    echo "Deleting all files from "$SAGE_LOCAL"/share/jmol..."
    50     rm -r $SAGE_LOCAL"/share/jmol"
     50    rm -r "$SAGE_LOCAL/share/jmol"
    5151    echo "replacing jmol directory and contents..."
    5252fi
    5353
    54 mkdir $SAGE_LOCAL"/share/jmol"
     54mkdir "$SAGE_LOCAL/share/jmol"
    5555
    5656TEMPDIR=$SPKDIR"/src/jmol"
    5757cd "$TEMPDIR"
    58 cp -r * $SAGE_LOCAL"/share/jmol/"
     58cp -r * "$SAGE_LOCAL/share/jmol/"
    5959
    6060cd $SAGE_LOCAL"/bin"
    6161if [ $? -ne 0 ]; then
    if [ $? -ne 0 ]; then 
    6464else
    6565    echo "Copying jmol script to "$SAGE_LOCAL"/bin."
    6666    cd "$TEMPDIR"
    67     cp -f jmol $SAGE_LOCAL"/bin"
     67    cp -f jmol "$SAGE_LOCAL/bin"
    6868fi
    6969
    7070echo "Installing applet web directory"
    71 mkdir $SAGE_LOCAL"/share/jmol/appletweb"
    72 cp Jmol.js $SAGE_LOCAL"/share/jmol/appletweb"
     71mkdir "$SAGE_LOCAL/share/jmol/appletweb"
     72cp Jmol.js "$SAGE_LOCAL/share/jmol/appletweb"
    7373
    7474TEMPDIR=$SPKDIR"/patches/appletweb"
    7575cd "$TEMPDIR"
    if [ $? -ne 0 ]; then 
    7777   echo "Error finding patches/appletweb directory. Exiting."
    7878   exit 1
    7979fi
    80 cp -r * $SAGE_LOCAL"/share/jmol/appletweb"
     80cp -r * "$SAGE_LOCAL/share/jmol/appletweb"
    8181
    8282if [ $? -ne 0 ]; then
    8383   echo "Error installing PACKAGE_NAME."

comment:13 in reply to: ↑ 12 Changed 9 years ago by gutow

  • Status changed from needs_work to needs_review

The below is done.  Try downloading the spkg again after 3 PM CDT.

Replying to jhpalmieri:

I upgraded my version of mercurial so I can look at the repo, and I hope that the version included with Sage will be upgraded soon, too (see #10594). Oh, and now I see that I can open this repo with the old version. Anyway, there are some uncommitted files: $ hg status ? patches/Jmol.js.patch ? patches/jmol.patch (So you should run "hg add" and then commit again.) The file spkg-install needs some work. In particular, any variable which refers to a path should be quoted, in case there are spaces in it. (Right now, we don't support having spaces in the path where Sage is, but when it's easy to allow, we should.) I think this should do it: #!diff diff --git a/spkg-install b/spkg-install --- a/spkg-install +++ b/spkg-install @@ -27,12 +27,12 @@ if [ $? -ne 0 ]; then fi # Check for sagenb location -cd $SAGE_ROOT"/devel/sagenb/sagenb/data/jmol" +cd "$SAGE_ROOT/devel/sagenb/sagenb/data/jmol" if [ $? -ne 0 ]; then echo "No old Jmol install in notebook. Skipping removal of Jmol from notebook." else echo "Removing Jmol files from the notebook data directory..." - rm -r $SAGE_ROOT"/devel/sagenb/sagenb/data/jmol" + rm -r "$SAGE_ROOT/devel/sagenb/sagenb/data/jmol" fi TEMPDIR=$SPKDIR"/src" @@ -47,15 +47,15 @@ if [ $? -ne 0 ]; then echo "Directory "$SAGE_LOCAL"/share/jmol does not exist. Creating Directory..." else echo "Deleting all files from "$SAGE_LOCAL"/share/jmol..." - rm -r $SAGE_LOCAL"/share/jmol" + rm -r "$SAGE_LOCAL/share/jmol" echo "replacing jmol directory and contents..." fi -mkdir $SAGE_LOCAL"/share/jmol" +mkdir "$SAGE_LOCAL/share/jmol" TEMPDIR=$SPKDIR"/src/jmol" cd "$TEMPDIR" -cp -r * $SAGE_LOCAL"/share/jmol/" +cp -r * "$SAGE_LOCAL/share/jmol/" cd $SAGE_LOCAL"/bin" if [ $? -ne 0 ]; then @@ -64,12 +64,12 @@ if [ $? -ne 0 ]; then else echo "Copying jmol script to "$SAGE_LOCAL"/bin." cd "$TEMPDIR" - cp -f jmol $SAGE_LOCAL"/bin" + cp -f jmol "$SAGE_LOCAL/bin" fi echo "Installing applet web directory" -mkdir $SAGE_LOCAL"/share/jmol/appletweb" -cp Jmol.js $SAGE_LOCAL"/share/jmol/appletweb" +mkdir "$SAGE_LOCAL/share/jmol/appletweb" +cp Jmol.js "$SAGE_LOCAL/share/jmol/appletweb" TEMPDIR=$SPKDIR"/patches/appletweb" cd "$TEMPDIR" @@ -77,7 +77,7 @@ if [ $? -ne 0 ]; then echo "Error finding patches/appletweb directory. Exiting." exit 1 fi -cp -r * $SAGE_LOCAL"/share/jmol/appletweb" +cp -r * "$SAGE_LOCAL/share/jmol/appletweb" if [ $? -ne 0 ]; then echo "Error installing PACKAGE_NAME."

comment:14 Changed 9 years ago by gutow

  • Description modified (diff)
  • Milestone changed from sage-4.8 to sage-5.0

Removing command line patch as it appears to be incorporated into 4.8.

comment:15 Changed 9 years ago by ddrake

  • Status changed from needs_review to needs_work

I'm looking this over, and I have some more minor nits to pick with spkg-install. Here's an idiom I see a couple times:

TEMPDIR=something
cd "$TEMPDIR"
do some stuff
...

Two things bother me about that: (1) you call it TEMPDIR, which implies to me that it's a temporary directory used for some scratch work, but you're talking about an existing directory with important files in it. (2) You never use the TEMPDIR variable, except in the cd command...so why not just cd to the directory?

Also, I think it's more readable to just put the paths into commands, rather than cding around. I would prefer to see

cp $SAGE_FOO/dir/file.txt $SAGE_BAR/dir/dir2/

rather than

cd $SAGE_FOO/dir
cp file.txt $SAGE_BAR/dir/dir2/

It makes it easier to read the script, since you don't have to remember what the working directory is. Also, you sometimes change directories inside an if-then-else, so the working directory depends on what happened there!

Finally, at the end of spkg-install, you have "PACKAGE NAME" instead of Jmol!

Changed 9 years ago by ddrake

changes to spkg-install, SPKG.txt. For review only.

comment:16 follow-up: Changed 9 years ago by ddrake

  • Authors changed from gutow to Jonathan Gutow
  • Status changed from needs_work to needs_review

I've looked over the spkg, and modulo my fixes to spkg-install, I can give it a positive review. My changes are in attachment:trac11503.patch and I have a spkg with those changes at http://sagenb.kaist.ac.kr/~drake/Jmol-12.0.45.p3.spkg. It's identical to Jonathan's version except for the changes in attachment:trac11503.patch.

Jason, you volunteered to look it over, but that shouldn't stop anyone else. :)

comment:17 in reply to: ↑ 16 Changed 9 years ago by gutow

  • Description modified (diff)

Dan's changes look good to me (obviously much more experience writing these scripts than I have:)) I suggest we try Dan's .spkg for the review and have changed the description to reflect this.

Jonathan

Replying to ddrake:

I've looked over the spkg, and modulo my fixes to spkg-install, I can give it a positive review. My changes are in attachment:trac11503.patch and I have a spkg with those changes at http://sagenb.kaist.ac.kr/~drake/Jmol-12.0.45.p3.spkg. It's identical to Jonathan's version except for the changes in attachment:trac11503.patch. Jason, you volunteered to look it over, but that shouldn't stop anyone else. :)

comment:18 Changed 9 years ago by jason

Dan: your changes look fine to me. Do you give everything else a positive review?

comment:19 Changed 9 years ago by jason

  • Status changed from needs_review to positive_review

Oh, wait, I see that you just said modulo your fixes, you give the spkg a positive review. So it sounds like we agree: positive review.

comment:20 Changed 9 years ago by jason

  • Reviewers set to Dan Drake, Jason Grout

comment:21 Changed 9 years ago by jason

  • Description modified (diff)

comment:22 Changed 9 years ago by jason

  • Status changed from positive_review to needs_work

hmm...nothing has been added to the deps file. There should be another patch for that.

comment:23 Changed 9 years ago by jason

  • Status changed from needs_work to needs_review

So I think what needs to be done at this point is:

  1. Make the jmol spkg not start with a capital letter (simple rename of the spkg file)
  1. Apply the trac-11503-jmol-spkg.patch to the sage root repository, make an sdist, put the jmol spkg in there, and try building. In essence, review the changes to the sage root repository.

comment:24 Changed 9 years ago by jason

  • Description modified (diff)

comment:25 Changed 9 years ago by jason

  • Dependencies changed from flask notebook to flask notebook, #11874

comment:26 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  • Work issues set to SPKG.txt

comment:27 follow-up: Changed 9 years ago by jdemeyer

Okay, it's not in the documentation, but you should format the Changelog with lines like

=== mypackage-3.18.spkg (Jeroen Demeyer, 17 January 2012) ===
 * Changed foo to bar

This is how every spkg does it (except for sagenb, which I can live with).

comment:28 in reply to: ↑ 27 Changed 9 years ago by gutow

Should I change this or will you Dan?

A comment.  As a relatively newer developer, I do not appreciate formatting requirements that are not documented.  I followed the example in the developer's guide.

Jonathan

Replying to jdemeyer:

Okay, it's not in the documentation, but you should format the Changelog with lines like === mypackage-3.18.spkg (Jeroen Demeyer, 17 January 2012) === * Changed foo to bar This is how every spkg does it (except for sagenb, which I can live with).

comment:29 Changed 9 years ago by jason

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

I made an spkg that reformats the changelog and makes the spkg lowercase. See the new instructions.

Jonathan: I understand---I didn't object before because it wasn't in the docs.

comment:30 follow-up: Changed 9 years ago by jdemeyer

Yeah, I can imagine it's annoying. It should be documented better. I always thought it was documented, for the simple reason that every spkg is doing it the same way.

The docs should probably be clarified.

comment:31 Changed 9 years ago by jdemeyer

  • Description modified (diff)

I made some further improvements to SPKG.txt, in particular I simplified the Dependencies to

 * Sage Notebook (sagenb)

comment:32 Changed 9 years ago by jason

I've updated the instructions over at #11080 to reflect your new spkg.

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

Replying to jdemeyer:

Yeah, I can imagine it's annoying. It should be documented better.

See http://trac.sagemath.org/sage_trac/attachment/ticket/9419/trac_9419-part2.patch.

comment:34 in reply to: ↑ 33 Changed 9 years ago by kcrisman

Yeah, I can imagine it's annoying. It should be documented better.

See http://trac.sagemath.org/sage_trac/attachment/ticket/9419/trac_9419-part2.patch.

I knew this sounded familiar!

Changed 9 years ago by jason

apply to sage repository

comment:35 Changed 9 years ago by jason

  • Description modified (diff)

Attached patch which fixes command-line jmol. This issue was discovered over on #9238; I used the fix from the comments there.

comment:36 Changed 9 years ago by jason

To review this ticket, this is what is left:

comment:37 Changed 9 years ago by jdemeyer

  • Work issues SPKG.txt deleted

comment:38 Changed 9 years ago by jdemeyer

Since jmol doesn't use setuptools, it should be listed normally between the dependencies. My new trac-11503-jmol-spkg.patch does this.

comment:39 Changed 9 years ago by kini

  • Keywords changed from sd31,Jmol, flask to sd31 Jmol flask

Jason, what did you base your commandline patch on? It doesn't apply to 4.8.rc0, even with fuzz, since there seem to be some different lines in the context of the second chunk. Here's a rebase on 4.8.rc0, though I don't know what's going on here so I won't put it in the ticket description.

Changed 9 years ago by kini

apply to $SAGE_ROOT/devel/sage

comment:40 Changed 9 years ago by jason

  • Dependencies changed from flask notebook, #11874 to flask notebook, #11078, #11874

I made the patch on 5.0something (prealpha/beta1 maybe?). I also had #11078 applied before this.

comment:41 Changed 9 years ago by kini

Oh, OK. Then there is no rebasing needed -- 4.8.rc0 -> #11078 -> your patch works perfectly.

comment:42 Changed 9 years ago by jason

  • Status changed from needs_review to needs_work

I tried building a clean sage with this spkg and the new notebook, and command-line jmol didn't work because the local/bin/jmol script was not executable. So it should probably be set to executable inside the spkg.

comment:43 Changed 9 years ago by jason

  • Work issues set to jmol script chmod'd to executable

comment:44 Changed 9 years ago by jason

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

I just uploaded a p4 version that sets the jmol script to executable after copying to local/bin. So it's needs review again.

comment:45 Changed 9 years ago by strogdon

  • Cc strogdon added

Changed 9 years ago by jdemeyer

apply to the sage root repository

Changed 8 years ago by ppurka

Rebased to sage-5.0beta11

comment:46 Changed 8 years ago by ppurka

The attachment trac-11503-jmol-spkg.2.patch has been rebased to sage-5.0beta11. Compiling it now (along with #11874 and the patches to SAGE_ROOT from #11080).

comment:47 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:48 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.0 to sage-5.1

comment:49 Changed 8 years ago by gutow

  • Description modified (diff)

Updated to Jmol-12.2.21...this will allow using Jmol to generate images server-side.

comment:50 follow-up: Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  • Work issues jmol script chmod'd to executable deleted
$ hg status
? SPKG.txt~

comment:51 Changed 8 years ago by kini

Since it's a new jmol version, the p-version number should be reset.

comment:52 in reply to: ↑ 50 Changed 8 years ago by gutow

Odd that the chmod is gone...I didn't edit the script....All I did was replace the .jar files....I'll check this afternoon.  Not sure why the SPKG backup got left behind, but I can delete it....

Replying to jdemeyer:

$ hg status ? SPKG.txt~

comment:53 Changed 8 years ago by kini

  • Dependencies changed from flask notebook, #11078, #11874 to #11080, #11078, #11874

comment:54 Changed 8 years ago by gutow

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

SPKG.txt~ removed and renamed to ...p0

comment:55 Changed 8 years ago by gutow

I believe this ticket is actually included in the positively reviewed #11080.  If there are no objections, I believe this should become a positive review as well.

comment:56 Changed 8 years ago by ppurka

  • Status changed from needs_review to positive_review

Indeed this should be positive review. I have been using it both from the notebook and from the command line without issues, for several months.

Setting it to positive review.

comment:57 Changed 8 years ago by kcrisman

  • Reviewers changed from Dan Drake, Jason Grout to Dan Drake, Jason Grout, Punarbasu Purkayastha

comment:58 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-5.2

comment:59 Changed 8 years ago by kini

Please put the latest version of the SPKG up on this ticket. It looks like #12299's instructions mention a newer version of the SPKG than is found here.

comment:60 Changed 8 years ago by kcrisman

Jonathan can correct me if I'm wrong, but as far as I know, this is on purpose, because the newest Jmol at #12299 needs the many updates to Jmol there. In that sense this ticket is a 'hard' prereq for #11080 and hence for #13121/#12299.

comment:61 Changed 8 years ago by kini

OK, fair enough.

comment:62 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.2.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:63 Changed 8 years ago by jdemeyer

  • Merged in sage-5.2.beta0 deleted
  • Milestone changed from sage-5.2 to sage-pending
  • Resolution fixed deleted
  • Status changed from closed to new

Unmerging this from sage-5.2 due to the serious security issue at #13270.

comment:64 Changed 8 years ago by jdemeyer

  • Status changed from new to needs_review

comment:65 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:66 Changed 8 years ago by jdemeyer

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