The jmol spkg contains testjava.sh

[SnarkBoojum]

This script would be better in the sage-scripts spkg, as it's sage-specific, probably as sage-testjava.sh or some such. The change will break one doctest in devel/sage/sage/interfaces/jmoldata.py, where it is looking for it using the full path.

CC: @gutow

Component: packages: standard

Author: Steven Trogdon, Jeroen Demeyer

Branch: c7c227e

Reviewer: Julien Puydt

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

[gutow]

comment:2

I would argue the move is only appropriate if any other package in Sage depends on java. To my knowledge, nothing in the base install other than Jmol does.

[SnarkBoojum]

comment:3

You can argue like that if you want, but jmol doesn't use testjava.sh -- devel/sage/sage/interfaces/jmoldata.py does. So I argue that only sage depends on it, and I stand by my report! :-P

[gutow]

comment:4

Replying to @SnarkBoojum:

You can argue like that if you want, but jmol doesn't use testjava.sh -- devel/sage/sage/interfaces/jmoldata.py does. So I argue that only sage depends on it, and I stand by my report! :-P

You have a good point. Since Sage depends on Jmol, this will always be available if Jmol needs it...I'll put this on my list as I try to incorporate the pure javascript version of Jmol for the notebook.

[SnarkBoojum]

comment:5

What's the status of this?

[gutow]

comment:6

Replying to @SnarkBoojum:

What's the status of this?

I'm swamped...I am hoping to spend more time on this starting next month, but I have equipment (spectrometers, vacuum lines, computers, etc..to get fixed for next Fall's classes), and some web site programming for use at my University to finish before I can get to this. Sorry, for the delay. I definitely have not forgot about this.
JG

[strogdon]

comment:8

Is there any interest in moving the functionality of testjava.sh into jmoldata.py? I'm not sure what is easiest to maintain, but I believe the following will work.

--- a/sage/interfaces/jmoldata.py
+++ b/sage/interfaces/jmoldata.py
@@ -86,13 +86,23 @@
             sage_makedirs(jmolscratch)
         scratchout = os.path.join(jmolscratch,"jmolout.txt")
         jout=open(scratchout,'w')
-        testjavapath = os.path.join(SAGE_LOCAL, "share", "jmol", "testjava.sh")
-        result = subprocess.call([testjavapath],stdout=jout)
-        jout.close()
-        if (result == 0):
-            return (True)
-        else:
-            return (False)
+        try:
+            version = subprocess.check_output(['java', '-version'], stderr=subprocess.STDOUT)
+            import re, types
+            java_version = re.search("version.*[1]\.[567]", version)
+            if type(java_version) is types.NoneType:
+                jout.write('Your version of Java is either deprecated or not supported\nWe support versions 1.5 to 1.7\n')
+                jout.close()
+                result = bool(java_version)
+            else:
+                jout.write('You have a supported version of Java, Exiting\n')
+                jout.close()
+                result = bool(java_version)
+        except (subprocess.CalledProcessError, OSError):
+            jout.write('You do not have Java installed\nWe support versions 1.5 to 1.7\n')
+            jout.close()
+            result = False
+        return result

     def export_image(self,
         targetfile,
@@ -183,7 +193,7 @@
             if not os.path.exists(jmolscratch):
                 sage_makedirs(jmolscratch)
             scratchout = os.path.join(jmolscratch,"jmolout.txt")
-            jout=open(scratchout,'w')
+            jout=open(scratchout,'a')
             #now call the java application and write the file.
             result = subprocess.call(["java","-Xmx512m","-Djava.awt.headless=true","-jar",jmolpath,"-iox","-g",sizeStr,"-J",launchscript,"-j",imagescript],stdout=jout)
             jout.close()

[jhpalmieri]

comment:9

This idea was also discussed earlier.

[strogdon]

comment:10

Replying to @jhpalmieri:

This idea was also discussed earlier .

And the ideas are really yours.

[jhpalmieri]

comment:11

I think a lot of people had ideas about this, in fact.

[gutow]

comment:12

I wrote the original code and think this is a good idea. I did try to make it work that way originally, but ran into some problems, I believe with escaping of quotations. I will try to test this soon and encourage others to as well. If we can give it a positive review it is one less file to keep up-to-date.

[jdemeyer]

comment:13

strongdon: I'm making an actual patch based on your suggestion. I don't see the need to write to jmolout.txt though...

[jdemeyer]

Dependencies: #14358

[jdemeyer]

Author: Steven Trogdon, Jeroen Demeyer

[jdemeyer]

Attachment: 14364_testjava.patch.gz

[jdemeyer]

Changed dependencies from #14358 to none

[strogdon]

Commit: b85230b

[strogdon]

comment:16

Attempting to push the 14364_testjava.patch​ to the Git Server. Let's hope this works!

---

New commits:

b85230b

No longer use testjava.sh script

[strogdon]

Branch: public/ticket/14364-testjava

[jdemeyer]

Changed branch from public/ticket/14364-testjava to u/jdemeyer/ticket/14364

[jdemeyer]

Changed commit from b85230b to c7c227e

[jdemeyer]

New commits:

c7c227e

Remove testjava.sh from jmol package

[nexttime]

comment:22

[ping]

Is there anything to test beyond (re)installing the spkg and testing src/sage/interfaces/jmoldata.py?

If not, works for me...

[SnarkBoojum]

comment:23

Ok, I was finally able to test, and it works.

I think reinstalling the spkg won't do the trick for a review, because you'll still have testjava.sh from the previous installation (sage-as-a-distribution doesn't have uninstallation if I don't err). And you'll also need to recompile the sagelib since one of the files has been modified.

[SnarkBoojum]

Reviewer: Snark

[gutow]

comment:24

Hmm...maybe I need to include this in the updated jmol.spkg I'm building? I expect to have some time to devote to that over the next few days. I'll try plugging this into the new code.

Replying to @SnarkBoojum:

Ok, I was finally able to test, and it works.

I think reinstalling the spkg won't do the trick for a review, because you'll still have testjava.sh from the previous installation (sage-as-a-distribution doesn't have uninstallation if I don't err). And you'll also need to recompile the sagelib since one of the files has been modified.

[SnarkBoojum]

comment:25

The fix for this ticket is two patches:

• one to detect java directly in sagelib ;
• the other to remove testjava.sh from the build/pkgs/jmol/ data.

If they go in, doesn't it update things (in particular the spkg) correctly and automatically?

[gutow]

comment:26

I believe so, but if I do not fix the updated .spkg it may reinstall testjava.sh, although it would not get used. I will make sure my update is compatible. I too think this ticket is fine. I just have to make my end work well with it.

Replying to @SnarkBoojum:

If they go in, doesn't it update things (in particular the spkg) correctly and automatically?

[vbraun]

Changed branch from u/jdemeyer/ticket/14364 to c7c227e

[kcrisman]

Changed reviewer from Snark to Julien Puydt

[kcrisman]

Changed commit from c7c227e to none