Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Simplify tmpdir handling in backend_pgf.#18654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| shutil.rmtree(tmpdir) | ||
| except: | ||
| TmpDirCleaner.add(tmpdir) | ||
| with TemporaryDirectory() as tmpdir: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We seriously don't have even a smoketest for this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good. Would love to have a test exercise the entirely unexecuted method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agree that an example would be good.
backend_pgf uses a complicated way to handle temporary directories, inparticular because (on top of the normal calls to tex) it runs aseparate, long-standing tex instance to compute text bounding boxes.I'm not sure this is actually needed, but at least, for the placeswhere we just run tex locally, we can certainly just use normalTemporaryDirectories with automatic cleanup.Do so in _print_pdf_to_fh (also with some pathlibification),_print_png_to_fh, and PdfPages._run_latex (where all the temporary filescan be set up in `_run_latex`, rather than in `__init__`, which savesthe need for a bunch of attributes).
Added a smoketest. I really don't want to add a new baseline image here because even the current pgf tests don't pass locally; my suspicion is that we have the same problem as with freetype versions, but worse because now it's the freetype versions used by the tex machinery, which we don't control... |
The problem that |
My understanding is that that's relevant for the long-running LatexManager (and even then my guess is that we could arrange for LatexManager to properly clean after itself). Here we're only looking at fully self-contained tex invocations, which should all be done by the end of the block. |
It seems like all these code paths end up going through |
That's my point. Compare with |
Well, let's see if anyone complains. |
Ah, it looks like some of this was also fixed on Python 3.4 per#1324 (comment). In any case I've confirmed locally that things work on Windows. |
backend_pgf uses a complicated way to handle temporary directories, in
particular because (on top of the normal calls to tex) it runs a
separate, long-standing tex instance to compute text bounding boxes.
I'm not sure this is actually needed, but at least, for the places
where we just run tex locally, we can certainly just use normal
TemporaryDirectories with automatic cleanup.
Do so in _print_pdf_to_fh (also with some pathlibification),
_print_png_to_fh, and PdfPages._run_latex (where all the temporary files
can be set up in
_run_latex, rather than in__init__, which savesthe need for a bunch of attributes).
PR Summary
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsandpydocstyle<4and runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).