fix for latex call on PS backend (Issue #5895) #5928

Grillard · 2016-01-27T18:44:06Z

If the ~ character appears on the path of the file, latex might recognize it as a line break, and therefore fail. By replacing the ~ for \string~ we can avoid this.

jankatins · 2016-01-27T20:03:35Z

lib/matplotlib/backends/backend_ps.py

@@ -1467,6 +1467,7 @@ def convert_psfrags(tmpfile, psfrags, font_preamble, custom_preamble,

    command = '%s cd "%s" && dvips -q -R0 -o "%s" "%s" > "%s"'%(precmd, tmpdir,
                os.path.split(psfile)[-1], os.path.split(dvifile)[-1], outfile)
+    command=command.replace("~","\\string~")


IMO this is the wrong place (probably because the line number is the same): you changed the command for dvips, but your bug occurred in the latex line which si now a few lines up.

Otherwise two small additions:

could you add a small comment why this is needed?

This should also make sure that only the input file (->latexfile) is treated like this, not the complete command line (e.g. the outfile might also contain a ~, but there it would result in a completely different filename).

Not sure if you need this or not, but just in case: just edit the file again, git add ., git diff (should show the removal of the wrong fix and includes the new fix), git commit --amend (overwrite the last commit) and then git push -f (force push to the PR).

Grillard · 2016-01-27T20:35:57Z

I think I added a new commit instead of amending the old one :/

WeatherGod · 2016-01-27T20:47:31Z

That ok, you can "squash" it!
https://ariejan.net/2011/07/05/git-squash-your-latests-commits-into-one/

On Wed, Jan 27, 2016 at 3:36 PM, Grillard notifications@github.com wrote:

I think I added a new commit instead of amending the old one :/

—
Reply to this email directly or view it on GitHub
#5928 (comment)
.

Grillard · 2016-01-27T21:11:47Z

I think I am a bit confused now... I was using the web editor since it was such a small change, I thought it would be enough, then I put the change in the wrong line, so I cloned the forked repository into my computer to have access to git, ( thats the merge branch I guess), tried to squash but now we have 4 commits instead of two hahaha...

jankatins · 2016-01-27T21:12:30Z

lib/matplotlib/backends/backend_ps.py

@@ -1454,6 +1454,8 @@ def convert_psfrags(tmpfile, psfrags, font_preamble, custom_preamble,
    else: precmd = ''
    command = '%s cd "%s" && latex -interaction=nonstopmode "%s" > "%s"'\
                %(precmd, tmpdir, latexfile, outfile)
+    # Replace ~ so Latex does not think it is line break
+    command=command.replace("~","\\string~")


Can you still move that up and change latexfile and not command?

Squash (this assumes that you use git in the command line; if not, please tell... :-)):

Make the changes

add a new commit, git add . & git commit -m "Fixup"

rebase-squash it all commits into only one: git rebase -i origin/master -> prepend all the commits but teh first one with squash instead of pick, close the editor and let rebase do its thing -> you will be prompted by an editor which shows all the commit messages combined -> edit it until you are happy

force push: git push -f

Grillard · 2016-01-28T12:03:41Z

ups, I think I screw up a little. I am even more confused now... Sorry!

Grillard · 2016-01-28T13:21:28Z

Ok, so I have a HUGE mess with the commits, not sure how to fix/clean that, I also noticed that what I had push did not work properly, I found a way to fix it properly now, and it is pushed. at least in the files modified is still clean what the changes are. I am sorry for the huge mess!!

jankatins · 2016-01-28T14:39:45Z

Hey, we all started with git at some time :-)

I'm not sure how you got to this git history, but here are two ways to get back to a working PR:

Version 1: Just close this PR and start a new one, where you only include the additons from https://github.com/Grillard/matplotlib/commit/cc4b1221ea2fdd5c07e0bc52ff55799dfc35789d :-)

Version 2: on the commandline do the following commands:

# This assumes that you have your own fork clones via 
# `git clone git@github.com:Grillard/matplotlib.git` -> if you do that this for will be registered as `origin`

# add a new remote which points to the matplotlib repo itself
git add remote upstream https://github.com/matplotlib/matplotlib.git

# fetch the latest stuff from that remote
git fetch upstream

# interactively rebase your changes on top of the latest upstream work and 
# also removing unwanted stuff and and "squashing" your commits into only one
git rebase upstream/master -i

No an editor is shown where each commit in your branch is shown in one line which has a pick in front of it. Delete all lines which are not from you -> git will remove these commits form your branch.

For the all but the first commit, replace the pick by a squash. You should end up with somethign like this:

pick 255669fa3d1a9ebb8b309bad0de5606a0bcc4267 fix for latex call on PS backend 
squash 7f2fbcbeb73bdfc18cdb228198a766a752664120 ...
squash 5da72d8ee28f296ddfb4fecfda33ee5877a14191 ...
...
squash cc4b1221ea2fdd5c07e0bc52ff55799dfc35789d Fix

-> Save and close the editor
-> git rebase will now do its work and in the end present you with an editor which lists the squashed commit message. Edit it, so that it has a headline and a short paragraph why the change was needed. E.g. something like this:

Fix for latex call on PS backend on windows

Sometimes, the filename for "latexfile" ends up in the short 8.3 format on windows, but latex does not like the `~ ` in the filename. 

Closes: https://github.com/matplotlib/matplotlib/issues/5923

Save and close the editor.

You can check what you produces by calling gitk. It should list only one commit by you containing the two line change and your commit message.

Finally, push that to your PR via git push -f (force -> overwrites what is already there; without -f, only adding new commits is possible)

Grillard · 2016-01-28T14:55:00Z

uff!, that was useful thanks!. now this looks as it supposed to!

I really like git!, it only get confusing sometimes!

about the changes: note that I also needed to change \ for /, since latex recognize the \ as a function call!

jankatins · 2016-01-28T15:20:52Z

Wow, great. Let's wait if the CI tools go green on this and if so I'm +1 on merging it :-)

Not sure if a test for this behaviour would be good... Maybe by setting TMP/TMPDIR/TEMP in one of the tests to a new dir with a ~ in it. Maybe that works even on linux? windows currently does not do any latex tests as it is not installed... Another reason to do the conda latex package...

QuLogic · 2016-01-28T19:48:34Z

lib/matplotlib/backends/backend_ps.py

@@ -1452,6 +1452,9 @@ def convert_psfrags(tmpfile, psfrags, font_preamble, custom_preamble,
    # multiple
    if sys.platform == 'win32': precmd = '%s &&'% os.path.splitdrive(tmpdir)[0]
    else: precmd = ''
+    # Replace ~ so Latex does not think it is line break


The comment is for the wrong line now.

Sometimes, the filename for "latexfile" ends up in the short 8.3 format on windows, but latex does not like the `~ ` in the filename.

Grillard · 2016-01-28T22:19:42Z

Ok, I modified based on QuLogic feedback. (I also squashed the last commit, I am not sure if this is the standard, if not please let me know)

tacaswell · 2016-01-30T19:57:47Z

The standards on squashing or not vary from project to project (and ever PR to PR). The pros of aggressively squashing is it keeps the project git history looking 'clean', but it puts a higher burden on occasional committers, coarse grains changes (which can make forensics harder), and makes people in the habit of regularly force pushing.

tacaswell · 2016-01-30T20:02:30Z

lib/matplotlib/backends/backend_ps.py

@@ -1452,6 +1452,10 @@ def convert_psfrags(tmpfile, psfrags, font_preamble, custom_preamble,
    # multiple
    if sys.platform == 'win32': precmd = '%s &&'% os.path.splitdrive(tmpdir)[0]
    else: precmd = ''
+    #Replace \\ for / so latex does not think there is a function call
+    latexfile = latexfile.replace("\\", "/")


Is this a fix to on unrelated problem to the one reported in #5923 ?

As far as I understand the latex stuff, it is needed because otherwise the next line would be interpreted as introducing a new directory "break":

c:\something\short~1\tempfile -> c:/something/short~1/tempfile -> c:/something/short\\string~1/tempfile

Without the first step, the dirs would read c: - something - short - string~1 - tempfile.

@Grillard can you confirm that?

So has the ps backend just always been broken on windows?

No: you can also specify all slashes as backlashes: the name is passed to a program and either used as filename with backlashes but no ~ allowed (is probably internally converted to forwards slashes) or as a latex input with / as path separator which can contain \string~ but no backlashes.

-> It was only broken when you use a short name as tempfile which contains ~ because latex has that as a special character which needs to be escaped.

It will probably be also broken on unix, if you add a testcase where you set TMP, TMPDIR and TEMP (whatever mkstemp used first is probably enough) to a path with a ~ in it. As far as I know, these are valid filenames on unix?!

Would probably enough to do that in the ps tests by simply adding a testcase which sets that in os.environ before doing the mpl calls...

Grillard · 2016-01-31T16:05:28Z

This is indeed related to #5923
without the first step latex thinks that all \ are a command call (or control sequence idk), so basically it reads like:

! Undefined control sequence.
<*> c:\users
            \xxxxxx\string~1\appdata\local\temp\tmpofdp_k.tex
! Undefined control sequence.
<*> c:\users\xxxxxx
                   \string~1\appdata\local\temp\tmpofdp_k.tex
! Undefined control sequence.
<*> c:\users\xxxxxx\string~1\appdata
                                    \local\temp\tmpofdp_k.tex

So, it recongnizes the string, but it tried to make all \ as a function call.
3. I think it is what JanSchulz said: you either have \ only, but no ~ allowed, or you go with / but then every \ is recongized as a function call.
I tested this by replacing the shortened username by the long username so the ~ is removed, and working fine, confirming that backslashes only works too.

tacaswell · 2016-02-01T22:43:56Z

Thank you both for the explanation.

fix for latex call on PS backend closes #5923

tacaswell · 2016-02-01T22:47:46Z

@Grillard Congratulations on what I think is your first (hopefully of many) contributions to mpl.

fix for latex call on PS backend closes #5923

tacaswell · 2016-02-01T22:48:42Z

backported to 1.5.x as ab7c982

jankatins · 2016-02-02T00:22:01Z

@tacaswell IMO this should get a testcase:

import os
os.environ["TMPDIR"] = "c:\\temp\\s~1"
# del the tempdir cache, so that an older value is removed
import tempfile
tempfile.tempdir = None
import matplotlib as mpl
mpl.use("ps")
assert mpl.get_backend() == "ps"
import matplotlib.pyplot as plt
plt.rc('text', usetex=True)
plt.plot([1,2,3,4])
plt.xlabel(r'\textbf{time} (s)')
mpl.verbose.set_level("debug")
plt.savefig('tex_demo.eps')

This triggers the bug in an older version and not anymore in a newer one...

jankatins · 2016-02-02T00:23:30Z

@Grillard Congratulations from me as well! :-)

tacaswell · 2016-02-02T00:27:45Z

I do not think we have even smoke tests for the ps backend .

jankatins · 2016-02-02T00:33:36Z

The funny thing is there is a test_backend_ps.py, but it doesn't set mpl.use("ps")?!?

tacaswell · 2016-02-02T00:37:25Z

Oh, so we do. I should have also known that because that file is the source of many of the transient travis failures.

The format='ps' makes sure the correct backend is used on save.

Grillard · 2016-02-02T12:13:08Z

Many thanks @tacaswell and @JanSchulz . Happy to be helpful. I learned a lot about git too!.

The PS uses latex in some cases (usetex=True) and latex does not like to be called with dirnames with ~ in it (reserved char). The Fix for that was in matplotlib#5928, this is just a testcase to test for the fix.

jankatins · 2016-02-02T13:47:28Z

Test in #5958

The PS uses latex in some cases (usetex=True) and latex does not like to be called with dirnames with ~ in it (reserved char). The Fix for that was in matplotlib#5928, this is just a testcase to test for the fix.

fix for latex call on PS backend closes matplotlib#5923

mdboom added the status: needs review label Jan 27, 2016

jankatins reviewed Jan 27, 2016
View reviewed changes

QuLogic reviewed Jan 28, 2016
View reviewed changes

Fix for latex call on PS backend on windows

e6b9604

Sometimes, the filename for "latexfile" ends up in the short 8.3 format on windows, but latex does not like the `~ ` in the filename.

tacaswell added this to the 1.5.2 (Critical bug fix release) milestone Jan 30, 2016

tacaswell reviewed Jan 30, 2016
View reviewed changes

tacaswell added a commit that referenced this pull request Feb 1, 2016

Merge pull request #5928 from Grillard/master

190c923

fix for latex call on PS backend closes #5923

tacaswell merged commit 190c923 into matplotlib:master Feb 1, 2016

tacaswell removed the status: needs review label Feb 1, 2016

tacaswell added a commit that referenced this pull request Feb 1, 2016

Merge pull request #5928 from Grillard/master

ab7c982

fix for latex call on PS backend closes #5923

jankatins mentioned this pull request Feb 2, 2016

TST: add a test for tilde in tempfile for the PS backend #5958

Merged

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request May 22, 2016

Merge pull request matplotlib#5928 from Grillard/master

92f463c

fix for latex call on PS backend closes matplotlib#5923

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for latex call on PS backend (Issue #5895) #5928

fix for latex call on PS backend (Issue #5895) #5928

Grillard commented Jan 27, 2016

jankatins Jan 27, 2016

Grillard commented Jan 27, 2016

WeatherGod commented Jan 27, 2016

Grillard commented Jan 27, 2016

jankatins Jan 27, 2016

Grillard commented Jan 28, 2016

Grillard commented Jan 28, 2016

jankatins commented Jan 28, 2016

Grillard commented Jan 28, 2016

jankatins commented Jan 28, 2016

QuLogic Jan 28, 2016

Grillard commented Jan 28, 2016

tacaswell commented Jan 30, 2016

tacaswell Jan 30, 2016

jankatins Jan 30, 2016

tacaswell Jan 31, 2016

jankatins Jan 31, 2016

jankatins Jan 31, 2016

Grillard commented Jan 31, 2016

tacaswell commented Feb 1, 2016

tacaswell commented Feb 1, 2016

tacaswell commented Feb 1, 2016

jankatins commented Feb 2, 2016

jankatins commented Feb 2, 2016

tacaswell commented Feb 2, 2016

jankatins commented Feb 2, 2016

tacaswell commented Feb 2, 2016

Grillard commented Feb 2, 2016

jankatins commented Feb 2, 2016

fix for latex call on PS backend (Issue #5895) #5928

fix for latex call on PS backend (Issue #5895) #5928

Conversation

Grillard commented Jan 27, 2016

Choose a reason for hiding this comment

Grillard commented Jan 27, 2016

WeatherGod commented Jan 27, 2016

Grillard commented Jan 27, 2016

Choose a reason for hiding this comment

Grillard commented Jan 28, 2016

Grillard commented Jan 28, 2016

jankatins commented Jan 28, 2016

Grillard commented Jan 28, 2016

jankatins commented Jan 28, 2016

Choose a reason for hiding this comment

Grillard commented Jan 28, 2016

tacaswell commented Jan 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Grillard commented Jan 31, 2016

tacaswell commented Feb 1, 2016

tacaswell commented Feb 1, 2016

tacaswell commented Feb 1, 2016

jankatins commented Feb 2, 2016

jankatins commented Feb 2, 2016

tacaswell commented Feb 2, 2016

jankatins commented Feb 2, 2016

tacaswell commented Feb 2, 2016

Grillard commented Feb 2, 2016

jankatins commented Feb 2, 2016