Opened 19 months ago

Closed 5 months ago

Last modified 3 months ago

#14364 closed enhancement (fixed)

The jmol spkg contains testjava.sh

Reported by: Snark Owned by: tbd
Priority: minor Milestone: sage-6.3
Component: packages: standard Keywords:
Cc: gutow Merged in:
Authors: Steven Trogdon, Jeroen Demeyer Reviewers: Julien Puydt
Report Upstream: N/A Work issues:
Branch: c7c227e (Commits) Commit:
Dependencies: Stopgaps:

Description

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.

Attachments (1)

14364_testjava.patch (1.6 KB) - added by jdemeyer 11 months ago.

Download all attachments as: .zip

Change History (29)

comment:1 Changed 19 months ago by kcrisman

  • Cc gutow added

comment:2 Changed 19 months ago by gutow

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.

comment:3 follow-up: Changed 19 months ago by Snark

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

comment:4 in reply to: ↑ 3 Changed 19 months ago by gutow

Replying to Snark:

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.

comment:5 follow-up: Changed 17 months ago by Snark

What's the status of this?

comment:6 in reply to: ↑ 5 Changed 17 months ago by gutow

Replying to Snark:

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

comment:7 Changed 15 months ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:8 Changed 13 months ago by strogdon

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.

  • sage/interfaces/jmoldata.py

    a b  
    8686            sage_makedirs(jmolscratch) 
    8787        scratchout = os.path.join(jmolscratch,"jmolout.txt") 
    8888        jout=open(scratchout,'w') 
    89         testjavapath = os.path.join(SAGE_LOCAL, "share", "jmol", "testjava.sh") 
    90         result = subprocess.call([testjavapath],stdout=jout) 
    91         jout.close() 
    92         if (result == 0): 
    93             return (True) 
    94         else: 
    95             return (False) 
     89        try: 
     90            version = subprocess.check_output(['java', '-version'], stderr=subprocess.STDOUT) 
     91            import re, types 
     92            java_version = re.search("version.*[1]\.[567]", version) 
     93            if type(java_version) is types.NoneType: 
     94                jout.write('Your version of Java is either deprecated or not supported\nWe support versions 1.5 to 1.7\n') 
     95                jout.close() 
     96                result = bool(java_version) 
     97            else: 
     98                jout.write('You have a supported version of Java, Exiting\n') 
     99                jout.close() 
     100                result = bool(java_version) 
     101        except (subprocess.CalledProcessError, OSError): 
     102            jout.write('You do not have Java installed\nWe support versions 1.5 to 1.7\n') 
     103            jout.close() 
     104            result = False 
     105        return result 
    96106 
    97107    def export_image(self, 
    98108        targetfile, 
     
    183193            if not os.path.exists(jmolscratch): 
    184194                sage_makedirs(jmolscratch) 
    185195            scratchout = os.path.join(jmolscratch,"jmolout.txt") 
    186             jout=open(scratchout,'w') 
     196            jout=open(scratchout,'a') 
    187197            #now call the java application and write the file. 
    188198            result = subprocess.call(["java","-Xmx512m","-Djava.awt.headless=true","-jar",jmolpath,"-iox","-g",sizeStr,"-J",launchscript,"-j",imagescript],stdout=jout) 
    189199            jout.close() 

comment:9 follow-up: Changed 13 months ago by jhpalmieri

This idea was also discussed earlier.

comment:10 in reply to: ↑ 9 Changed 13 months ago by strogdon

Replying to jhpalmieri:

This idea was also discussed earlier .

And the ideas are really yours.

comment:11 Changed 13 months ago by jhpalmieri

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

comment:12 Changed 13 months ago by gutow

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.

comment:13 Changed 11 months ago by jdemeyer

  • Dependencies set to #14358

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

comment:14 Changed 11 months ago by jdemeyer

  • Authors set to Steven Trogdon, Jeroen Demeyer

Changed 11 months ago by jdemeyer

comment:15 Changed 11 months ago by jdemeyer

  • Dependencies #14358 deleted

comment:16 Changed 10 months ago by strogdon

  • Branch set to public/ticket/14364-testjava
  • Commit set to b85230b192612c14d2ae1b9de6c03aa0c4fc3e53

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


New commits:

b85230bNo longer use testjava.sh script

comment:17 Changed 10 months ago by jdemeyer

  • Branch changed from public/ticket/14364-testjava to u/jdemeyer/ticket/14364
  • Created changed from 03/27/13 01:30:24 to 03/27/13 01:30:24
  • Modified changed from 12/29/13 20:48:07 to 12/29/13 20:48:07

comment:18 Changed 10 months ago by jdemeyer

  • Status changed from new to needs_review

comment:19 Changed 10 months ago by jdemeyer

  • Commit changed from b85230b192612c14d2ae1b9de6c03aa0c4fc3e53 to c7c227e6006762b1b5b5b9e5113c3fbaf21f24b2

New commits:

c7c227eRemove testjava.sh from jmol package

comment:20 Changed 9 months ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:21 Changed 6 months ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:22 Changed 5 months ago by leif

[ping]

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

If not, works for me...

comment:23 follow-up: Changed 5 months ago by Snark

  • Reviewers set to Snark
  • Status changed from needs_review to positive_review

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.

comment:24 in reply to: ↑ 23 Changed 5 months ago by gutow

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 Snark:

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.

comment:25 follow-up: Changed 5 months ago by Snark

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?

comment:26 in reply to: ↑ 25 Changed 5 months ago by gutow

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 Snark:

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

comment:27 Changed 5 months ago by vbraun

  • Branch changed from u/jdemeyer/ticket/14364 to c7c227e6006762b1b5b5b9e5113c3fbaf21f24b2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:28 Changed 3 months ago by kcrisman

  • Commit c7c227e6006762b1b5b5b9e5113c3fbaf21f24b2 deleted
  • Reviewers changed from Snark to Julien Puydt
Note: See TracTickets for help on using tickets.