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

Building ImageMagick #7

Closed
SimonDanisch opened this issue Sep 26, 2015 · 47 comments
Closed

Building ImageMagick #7

SimonDanisch opened this issue Sep 26, 2015 · 47 comments

Comments

@SimonDanisch
Copy link
Member

Hi all,
I want to finally have a clean build for ImageMagick.
There seem to be a lot of issues when building ImageMagick.
It seems to me as if the current build system relies on a lot of uncertainties,.
E.g.:

  • using __init__, in an automatically generated include. Depending on where included and what Julia version you have, __init__ will be called at different points, which makes it hard to reason about
  • onload hooks are not guaranteed to get executed. Since there is no documentation its pretty opaque to me, but seems they don't get executed when the library is already present. But e.g. the variable ENV["MAGICK_CODER_MODULE_PATH"] should also be set even if ImageMagick is already present, as some of my experiments show. So putting this into onload is asking for trouble.
  • why and when exactly ccall((:MagickQueryConfigureOption, libwand), Ptr{UInt8}, (Ptr{UInt8},), "LIB_VERSION_NUMBER") segfaults on OSX wasn't really found out?! Build segfaults on OS X JuliaImages/Images.jl#188
  • I'm not sure why the version check is in a Module. Is this some hack for __init__ ?

From what I gathered we need these:

  • always set ENV["MAGICK_CODER_MODULE_PATH"] and ENV["MAGICK_CODER_MODULE_PATH"] before calling any function from ImageMagick
  • it seems osx needs also the path to be set!?
  • libversion is only needed for sRGB vs RGB
  • MagickWandGenesis and the ENV variables must be called before getting the version
  • MagickWandGenesis only needs to get called one time after the build?! And on OSX every time the module loads!? I don't know :D

Then there is also this segfault with ImageMagick: JuliaLang/julia#13318
Because of all these issues, I started looking into polishing DevIL.jl a little more, as it is more lightweight and easier to setup compared to ImageMagick.
There I have problems with the Linux and OSX build. Any help would be appreciated:
https://travis-ci.org/JuliaGL/DevIL.jl
(Tests are successful, because they're still empty - builds are not.)
Here is an overview of DevIL's features: http://openil.sourceforge.net/features.php
It seems pretty capable and easy to integrate into FileIO.
Since I now allow for multiple IO libraries for the same formats, its also straight forward to have multiple libraries as a back up for some format.

Sorry for any misunderstandings. Everything is a little opaque to me and I don't have lots of time to do this :(

Best,
Simon

From the issues on Images.jl I conclude that this concerns: @timholy @lucasb-eyer @staticfloat @rsrock @jwmerrill

@SimonDanisch
Copy link
Member Author

Whoops forgot to include the PR dealing with this:
#6

@SimonDanisch
Copy link
Member Author

For DevIL its: JuliaGL/DevIL.jl#1

@lucasb-eyer
Copy link

I can only add useful info concerning two points:

  1. MagickWandGenesis is a "wand-library init function" which you should always call once in the beginning of a process. See an example. We can see that it calls MagickCoreGenesis which sets up signal handlers and calls a boatload of other genesis functions, a lot of which initialize globals.
  2. I think you have a typo about the env variables, because you have the same twice. This SO answer might be of use about the env-variables required for IM, also on OSX.

@SimonDanisch
Copy link
Member Author

Thanks a lot!
This probably explains, why it segfaults in dump.c, as on windows ccall((:MagickWandGenesis,libwand), Void, ()) only gets called at precompilation.
Which is then wrong?

@lucasb-eyer
Copy link

Only at precompilation sounds wrong, yes.

@timholy
Copy link
Member

timholy commented Sep 26, 2015

Images has finally figured out how to do this correctly (took me long enough...):
https://github.com/timholy/Images.jl/blob/ecd35f185912489a94e57f22852d8be974b6a9dd/deps/build.jl#L18

It generates a deps.jl file which, on my system, is this:

# This is an auto-generated file; do not edit                                                                                        

# Pre-hooks                                                                                                                          

# Macro to load a library                                                                                                            
macro checked_lib(libname, path)                                                                                                     
    ((VERSION >= v"0.4.0-dev+3844" ? Base.Libdl.dlopen_e : Base.dlopen_e)(path) == C_NULL) && error("Unable to load \n\n$libname ($path)\n\nPlease re-run Pkg.build(package), and restart Julia.")                                                                        
    quote const $(esc(libname)) = $path end                                                                                          
end                                                                                                                                  

# Load dependencies
@checked_lib libwand "/usr/lib/x86_64-linux-gnu/libMagickWand.so"

# Load-hooks
ccall((:MagickWandGenesis,libwand), Void, ())

@timholy
Copy link
Member

timholy commented Sep 26, 2015

See also https://github.com/timholy/Images.jl/blob/ecd35f185912489a94e57f22852d8be974b6a9dd/src/ioformats/libmagickwand.jl#L26-L35. My versioninfo.jl file contains:

const libversion = v"6.7.7"

@SimonDanisch
Copy link
Member Author

Isn't that exactly the issue, because MagickWandGenesis is only evaluated once while precompiling?
I tried the build.jl from Images master, which fails for me on windows...Debugging it right now.

@timholy
Copy link
Member

timholy commented Sep 26, 2015

Oh, crap. See JuliaImages/Images.jl@bf4b6bb
I'd better move it inside an __init__.

@timholy
Copy link
Member

timholy commented Sep 26, 2015

@SimonDanisch
Copy link
Member Author

I'm running into this:
JuliaImages/Images.jl#181
from what I read, the solution wasn't really solved in build.jl, but by manually installing imagemagick?
Fun fact, it worked for me the last couple of times, this just started recently. Not really sure what's going on, since I tried different versions of the build script.

@SimonDanisch
Copy link
Member Author

Aha! ENV["MAGICK_CODER_MODULE_PATH"] needs to be set before ANYTHING. Which is not the case for all the build scripts I tried. I mean it looks like it, but still didn't work. I look into what needs to be done on the build script side.

@timholy
Copy link
Member

timholy commented Sep 26, 2015

Responding to your bullets at the top:

Depending on where included and what Julia version you have, init will be called at different points, which makes it hard to reason about

I don't understand this point; __init__ gets called on module initialization, right? It's kind of the last thing to run before loading the module is done.

onload hooks are not guaranteed to get executed

Right, they just get written to the deps.jl file. But if you include the deps.jl file into your source...

why and when exactly ccall((:MagickQueryConfigureOption, libwand), Ptr{UInt8}, (Ptr{UInt8},), "LIB_VERSION_NUMBER") segfaults on OSX wasn't really found out?!

Shouldn't happen anymore. These lines seem to solve the problem:
https://github.com/timholy/Images.jl/blob/ecd35f185912489a94e57f22852d8be974b6a9dd/deps/build.jl#L92-L95

I'm not sure why the version check is in a Module. Is this some hack for init ?

Not sure, but I bet I didn't want to risk borking an __init__ function in whatever code called include("build.jl").

I'm running into this: JuliaImages/Images.jl#181

Which aspect? It's a pretty multifaceted issue.

@timholy
Copy link
Member

timholy commented Sep 26, 2015

Yay, glad you figured it out!

@SimonDanisch
Copy link
Member Author

onload hooks are not guaranteed to get executed

I meant, they're not guaranteed to be included into the deps.jl file.

@SimonDanisch
Copy link
Member Author

Not sure, but I bet I didn't want to risk borking an init function in whatever code called

that's exactly my point about __init__ being a little bit hard to reason about.

@SimonDanisch
Copy link
Member Author

Now that I got ImageMagick working on windows, calling MagickWandGenesis doesn't solve the segfault from JuliaLang/julia#13318 :(

@timholy
Copy link
Member

timholy commented Sep 26, 2015

I meant, they're not guaranteed to be included into the deps.jl file.

https://github.com/JuliaLang/BinDeps.jl/blob/b0d298380106c6dc67bfdcddd56333b7f4863a61/src/dependencies.jl#L783
https://github.com/JuliaLang/BinDeps.jl/blob/b0d298380106c6dc67bfdcddd56333b7f4863a61/src/dependencies.jl#L822

If it's not happening, I'd call that a BinDeps bug.

Now that I got ImageMagick working on windows, calling MagickWandGenesis doesn't solve the segfault from JuliaLang/julia#13318 :(

Hmm. Does it work if you check out the teh/genesis branch of Images?

@timholy
Copy link
Member

timholy commented Sep 26, 2015

(Note that branch passed AppVeyor)

@SimonDanisch
Copy link
Member Author

Interesting.
I only tried the minimal example with the addition of ENV and the genesis.

@SimonDanisch
Copy link
Member Author

But not a big surprise, since:

 @unix_only context("Reading from a stream (issue #312)") do
        fn = joinpath(workdir, "2by2.png")
        io = open(fn)
        img = Images.imread(io, Images.ImageMagick)
        close(io)
        @fact isa(img, Images.Image) --> true
    end

@timholy
Copy link
Member

timholy commented Sep 26, 2015

git blame discovered this: JuliaImages/Images.jl#324

@SimonDanisch
Copy link
Member Author

Any ideas what I'm missing?
I tried setting the ENV at all thinkable places, but this is the only way it works:

ENV["MAGICK_CONFIGURE_PATH"] = "C:\\Users\\Sim\\.julia\\v0.4\\ImageMagick\\deps\\usr\\lib\\x64"
ENV["MAGICK_CODER_MODULE_PATH"] = "C:\\Users\\Sim\\.julia\\v0.4\\ImageMagick\\deps\\usr\\lib\\x64"
using ImageMagick
load("test.png")

Places I tried:
In deps.jl and while building all over the place, in the __init__, in the module body of ImagMagick, in VersionCheck and before the loading of the image.
In the crucial places, it always seems to be set correctly.
I'm not sure what that means.
Is there a peculiarity about setting ENV, like it's only defined for some process, so it must be the first statement of the process that actually executes load("test.png")?

@SimonDanisch
Copy link
Member Author

Ah silly me... I should have thought more about why the test is actually disabled.
So I can remove that from the checklist and wait for WinRPM :)

@SimonDanisch
Copy link
Member Author

concerning the ENV, I'll look into Images.jl a bit more, since it seems to pass on appveyor.

@SimonDanisch
Copy link
Member Author

Okay, so RegistryKeyLookupFailedCoderModulesPath' is due to the precompilation as figured out by @tkelman. As I precompile ImageMagick, it's failing for me.
This should be fixable differently right? I would really like to precompile ImageMagick :D

@timholy
Copy link
Member

timholy commented Sep 26, 2015

Yes, Images is precompilable, so it should work. The main trick is doing all the initialization inside the __init__, but I think you're doing that now?

Link to @tkelman's comment? I'm not sure what you're referring to.

@timholy
Copy link
Member

timholy commented Sep 26, 2015

In case it's not obvious, AppVeyor is precompiling Images.

@timholy
Copy link
Member

timholy commented Sep 26, 2015

Can you set ENV inside __init__?

@lucasb-eyer
Copy link

I'll abuse this thread to share with you what I just found out about: the MAGICK_DEBUG env variable which you can set to some of these values and might help your debugging.

@SimonDanisch
Copy link
Member Author

In case it's not obvious, AppVeyor is precompiling Images.

Really, how do you see that? I can't see anything in the logs, and https://github.com/timholy/Images.jl/blob/teh/genesis/src/Images.jl#L1 quite clearly disables it.

I've tried to set ENV wherever possible and __init__ was one of these places.
Thanks @lucasb-eyer I will see what that gives me.

@SimonDanisch
Copy link
Member Author

that's the commit by @tkelman
JuliaImages/Images.jl@e60ad4a

@timholy
Copy link
Member

timholy commented Sep 26, 2015

Oh, you're right. I keep forgetting, sorry.

@SimonDanisch
Copy link
Member Author

Okay here's the log from debug for setting the ENV before using ImageMagick and without :
https://gist.github.com/SimonDanisch/d9ba11ccd5c112bbe4e2

I need to get some coffee before I look into this^^

@timholy
Copy link
Member

timholy commented Sep 26, 2015

Gotta run, but one idea:

  • get a simple example working
  • put the whole example in an __init__ and verify it's precompilable
  • start moving things out of the __init__ until it stops working

@SimonDanisch
Copy link
Member Author

Oh what?!
MagickWandGenesis is actually the culprit it seems.
If I call it, I get the error, without it, it passes all tests...

@SimonDanisch
Copy link
Member Author

And without precompile, it doesn't matter if I call MagickWandGenesis.
This is all inside the __init__ function, so one would think precompilation shouldn't make a difference.
@tkelman do actually know whats up, or did you just deactivate precompile and it started working?

@tkelman
Copy link
Contributor

tkelman commented Sep 26, 2015

Check the Images PRs. The windows binaries that Images has been using do something with the registry that is not precompilable. If you're doing a new package here, you should try using the WinRPM build of imagemagick which is built differently.

@staticfloat
Copy link
Contributor

@SimonDanisch I'm having a little trouble following the conversation, but if you have questions about BinDeps or Homebrew or onload hooks etc, feel free to send them my way.

@SimonDanisch
Copy link
Member Author

@staticfloat thanks a lot =)
I do have some questions.
I tried to get ImageMagick working via WinRPM.
It downloads a lot of stuff and then tries to load libMagickWand-Q16-7.dll in the end, which seem to fail.
(By the way the error message "Can't satisfy dependency" is not very helpfull)
So I found out that loading the dll fails, with the most likely cause being that it has unsatisfied dependencies.
Legend says, you were the last to try out WinRPM and ImageMagick successfully!?

@SimonDanisch
Copy link
Member Author

Little mix up here, I meant @ihnorton, sorry about that.

@SimonDanisch
Copy link
Member Author

@staticfloat I'll still bug you with Homebrew and BinDeps question ;)
But at least for ImageMagick these seem to work fine at the moment!
DevIL needs some more attention, but it's not that high in the priority list right now.

SimonDanisch added a commit to JuliaImages/Images.jl that referenced this issue Sep 27, 2015
This was only for ImageMagick, see JuliaIO/ImageMagick.jl#7
@SimonDanisch
Copy link
Member Author

Okay that's what I mean with onload/preload being not included:
https://ci.appveyor.com/project/SimonDanisch/imagemagick-jl/build/job/1tr40ekc8uka5cyt
with this build script
https://github.com/JuliaIO/ImageMagick.jl/blob/master/deps/build.jl

@staticfloat is this expected behavior? If yes we can't put important statements into these hooks.

@SimonDanisch
Copy link
Member Author

Okay I think I know whats up. It gets accidentally installed by Images.jl. But should it really leave out the hooks in that case?

@staticfloat
Copy link
Contributor

It looks like what's happening is that, since Images has a CORE_RL_wand_.DLL file which satisfies this module's needs (and that directory is mysteriously on the library search path; are you using Images anywhere?) that library is being picked up by the implicit SystemPaths bindeps provider (note here how it is always searched last), and which doesn't have an onload/preload hook associated with it. I'm not sure how you would register a hook for a SystemPaths thing; I guess something like the following:

provides( SystemPaths, "name.dll", libname, onload = initfunc )

@timholy
Copy link
Member

timholy commented Oct 29, 2015

I think this is fixed in #14, and appears to be the same as #12. OK to close?

@SimonDanisch
Copy link
Member Author

Seems reasonable :)

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

No branches or pull requests

5 participants