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

Fix using wand on Windows, and allow loading of binary blobs #261

Merged
merged 14 commits into from Nov 29, 2015

Conversation

@roelandschoukens
Copy link
Contributor

@roelandschoukens roelandschoukens commented Oct 19, 2015

I made some changes to make wand work on Python 3.5 on Windows:

  • Python 3.5 breaks a function needed to load the standard C library
  • With standard ImageMagick installs the location of the DLLs is not stored in %MAGICK_HOME%, but in the registry.

I also needed to read images from binary blobs, which requires passing the width, height and depth, as well as the actual blob. So I changed the Image() constructor to accept that combination of arguments.

…indows Binary Release

Work around issue on Python 3.5 (ctypes.util.find_msvcrt() is broken)
…rs with the filename, file or blob parameter, which is necessary to load raw image data.

Added the depth parameter to specify the image depth in raw images
bytes([1, 2, 3]) doesn't have the desired behaviour in Python 2.x
@dahlia
Copy link
Collaborator

@dahlia dahlia commented Nov 14, 2015

Sorry for late review. Could you add .. versionadded::/.. versionchanged:: directives into docstring? Also please write changelog (docs/changes.rst) as well. The next release would be 0.4.2.

…n setting a fake file name.

 * allow loading raw pixel data using a file name
 * add a few extra tests
 * added version to the docstrings
@roelandschoukens
Copy link
Contributor Author

@roelandschoukens roelandschoukens commented Nov 16, 2015

No problem, I will make the changes.

I have made a few extra changes as well, see the last commit on my fork:

  • use MagickSetFormat for changing the format of blobs, rather than setting a fake file name.
  • allow loading raw pixel data using a file name
  • add a few extra tests
  • added version to the docstrings

I was also thinking of moving some of that new functionality from the constructor into the read() function.

One outstanding issue is that using ctypes.c_void_p is broken on 64-bit Windows. Sometimes calling a function which returns or accepts such pointer will raise an OverflowError for no apparent reason. I have not found a fix which doesn't break other things, but it means that wand (and probably a lot of other packages) can be used reliably only on 32-bit installations.

# yet found a suitable fix for this problem
#
# One proposed solution is subclassing ctypes.c_void_p, but that breaks the
# assert statements.

This comment has been minimized.

@dahlia

dahlia Nov 16, 2015
Collaborator

How does a subclass of c_void_p break the assert statements? Which asserts?

This comment has been minimized.

@roelandschoukens

roelandschoukens Nov 16, 2015
Author Contributor

A subclass is always False in a boolean expression, even if that pointer is NULL. But I just found out there was a signature missing in api.py. With that declaration I no longer get that error.

But that being said, I saw the error before when trying to call MagickSetFormat but I'm not seeing that error anymore. I'll let you know if it comes back.

@roelandschoukens
Copy link
Contributor Author

@roelandschoukens roelandschoukens commented Nov 24, 2015

I'm not getting the overflow error anymore. Probably it was caused by the missing definition of MagickDeleteOption in api.py.

@dahlia
Copy link
Collaborator

@dahlia dahlia commented Nov 25, 2015

The broken build was just fixed in upstream master. Could you rebase it on the current master?

@roelandschoukens
Copy link
Contributor Author

@roelandschoukens roelandschoukens commented Nov 26, 2015

OK, all done.
Coveralls is still showing a slight decrease, but that is unavoidable as a lot of the added code is only executed on Windows.

@@ -2385,6 +2386,14 @@ class Image(BaseImage):
.. versionadded:: 0.3.0
The ``resolution`` parameter.
.. versionadded:: 4.2.0

This comment has been minimized.

@dahlia

dahlia Nov 26, 2015
Collaborator

It has to be 0.4.2.

.. versionadded:: 4.2.0
The ``depth`` parameter.
.. versionchanged:: 4.2.0

This comment has been minimized.

@dahlia

dahlia Nov 26, 2015
Collaborator

It also has to be 0.4.2.

@roelandschoukens
Copy link
Contributor Author

@roelandschoukens roelandschoukens commented Nov 26, 2015

OK, that's fixed.

@dahlia
Copy link
Collaborator

@dahlia dahlia commented Nov 27, 2015

CI build was broken due to its code style:

$ flake8 .
./wand/api.py:678:39: E128 continuation line under-indented for visual indent
./wand/api.py:682:39: E128 continuation line under-indented for visual indent
@@ -650,6 +674,14 @@ class AffineMatrix(ctypes.Structure):
ctypes.c_uint]
library.MagickSetSize.restype = ctypes.c_int

library.MagickSetDepth.argtypes = [ctypes.c_void_p,
ctypes.c_uint]

This comment has been minimized.

@dahlia

dahlia Nov 27, 2015
Collaborator

./wand/api.py:678:39: E128 continuation line under-indented for visual indent
library.MagickSetDepth.restype = ctypes.c_int

library.MagickSetFormat.argtypes = [ctypes.c_void_p,
ctypes.c_char_p]

This comment has been minimized.

@dahlia

dahlia Nov 27, 2015
Collaborator

./wand/api.py:682:39: E128 continuation line under-indented for visual indent
@roelandschoukens
Copy link
Contributor Author

@roelandschoukens roelandschoukens commented Nov 27, 2015

Yes, I missed those two, sorry about that. The CI build is now passing.

dahlia added a commit that referenced this pull request Nov 29, 2015
Fix using wand on Windows, and allow loading of binary blobs
@dahlia dahlia merged commit 4714581 into emcconville:master Nov 29, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 85.089%
Details
@dahlia
Copy link
Collaborator

@dahlia dahlia commented Nov 29, 2015

Merged! Thanks for your effort. 😄

@roelandschoukens
Copy link
Contributor Author

@roelandschoukens roelandschoukens commented Nov 29, 2015

No problem, thank you for your time reviewing it.

@dahlia
Copy link
Collaborator

@dahlia dahlia commented Nov 29, 2015

FYI it was included in 0.4.2 release.
On Mon, Nov 30, 2015 at 5:35 AM roelandschoukens notifications@github.com
wrote:

No problem, thank you for your time reviewing it.


Reply to this email directly or view it on GitHub
#261 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants