Skip to content

Backport PR #19108 on branch v3.3.x#19115

Merged
tacaswell merged 4 commits intomatplotlib:v3.3.xfrom
timhoffm:auto-backport-of-pr-19108-on-v3.3.x
Dec 22, 2020
Merged

Backport PR #19108 on branch v3.3.x#19115
tacaswell merged 4 commits intomatplotlib:v3.3.xfrom
timhoffm:auto-backport-of-pr-19108-on-v3.3.x

Conversation

@timhoffm
Copy link
Copy Markdown
Member

@timhoffm timhoffm commented Dec 14, 2020 •
edited
Loading

Backport PR #19108 on branch v3.3.x

In contrast to the original PR, no changes in tests are needed because we did not have tests that would trigger the warning back in 3.3.x.

@timhoffm timhoffm changed the base branch from master to v3.3.x December 14, 2020 23:03
QuLogic
QuLogic previously approved these changes Dec 14, 2020
View reviewed changes
@QuLogic QuLogic closed this Dec 14, 2020
@QuLogic QuLogic reopened this Dec 14, 2020
@QuLogic
Copy link
Copy Markdown
Member

QuLogic commented Dec 14, 2020

Are you sure about that? The test is still failing.

@QuLogic QuLogic dismissed their stale review December 15, 2020 04:55

Tests aren't passing

@tacaswell tacaswell added this to the v3.3.4 milestone Dec 16, 2020
@tacaswell
FIX: Explicitly clean up temporary files in HTMLWriter
400ea30 
- HTMLWriter inherits from FileMovieWriter.
- FileMovieWriter create a non-context manager TemporayDirectory on
  init.
- The TemporayDirectory is cleaned up in the cleaup method of
  FileMovieWriter.
- However this method (and its call chain through finish) try to
  either call a subprocess to merge a bunch of single-frame files into
  a movie (not relevant) or to clean up such a process (not relevant).
- The least disruptive fix is to duplicate the file clean up code.
@tacaswell
Copy link
Copy Markdown
Member

tacaswell commented Dec 16, 2020

@QuLogic and I talked through this today. The issue is:

  • The HTMLWriter inherits from FileMovieWritter which sets up a non-context manager TemporaryDirectory in the in init
  • However the cleanup code is not called (because the super class cleanup code does things that do not make sense for HTMLWriter)
  • we do not see this failure on the default branch because sometime after we branched v3.3.x off the tests got cleaned up a best and the object that triggers the failure is now created in a fixture so is escaping this pytest logic that converts un-handled exceptions into failures.

This code probably should on the main branch, we will handle that with a careful merge up.

@QuLogic
Copy link
Copy Markdown
Member

QuLogic commented Dec 17, 2020

It looks like there's another failure in pgf due to the LatexManager teardown. There is a weakref finalizer for the Popen added by @anntzer in #18679, but it doesn't appear to have been called early enough.

@anntzer
Copy link
Copy Markdown
Contributor

anntzer commented Dec 18, 2020

#18679 doesn't appear to have been backported, and I guess it basically fixed the bug that is now getting exposed by pytest?

@timhoffm @tacaswell
Merge pull request matplotlib#18679 from anntzer/pgftmpdir
1c9518b 
Further simplify pgf tmpdir cleanup.

Backports matplotlib#18679 but drops the deprecation steps.
@tacaswell
Copy link
Copy Markdown
Member

tacaswell commented Dec 18, 2020

I tried to backport the non-deprecation related changes.

I think I missed these failures locally because I have other pgf failures due to latex changing under us (I see this on Arch, assume we will see it in test containers eventually, but that is a problem for the future).

tacaswell
tacaswell approved these changes Dec 18, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have commits in here so leave it to the judgement of further reviewers if they they feel comfortable merging or wait for additional review.

@tacaswell tacaswell mentioned this pull request Dec 18, 2020
Merged
@QuLogic
Copy link
Copy Markdown
Member

QuLogic commented Dec 19, 2020

Ah, sorry, I was looking at master instead of v3.3.x.

QuLogic
QuLogic reviewed Dec 19, 2020
View reviewed changes
Comment thread lib/matplotlib/backends/backend_pgf.py Outdated
@tacaswell tacaswell merged commit e43e840 into matplotlib:v3.3.x Dec 22, 2020
@QuLogic QuLogic mentioned this pull request Feb 5, 2021
Merged
3 tasks
@timhoffm timhoffm deleted the auto-backport-of-pr-19108-on-v3.3.x branch June 10, 2022 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tacaswell tacaswell tacaswell approved these changes

@QuLogic QuLogic QuLogic approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.3.4

Development

Successfully merging this pull request may close these issues.

4 participants

@timhoffm @QuLogic @tacaswell @anntzer