Skip to content
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

Update artresizer's ImageMagick commands to use the magick binary #3236

Merged
merged 9 commits into from Apr 30, 2019

Conversation

Projects
None yet
2 participants
@ababyduck
Copy link
Contributor

commented Apr 23, 2019

Updated artresizer's ImageMagick commands to use the magick binary added in ImageMagick 7.x, rather than the legacy utilities ('convert', 'identify', etc.) This resolves an issue where beets is failing to detect or use ImageMagick on Windows even when it is set correctly on the PATH, which in turn restores functionality to the fetchart and embedart plugins on Windows.

Closes #2093

Update artresizer's ImageMagick commands to use the magick binary
Updated artresizer's ImageMagick commands to use the magick binary
added in ImageMagick 7.x, rather than the legacy utilities ('convert',
'identify', etc.) This resolves an issue where beets is failing to
detect or use ImageMagick on Windows even when it is set correctly
on the PATH, which in turn restores functionality to the fetchart and
embedart plugins on Windows.

Closes #2093
@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

Thanks! This seems like a good thing to do, but as a maintainer I'm contractually obligated to worry about backwards compatibility. How much do we need to worry about systems with only the older versions of IM, without the magick binary, installed? (How many CentOS or Debian versions back would you have to go for that to be the case?) If that's a real possibility, can we set this up to try magick and, if that doesn't exist, fall back to convert?

We do something similar in audioread to switch between avconv and ffmpeg, for example:
https://github.com/beetbox/audioread/blob/c8bedf7880f13a7b7488b108aaf245d648674818/audioread/ffdec.py#L82-L98

@ababyduck

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

You're right, it looks like the CentOS repos are still serving version 6.7 of ImageMagick. Even Ubuntu is on 6.9.

Testing a solution now.

ababyduck added some commits Apr 24, 2019

Added fallback to ImageMagick's legacy utilities
When the `magick` binary is not available, artresizer will fall back
to the "legacy" binaries (`convert`, `identify`, etc.)
Fix whitespace for flake8 compliance
Fixed various whitespace issues and a global variable reference to
comply with flake8 linting.
Refactor to eliminate use of global variable
`get_im_version` now returns an additional bool `isLegacy`, which
indicates whether the the `magick` binary is accessible. It is stored
in `self.im_legacy` on initialization of an `ArtResizer` object, and
can be accessed via `ArtResizer.shared.im_legacy`
Handle TypeError exception when no ImageMagick install is present
Fixes an error introduced in 1a6e0a7 where a TypeError exception was
raised when calling `_check_method()` with no ImageMagick installation
present.
@ababyduck

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

How do you feel about this solution?

Tested and working with both fetchart and embedart on:

  • Windows 10, ImageMagick 7.0.8-41 with and without magick
  • Ubuntu 18.04.1 LTS, ImageMagick 6.9.7-4 without magick, with and without convert
@sampsyo
Copy link
Member

left a comment

Thanks! This looks like it's on the right track. Here are a few coding suggestions.

Show resolved Hide resolved beets/util/artresizer.py Outdated
Show resolved Hide resolved beets/util/artresizer.py Outdated
Show resolved Hide resolved beets/util/artresizer.py Outdated
Show resolved Hide resolved beets/util/artresizer.py Outdated
Show resolved Hide resolved beets/util/artresizer.py Outdated
Make requested changes for PR 3236
- Refactored convert and identify command names to an ArtResizer
member variable, set on ArtResizer init. Functions that use this info
will now access it from there.
- Changed the way `cmd` variables are written so that the command name
and command args are assigned directly to `cmd`, rather than doing
`command_output(cmd + args)`
- `get_im_version()` will now always return a tuple containing two
values: a tuple representing the version, and either a bool or None
flag representing whether we should send legacy commands to ImageMagick
- Improved readability of successful return value in `get_im_version()`
@ababyduck

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

Thanks for the review! I've implemented the requested changes in 48be3a7.

Make `get_im_version()` return same types across all conditions
`get_im_version` should now always return a tuple containing:
- index 0: a tuple representing the version
- index 1: a bool or None, representing legacy status
@sampsyo
Copy link
Member

left a comment

Great; thank you!! Here is another round of low-level suggestions. Thanks for sticking with this! 😃

Show resolved Hide resolved beets/util/artresizer.py Outdated
Show resolved Hide resolved beets/util/artresizer.py Outdated
Show resolved Hide resolved beets/util/artresizer.py Outdated
Show resolved Hide resolved beets/util/artresizer.py Outdated
Show resolved Hide resolved beets/util/artresizer.py
Show resolved Hide resolved beets/util/artresizer.py Outdated
Show resolved Hide resolved beets/util/artresizer.py Outdated
Show resolved Hide resolved beets/util/artresizer.py Outdated
Show resolved Hide resolved beets/util/artresizer.py Outdated
Show resolved Hide resolved beets/util/artresizer.py Outdated
Make more requested changes for PR 3236
- Moved several variable assignments outside of try blocks
- Added and clarified various comments and docstrings
- Modified the command loop in `get_im_version()` to a slightly more
readable approach
- `get_im_version()` now returns None when ImageMagick is unavailable
- Updated `ArtResizer._check_method` to handle our new returns in a way
that is more readable
- Fixed an issue where `get_im_version()` could crash if the regex
search failed to find a match
@ababyduck

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

Looks like Appveyor's failing tests because it's having issues setting up unrar in the test environment. 👌

Latest round of requested changes implemented in 278d87f.

@sampsyo sampsyo merged commit 278d87f into beetbox:master Apr 30, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

sampsyo added a commit that referenced this pull request Apr 30, 2019

Merge pull request #3236 from ababyduck/2093-update-imagemagick-cmds
Update artresizer's ImageMagick commands to use the magick binary

sampsyo added a commit that referenced this pull request Apr 30, 2019

sampsyo added a commit that referenced this pull request Apr 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.