Possible mistakes in scripts.dtx

[TMueller83]

Hi SageTeX developers,

I think I have found two issues with scripts.dtx and one thing that could possibly be improved:

1. The first issue with scripts.dtx concerns Line 75: ........ignore = r"^( _st_.goboom|print('SageT| ?_st_.current_tex_line))". There are two mistakes:

   1. The line is indented which leads to an exception
   2. In order to find statements like "print('SageTex....')", the line has to be adapted as follows: ignore = r"^( _st_.goboom|print\('SageT| ?_st_.current_tex_line)". This is because (...) is special regexp syntax and therefore the opening bracket in print( needs to be escaped.

   The problem with the original line is that after removing the incorrect indentation, the print('SageTex...') lines are not correctly identified and consequently the md5 sums always differ, such that the script run-sagetex-if-necessary.py always decides to run Sage again.

2. Another issue is line 59: usepackage = r'\usepackage{sagetex}', since it does not work if sagetex is loaded with some options, such as \usepackage[imagemagick]{sagetex} provided in the example file. Therefore I recommend to change this line as follows: usepackage = r'\usepackage(\[.*\])?{sagetex}'.

3. The check in line 66 could possibly be improved and made be cleaner by removing line 59 completely and change line 66 to if re.search(usepackage, line.split('%')[0]):. This splits the line into a command and comment part and the regexp search is only performed on the command part. This is an improvement, because the original implementation does not work as intended if for example one has a line like \usepackage{amsmath} % \usepackage{sagetex} in which case the script does not recognize that \usepackage{sagetex} is commented out.

[kcrisman]

You are correct about the first thing - @dimpase introduced this (in retrospect) wrong indentation in this commit, and I or someone else should have noticed that. I guess we are so used to Python syntax - sorry and thank you for the report! See #34 for this.

[kcrisman]

As for the other two, I suggest you create a pull request for each of them separately; they seem pretty likely but I'd want to have a few more eyes on any potential side effects. I admit I'm not 100% sure what the ignore line does, and I don't have time to figure it out immediately.

[dimpase]

most importantly, it would be great to have complete test cases (pointing out to some public SageTeX files somewhere is good enough). Thanks!

[TMueller83]

Hi @kcrisman and @dimpase,

thank you both for your responses! @kcrisman thank you for creating the pull request for the wrong indentation. I will create a pull request for the other two issues. I have to admit, that I have never done that before so first I need to check out how it works.

@dimpase: As test case you can take the example.tex that comes with SageTeX. For the first issue (line 75) after only the indentation is corrected you will observe the following behavior:

$ pdflatex example.tex
$ python run-sagetex-if-necessary.py example.tex

The call to pdflatex creates the .sage files and run-sagetex-if-necessary.py will run Sage as it should. So far so good. If one runs run-sagetex-if-necessary.py again by typing

$ python run-sagetex-if-necessary.py example.tex

the expected behavior would be that it decides to not run Sage again. However, due to the incorrect regexp in line 75 the md5 that the script computes does not match the md5 saved in the file example.sagetex.sout and consequently run-sagetex-if-necessary.py reports:

('computed md5:', '0e48d33eb028a3e887b6f3df68162a61')
('sagetex.sout md5:', 'dab77ca1d1fcb235fafb1e5dcf727416')
Need to run Sage on example.

If you now change line 75 to the one I proposed and you run run-sagetex-if-necessary.py on example.tex again, the output is:

('computed md5:', 'dab77ca1d1fcb235fafb1e5dcf727416')
('sagetex.sout md5:', 'dab77ca1d1fcb235fafb1e5dcf727416')
Not necessary to run Sage on example.

which is how it is supposed to be.

For the second issue in line 59 we take example.tex again and in line 20 we replace \usepackage{sagetex} by \usepackage[imagemagick]{sagetex}. Then we run

$ pdflatex example.tex
$ python run-sagetex-if-necessary.py example.tex

and we'll see that run-sagetex-if-necessary.py outputs

example.tex doesn't seem to use SageTeX, exiting.

which is not true, since we use SageTeX however we load it with some optional arguments. If we now change line 59 in run-sagetex-if-necessary.py to the one I have proposed and we run that script again on example.tex it should work as expected.

P.S.: I have edited my initial post in oder to propose an improvement of the part in run-sagetex-if-necessary.py which checks if SageTeX is used or not.

[dimpase]

I just too care of ii in 1. above. See the latest commits

[dimpase]

fixed 2. as well. 3. seems to need more work, please do a PR...

[kcrisman]

For reference, the indentation and part of the syntax error in i. were recognized earlier this year in #19 . We (or I) didn't follow up, so thank you very much @TMueller83

[kcrisman]

@TMueller83 by the way when you get back to us with 3, if you get us a real name we would be happy to add you to the list of contributors for these fixes.

[TMueller83]

@dimpase Thank you for committing the fixes for 1.) and 2.). As far as I understood I have to fork this project in order to create a pull request, since I have no write permission for the original SageTeX project. However, issue 3. is very minor and in reality it will probably not pose a problem. I will anyways shortly explain what I meant with No. 3:

This time we work with the following simple example.tex file (which actually does not use SageTeX):

\documentclass{article}
\usepackage{sagetex}
\begin{document}
Sage\TeX{} is cool!
\end{document}

Now we run pdflatex and on it:

$ pdflatex example.tex

We now 'realize' that our file does not use SageTeX at all, so we comment out line 2 in example.tex:

\documentclass{article}
% \usepackage{sagetex}
\begin{document}
Sage\TeX{} is cool!
\end{document}

If we now run

$ python run-sagetex-if-necessary.py example.tex

then we obtain the supposed response of SageTeX:

example.tex doesn't seem to use SageTeX, exiting.

However, if we modify example.tex as follows

\documentclass{article}
\usepackage{amsmath} % \usepackage{sagetex}
\begin{document}
Sage\TeX{} is cool!
\end{document}

i.e. before the comment % \usepackage{sagetex} we write anything other than spaces, then we have tricked the current commented_out-test in run-sagetex-if-necessary.py since in this case we obtain:

Need to run Sage on example.
Processing Sage code for example.tex...
Sage processing complete. Run LaTeX on example.tex again.

This is, because of the current implementation of the check if \usepackage{sagetex} is commented out or not (i.e. lines 59 and 66 in scripts.dtx). A cleaner approach that should always work would be to split each line at each occurrence of the character % by using line.split('%') and to preform the search for \usepackage{sagetex} only on the part of the line before the first occurrence of %. This is what I have proposed in 3.

@kcrisman Thank you for referencing the older issue concerning the indentation and for considering to mention me in the list of contributors! My real name is Tobias Müller.

[kcrisman]

As far as I understood I have to fork this project in order to create a pull request, since I have no write permission for the original SageTeX project.

Correct, that is the usual procedure.

[TMueller83]

As far as I understood I have to fork this project in order to create a pull request, since I have no write permission for the original SageTeX project.

Correct, that is the usual procedure.

I have never done this since I actually never use github. So if you think that No. 3 is worth it then I would be thankful if you could create the pull request for me (are just two lines). Like I said, in contrast to 1 and 2, No. 3 is pretty minor. So thank you very much for fixing 1 and 2 and I let No. 3 to your consideration.

[dimpase]

On Tue, Jun 11, 2019 at 5:21 PM TMueller83 notifications@github.com wrote:

As far as I understood I have to fork this project in order to create a pull request, since I have no write permission for the original SageTeX project.

Correct, that is the usual procedure.

I have never done this since I actually never use github. So if you think that No .3 is worth it then I would be thankful if you could create the pull request for me (are just two lines). Like I said, in contrast to 1 and 2, No. 3 is pretty minor. So thank you very much for fixing 1 and 2 and I let No. 3 to your consideration.

creating a PR is really easy, you don't need to leave the browser. Click on a file you want to edit; in the right top corner you'll see a "pen" icon. Click on it, and edit the file. Then creating a PR is pretty easy; there will be a "propose file change" button below, and a couple of optional things to fiil in, like a comment.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sagemath/sagetex/issues/33?email_source=notifications&email_token=AAJXYHCRHWOVRD6Z664EH7TPZ7GIXA5CNFSM4HV2EQQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXNWIKI#issuecomment-500917289, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJXYHEPIHSGIICHBLXC7GLPZ7GIXANCNFSM4HV2EQQA .

[TMueller83]

On Tue, Jun 11, 2019 at 5:21 PM TMueller83 @.***> wrote: As far as I understood I have to fork this project in order to create a pull request, since I have no write permission for the original SageTeX project. Correct, that is the usual procedure. I have never done this since I actually never use github. So if you think that No .3 is worth it then I would be thankful if you could create the pull request for me (are just two lines). Like I said, in contrast to 1 and 2, No. 3 is pretty minor. So thank you very much for fixing 1 and 2 and I let No. 3 to your consideration. creating a PR is really easy, you don't need to leave the browser. Click on a file you want to edit; in the right top corner you'll see a "pen" icon. Click on it, and edit the file. Then creating a PR is pretty easy; there will be a "propose file change" button below, and a couple of optional things to fiil in, like a comment. … — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#33?email_source=notifications&email_token=AAJXYHCRHWOVRD6Z664EH7TPZ7GIXA5CNFSM4HV2EQQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXNWIKI#issuecomment-500917289>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJXYHEPIHSGIICHBLXC7GLPZ7GIXANCNFSM4HV2EQQA .

Okay, thank you very much for the explanations! I have done it.

[kcrisman]

This was taken care of in #36.

[dimpase]

by the way, could you review https://trac.sagemath.org/ticket/28732 ?

[kcrisman]

It may take a little time, but probably - and anomalously for the past couple years - I think so.

[kcrisman]

@dimpase - also see the minor issue https://trac.sagemath.org/ticket/28733 which I just discovered.