Skip to content

Ignore ImageMagick deprecation of "convert" command. #29569

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Feb 3, 2025

Ignoring the deprecation warning (present since IM 7.1.1-33, May 2024) is the quickest fix.

The proper fix would be to switch to using "magick" everywhere but the command's syntax is a bit different and the CLI usage thus needs to be verified; moreover this would break users with very old installs of imagemagick (unless adding some version sniffing code) although IMv7 has been out since 2016 so that is likely acceptable. (That proper fix likely also needs some fiddling around the special-casing of rcParams["animation.convert_path"] = "convert" in ImageMagickBase.bin_path; e.g. should the rcParams be renamed to "animation.magick_path" and have "magick" be the special case...)

Closes (in the short term) #29567, although (per the above) a better fix would be welcome, and it would also be nice if this could have been caught by CI... Likely worth a backport too?

PR summary

PR checklist

Ignoring the deprecation warning (present since IM 7.1.1-33, May 2024)
is the quickest fix.

The proper fix would be to switch to using "magick" everywhere but the
command's syntax is a bit different and the CLI usage thus needs to be
verified; moreover this would break users with very old installs of
imagemagick (unless adding some version sniffing code) although IMv7 has
been out since 2016 so that is likely acceptable.  (That proper fix
likely also needs some fiddling around the special-casing of
`rcParams["animation.convert_path"] = "convert"` in
ImageMagickBase.bin_path; e.g. should the rcParams be renamed to
`"animation.magick_path"` and have "magick" be the special case...)
@anntzer anntzer added Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: animation labels Feb 3, 2025
@tacaswell
Copy link
Member

All of the windows failures are download issues on installing system deps (nija via chocolaty on azure and minimamba on appveyor).

@anntzer
Copy link
Contributor Author

anntzer commented Feb 4, 2025

Not that it really matters, but does anyone know why the "topic: animation" labeled was auto-removed by a bot?

@timhoffm
Copy link
Member

timhoffm commented Feb 4, 2025

Not sure, but I have seen some interference when I try to modify labels right after creating a PR. I assume the bot and me are editing in parallel and that's a race condition.

@jklymak jklymak merged commit ba90bce into matplotlib:main Feb 4, 2025
46 of 48 checks passed
@anntzer anntzer deleted the im71133 branch February 4, 2025 16:51
@QuLogic
Copy link
Member

QuLogic commented Feb 4, 2025

Not that it really matters, but does anyone know why the "topic: animation" labeled was auto-removed by a bot?

That label is only applied for animation.py/_animation_data.py:

"topic: animation":
- changed-files:
- any-glob-to-any-file:
- 'lib/matplotlib/animation.py*'
- 'lib/matplotlib/_animation_data.py*'

Unfortunately, it doesn't support preserving manually added labels. You can add the label after the action has run though (but keep in mind it runs on pushes.)

@QuLogic QuLogic added this to the v3.11.0 milestone Feb 4, 2025
@anntzer
Copy link
Contributor Author

anntzer commented Feb 6, 2025

Do we really need the topic autolabeler?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: animation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants