Fix sws2rst

[jdemeyer]

sws2rst abuses tempfile.gettempdir() such that it breaks if os.path.join(tempfile.gettempdir(), file_name) happens to exist already (in particular, if the original .sws file was stored in /tmp).

Also, I don't like

    try:
        process_sws(file_name)
    except Exception as e:
        print "Unable to process file ('%s')" % file_name
        print "Error message: %s" % e
        print "Exiting."
        sys.exit(1)

I would much rather see a full traceback rather than a "beautified" useless error message.

This strange assumption should also be removed:

Remark:

    The sws file(s) should be in the current directory; using an
    absolute path will result in an error."""

[kcrisman]

Are you suggesting something like the following?

src/bin/sage-sws2rst

@@ -50 +50 @@
     sws_file = tarfile.open(file_name, mode='r:bz2')
     #TODO: python complains about using tempnam, but I don't
     #know hot to fix it or see any danger
-#    tempname = os.tempnam('.')
-    tempname = os.path.join(tempfile.gettempdir(), file_name)
+    tempname = os.tempnam('.')
+#    tempname = os.path.join(tempfile.gettempdir(), file_name)
     sws_file.extractall(tempname)
     base_name = os.path.split(os.path.splitext(file_name)[0])[1]
     base_name_clean = base_name.replace(' ','_')
@@ -180 +180 @@
 
 for file_name in args:
     print "Processing "+file_name
-    try:
-        process_sws(file_name)
-    except Exception as e:
-        print "Unable to process file ('%s')" % file_name
-        print "Error message: %s" % e
-        print "Exiting."
-        sys.exit(1)
+    process_sws(file_name)

I don't know whether the assumption is incorrect, so I would rather not remove it. Probably the guy who wrote sws2rst originally had problems with it.

[kcrisman]

I checked it out, and the ​sort of security implications that using os.tempnam has is very, very unlikely to hit someone creating a tutorial. Otherwise I guess we could use ​temp file.mkdtemp(). I'd appreciate your advice on which option seems better, I haven't tested either.

src/bin/sage-sws2rst

@@ -51 +51 @@
     #TODO: python complains about using tempnam, but I don't
     #know hot to fix it or see any danger
 #    tempname = os.tempnam('.')
-    tempname = os.path.join(tempfile.gettempdir(), file_name)
+    tempdirname = tempfile.mkdtemp()
+    tempname = os.path.join(tempdirname, file_name)
     sws_file.extractall(tempname)
     base_name = os.path.split(os.path.splitext(file_name)[0])[1]
     base_name_clean = base_name.replace(' ','_')
@@ -101 +102 @@
     print "File at "+rst_file
     print "Image directory at "+images_dir
 
-    shutil.rmtree(tempname)
+    shutil.rmtree(tempdirname)

[jdemeyer]

Replying to kcrisman:

I checked it out, and the ​sort of security implications that using os.tempnam has is very, very unlikely to hit someone creating a tutorial.

os.tempnam simply shouldn't be used. Why use something potentially dangerous if there are better alternatives?

Otherwise I guess we could use ​tempfile.mkdtemp().

Good!

[jdemeyer]

Wouldn't just

tempname = tempfile.mkdtemp() 

work, without the os.path.join(..., file_name) part?

[kcrisman]

Wouldn't just

tempname = tempfile.mkdtemp() 

work, without the os.path.join(..., file_name) part?

Probably, but I was trying to keep things pretty minimal in change. Perhaps there is a reason to keep the filename involved.

[kcrisman]

Okay, hopefully this will be fine. I couldn't get it to break with various choices of directory and path, but please do try to break it.

Incidentally, I was able to get Trac to barf by accidentally putting in a branch name starting with a colon... that seems a bit excessive, but of course I have no idea how all the plugins work.

---

New commits:

​5d0c639

Update sws2rst temp files and a bit else

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​6eed79e

Modernize sage-sws2rst script

[jdemeyer]

I ended up changing a lot of the sage-sws2rst script, but I think it is better now and less prone to breakage. If you agree, you can set this to positive_review.

[jdemeyer]

Do you happen to know why the try/except was put here originally?

    #"data" dir
    data_path = os.path.join(tempname,'sage_worksheet','data')
    if os.path.exists(data_path):
        for image in os.listdir(data_path):
            try:
                shutil.move(os.path.join(data_path, image), os.path.join(images_dir, image.replace(' ','_')))
            except shutil.Error:
                pass

I cannot see how this move could generate an error which can be ignored, so I removed the exception handling.

[kcrisman]

I ended up changing a lot of the sage-sws2rst script, but I think it is better now and less prone to breakage. If you agree, you can set this to positive_review.

Yes, and you can add yourself to the author log too since you added me.

Regarding your original remark

This strange assumption should also be removed:
Remark:

    The sws file(s) should be in the current directory; using an
    absolute path will result in an error."""

see here and here - and Pablo said he did it, but somehow it didn't make it in. But it turned out that switching to mkdtemp() completely removed the problem anyway, since the only place two paths were joined (as opposed to a path and a string like 'data', or something from a list of a directory) was there. So you were okay to remove that, and I have tested it with the latest version.

I cannot see how this move could generate an error which can be ignored, so I removed the exception handling.

How about a permissions error or something about too big of a file, or weird characters, or...? (Just brainstorming.) That could be safely ignored; in the event of emergency, just don't copy that particular file, I guess that was the thinking. Because after all otherwise you could never really use sws2rst at all on that worksheet. Because of this, and because this was in the very first version Pablo put up, so I'm reluctant to have it removed.

Otherwise all should be well, I even tested some of the more unusual error possibilities by renaming/moving/not moving various files.

[jdemeyer]

Replying to kcrisman:

How about a permissions error or something about too big of a file, or weird characters, or...? (Just brainstorming.) That could be safely ignored;

I don't see why such errors can be "safely ignored". The .rst file refers to (some of) these images! I would prefer to get an error instead of a silently broken .rst file.

[kcrisman]

How about a permissions error or something about too big of a file, or weird characters, or...? (Just brainstorming.) That could be safely ignored;

I don't see why such errors can be "safely ignored". The .rst file refers to (some of) these images!

Hmm, well, there is that. But that only is a problem when using Sphinx; those references can always be deleted. Indeed, there is usually at least a little tweaking needed anyway, though amazingly little. I guess it comes down to whether it's worse to have references that don't work in the rst file, or no rst file at all, and I would lean middling-heavy on the latter.

[jdemeyer]

Replying to kcrisman:

I guess it comes down to whether it's worse to have references that don't work in the rst file, or no rst file at all, and I would lean middling-heavy on the latter.

A compromize could be to have an option --rst-only to create just the .rst file without the images.

[kcrisman]

I guess it comes down to whether it's worse to have references that don't work in the rst file, or no rst file at all, and I would lean middling-heavy on the latter.

A compromize could be to have an option --rst-only to create just the .rst file without the images.

We already have the help and sphinxify options, that seems reasonable. Something like

parser.add_option("--rst-only",
                  action="store_true", dest="rst_only",
                  help="Create just a .rst file, without images or other resources")

I'm in the middle of a different branch right now, unfortunately, and am not yet comfortable enough with git stash to deal with this... - we'd need to update usage as well.

Incidentally, the jmol images now start with pngs so it could be possible to extract those as well - obviously, not here!

[jdemeyer]

If you agree, we could add the --rst-only in a different ticket.

[kcrisman]

If you agree, we could add the --rst-only in a different ticket.

Haha, true, except then I might do to you what you did to me in #17265 and make this ticket depend on that one ;-)

[jdemeyer]

Well, I don't consider it a bug that sage-sws2rst gives an error if there was an error copying files...

[kcrisman]

Hmm, good point. Though it's also a good point that this is a change in behavior.

However, enough bikeshedding; presumably this is used rarely enough in such cases (or at all?) that we should do that. Put your fix there and I'll get to it at some point soon, this is a green light.