Opened 11 years ago
Last modified 10 years ago
#11503 closed task
Make new spkg to install Jmol in SAGE_LOCAL/share — at Version 21
Reported by:  gutow  Owned by:  gutow 

Priority:  major  Milestone:  sage5.2 
Component:  notebook  Keywords:  sd31 Jmol flask 
Cc:  rado, fbissey, strogdon  Merged in:  
Authors:  Jonathan Gutow  Reviewers:  Dan Drake, Jason Grout 
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  flask notebook  Stopgaps: 
Description (last modified by )
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:
 install this .spkg
./sage f "http://sagenb.kaist.ac.kr/~drake/Jmol12.0.45.p3.spkg"
(which lives at this location)
The necessary patch for sagenb has already been merged with the master sagenb branch: https://github.com/sagemath/sagenb/pull/28
Change History (24)
comment:1 Changed 11 years ago by
 Owner changed from jason, mpatel, was to gutow
Changed 11 years ago by
comment:2 Changed 11 years ago by
 Description modified (diff)
comment:3 Changed 11 years ago by
 Description modified (diff)
comment:4 Changed 11 years ago by
 Cc fbissey added
comment:5 Changed 11 years ago by
 Description modified (diff)
comment:6 Changed 11 years ago by
 Description modified (diff)
spkg refactored to better meet Sage standards and remove dead code from spkginstall.
comment:7 Changed 11 years ago by
 Description modified (diff)
 Status changed from new to needs_review
comment:8 followup: ↓ 9 Changed 11 years ago by
 Status changed from needs_review to needs_work
Several comments:
 There are some backup files present: SPKG.txt~ and spkginstall~.
 As I noted on #11496, you should copy the jmol shell script to SAGE_LOCAL/bin as part of spkginstall.
 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 11 years ago by
 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 flasknotebook 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 spkginstall~.  As I noted on #11496, you should copy the jmol shell script to SAGE_LOCAL/bin as part of spkginstall.  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 11 years ago by
 Description modified (diff)
comment:11 Changed 11 years ago by
 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 followup: ↓ 13 Changed 11 years ago by
 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 spkginstall 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:

spkginstall
diff git a/spkginstall b/spkginstall
a b if [ $? ne 0 ]; then 27 27 fi 28 28 29 29 # Check for sagenb location 30 cd $SAGE_ROOT"/devel/sagenb/sagenb/data/jmol"30 cd "$SAGE_ROOT/devel/sagenb/sagenb/data/jmol" 31 31 if [ $? ne 0 ]; then 32 32 echo "No old Jmol install in notebook. Skipping removal of Jmol from notebook." 33 33 else 34 34 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" 36 36 fi 37 37 38 38 TEMPDIR=$SPKDIR"/src" … … if [ $? ne 0 ]; then 47 47 echo "Directory "$SAGE_LOCAL"/share/jmol does not exist. Creating Directory..." 48 48 else 49 49 echo "Deleting all files from "$SAGE_LOCAL"/share/jmol..." 50 rm r $SAGE_LOCAL"/share/jmol"50 rm r "$SAGE_LOCAL/share/jmol" 51 51 echo "replacing jmol directory and contents..." 52 52 fi 53 53 54 mkdir $SAGE_LOCAL"/share/jmol"54 mkdir "$SAGE_LOCAL/share/jmol" 55 55 56 56 TEMPDIR=$SPKDIR"/src/jmol" 57 57 cd "$TEMPDIR" 58 cp r * $SAGE_LOCAL"/share/jmol/"58 cp r * "$SAGE_LOCAL/share/jmol/" 59 59 60 60 cd $SAGE_LOCAL"/bin" 61 61 if [ $? ne 0 ]; then … … if [ $? ne 0 ]; then 64 64 else 65 65 echo "Copying jmol script to "$SAGE_LOCAL"/bin." 66 66 cd "$TEMPDIR" 67 cp f jmol $SAGE_LOCAL"/bin"67 cp f jmol "$SAGE_LOCAL/bin" 68 68 fi 69 69 70 70 echo "Installing applet web directory" 71 mkdir $SAGE_LOCAL"/share/jmol/appletweb"72 cp Jmol.js $SAGE_LOCAL"/share/jmol/appletweb"71 mkdir "$SAGE_LOCAL/share/jmol/appletweb" 72 cp Jmol.js "$SAGE_LOCAL/share/jmol/appletweb" 73 73 74 74 TEMPDIR=$SPKDIR"/patches/appletweb" 75 75 cd "$TEMPDIR" … … if [ $? ne 0 ]; then 77 77 echo "Error finding patches/appletweb directory. Exiting." 78 78 exit 1 79 79 fi 80 cp r * $SAGE_LOCAL"/share/jmol/appletweb"80 cp r * "$SAGE_LOCAL/share/jmol/appletweb" 81 81 82 82 if [ $? ne 0 ]; then 83 83 echo "Error installing PACKAGE_NAME."
comment:13 in reply to: ↑ 12 Changed 11 years ago by
 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 spkginstall 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/spkginstall b/spkginstall  a/spkginstall +++ b/spkginstall @@ 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 11 years ago by
 Description modified (diff)
 Milestone changed from sage4.8 to sage5.0
Removing command line patch as it appears to be incorporated into 4.8.
comment:15 Changed 11 years ago by
 Status changed from needs_review to needs_work
I'm looking this over, and I have some more minor nits to pick with spkginstall. 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 cd
ing 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 ifthenelse, so the working directory depends on what happened there!
Finally, at the end of spkginstall
, you have "PACKAGE NAME" instead of Jmol!
comment:16 followup: ↓ 17 Changed 11 years ago by
 Status changed from needs_work to needs_review
I've looked over the spkg, and modulo my fixes to spkginstall
, 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/Jmol12.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 11 years ago by
 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
spkginstall
, 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/Jmol12.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 11 years ago by
Dan: your changes look fine to me. Do you give everything else a positive review?
comment:19 Changed 11 years ago by
 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 11 years ago by
 Reviewers set to Dan Drake, Jason Grout
comment:21 Changed 11 years ago by
 Description modified (diff)
path fix for flask