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

[Grillard]

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.

[janschulz]

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]

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

[WeatherGod]

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]

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...

[janschulz]

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]

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

[Grillard]

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!!

[janschulz]

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]

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!

[janschulz]

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]

The comment is for the wrong line now.

[QuLogic]

There should be spaces around the = and after the ,.

[Grillard]

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]

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]

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

[janschulz]

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?

[tacaswell]

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

[janschulz]

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.

[janschulz]

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]

1. This is indeed related to #5923
2. 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]

Thank you both for the explanation.

[tacaswell]

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

[tacaswell]

backported to 1.5.x as ab7c982

[janschulz]

@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...

[janschulz]

@grillard Congratulations from me as well! :-)

[tacaswell]

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

[janschulz]

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

[tacaswell]

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]

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

[janschulz]

Test in #5958