Fix stripping of CRLF on Windows. #12356
Conversation
lib/matplotlib/dviread.py
Outdated
| except RuntimeError: | ||
| return '' | ||
| if os.name == 'nt': | ||
| # On Windows only, kpathsea appears to use utf-8 output(?); see | ||
| # __win32_fputs in the kpathsea sources and mpl issue #11848. | ||
| return result.decode('utf-8') | ||
| return result.decode('utf-8')[:-2] # Strip final '\r\n'. |
There was a problem hiding this comment.
wouldn't it be more clear to just .rstrip() in both cases?
There was a problem hiding this comment.
Well, technically filenames can end with spaces... not in this case as we're always passing in the extension, but I think "strip the final newlines" (at least rstrip("\r\n")) is clearer in intent.
There was a problem hiding this comment.
I wouldn't trust any command line tool with trailing whitespace. 😄
Anyway .rstrip("\r\n") would be more safe. Just in case the might be more or less newlines in future versions.
|
I'd just close the backport and milestone everything to 3.1. |
|
I can report that this fixes the issue for me on windows. However, I'm hesitant to approve this myself, since there are no tests for this and my knowledge on linebreaks in different OS is limited. |
cdd850e to
9192d90
Compare
|
Some image comparison tests are broken. Any idea why? |
|
Nope... they don't even look like they usetex (from the names at least :)), so it's likely unrelated, but dunno... |
|
Those image comparissons are broken in other PRs' tests as well. They are only pdf related it seems. Maybe some version in the test suite changed? |
| @@ -1017,16 +1017,16 @@ def find_tex_file(filename, format=None): | |||
| if format is not None: | |||
| cmd += ['--format=' + format] | |||
| cmd += [filename] | |||
| try: # Below: strip final newline. | |||
| result = cbook._check_and_log_subprocess(cmd, _log)[:-1] | |||
There was a problem hiding this comment.
Why did this change?
This is a little hard to review in the absence of tests; What test should I run locally to make sure this doesn't break?
There was a problem hiding this comment.
Maybe simply replace [:-1] with .rstrip('\r\n')?
There was a problem hiding this comment.
That's what we do below... I don't mind doing it for both windows and non-windows, it won't matter in practice.
The main difference is with the encoding though.
There was a problem hiding this comment.
@jklymak I do not know why old TEST_ALL: "yes" builds were removed without making at least a single newer one. I have started a TEST_ALL: "yes" Appveyor build https://ci.appveyor.com/project/Kojoley/matplotlib/build/1.0.133 in my fork to see how long it takes.
|
Superseded by #12351. |
PR Summary
Closes #12308,
Closes #12352 (patch proposed there).
I actually don't think the original PR (#12253) needed to be milestoned to 3.0.x, but now that it has been, this fix needs to go in too.
PR Checklist