fix for latex call on PS backend (Issue #5895) #5928
Conversation
| @@ -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~") | |||
There was a problem hiding this comment.
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).
|
I think I added a new commit instead of amending the old one :/ |
|
That ok, you can "squash" it! On Wed, Jan 27, 2016 at 3:36 PM, Grillard notifications@github.com wrote:
|
|
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... |
| @@ -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~") | |||
There was a problem hiding this comment.
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 withsquashinstead ofpick, 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
|
ups, I think I screw up a little. I am even more confused now... Sorry! |
|
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!! |
|
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: No an editor is shown where each commit in your branch is shown in one line which has a For the all but the first commit, replace the -> Save and close the editor Save and close the editor. You can check what you produces by calling Finally, push that to your PR via |
|
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! |
|
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 |
| @@ -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 | |||
There was a problem hiding this comment.
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.
|
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) |
|
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. |
| @@ -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("\\", "/") | |||
There was a problem hiding this comment.
Is this a fix to on unrelated problem to the one reported in #5923 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
So has the ps backend just always been broken on windows?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
So, it recongnizes the string, but it tried to make all |
|
Thank you both for the explanation. |
fix for latex call on PS backend closes #5923
|
@Grillard Congratulations on what I think is your first (hopefully of many) contributions to mpl. |
fix for latex call on PS backend closes #5923
|
backported to 1.5.x as ab7c982 |
|
@tacaswell IMO this should get a testcase: This triggers the bug in an older version and not anymore in a newer one... |
|
@Grillard Congratulations from me as well! :-) |
|
I do not think we have even smoke tests for the ps backend . |
|
The funny thing is there is a |
|
Oh, so we do. I should have also known that because that file is the source of many of the transient travis failures. The |
|
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.
|
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
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.