build with binaryprovider #121
Conversation
|
Hm windows test failures are not present on master.. guess I build ImageMagick with less support for some operations.... Not really sure what's up with Linux! Seems to work locally and fails for the same tests as on windows... |
|
So seems like the problems lays with writing a png to a stream... |
|
Ok, something is really weird with the tests :D They all pass, but actually trying to just save/load pngs and any other file doesn't work in isolation :D So I think the compile with delegate theory actually holds... |
|
pretty clear now: I mean, that's kind of expected ;) We just seem to have written the tests in a way, that they still pass. Maybe related to the mimewriter changes in FileIO (since I definitely don't have another jpg loader library installed)! |
|
Aw, almost... I can't reproduce the failure on travis with my local linux machine. On all PCs I have available this new version passes all tests! If people can try this out on their machine and see if they can reproduce the failure, that'd help a lot :) |
|
Seems like |
REQUIRE
Outdated
| FileIO | ||
| @osx Homebrew | ||
| BinDeps | ||
| BinaryProvider |
There was a problem hiding this comment.
Given the comment at the top of deps/build.jl this needs a minimum version number
deps/build.jl
Outdated
|
|
||
| is_windows() && pop!(BinDeps.defaults) | ||
| # Write out a deps.jl file that will contain mappings for our products | ||
| write_deps_file(joinpath(@__DIR__, "deps.jl"), products) |
There was a problem hiding this comment.
Will all the dependencies as well as ImageMagick itself try to create the same deps.jl file? Is that bad?
There was a problem hiding this comment.
I was wondering about this as well, but since it works and I'm not sure how to do it differently I was leaving it like that for now.. @staticfloat ?!
There was a problem hiding this comment.
Presumably you could have them each write their own foo_deps.jl file and then have one deps.jl that includes them all?
There was a problem hiding this comment.
I don't think that's necessary, so I'd just follow @staticfloat recommendation, if he has any ;)
There was a problem hiding this comment.
Having multiple deps.jl files is only necessary if you want to manually dlopen() files from the other projects; right now you're only going to have access to variables that define the libwand variable; you won't get variables that point to the location of zlib, libpng, etc... I don't think you care about those, so this is fine, but this user experience is high on our list of things to improve. :)
src/libmagickwand.jl
Outdated
| MagickRelinquishMemory[] = loadsym(:MagickRelinquishMemory) | ||
| MagickQueryConfigureOptions[] = loadsym(:MagickQueryConfigureOptions) | ||
| MagickQueryConfigureOption[] = loadsym(:MagickQueryConfigureOption) | ||
| libversion() = v"6.9.10-4" |
There was a problem hiding this comment.
Can this be provided by the builder script? Otherwise I worry we will forget to update it and it will get out-of-sync.
There was a problem hiding this comment.
Sure, there isn't any infrastructure in place in binaryprovider right now, so I figured that any hacky workaround I do would be as bad as this;)
There was a problem hiding this comment.
As further background, the call to get the version from ImageMagick directly segfaulted, which I was too tired to debug especially since all other calls worked :D So I just assumed it came naturally from the different build process ...
There was a problem hiding this comment.
Hmm, we've seen that before and it seems to correlate with problematic initialization. Can you post what you tried somewhere?
There was a problem hiding this comment.
Investigating it right now! Might have found a problematic flag in the configure ;) Seems like there is a flag to make ImageMagick find all configuration files itself instead of having them hardcoded at compile time.
| @@ -25,117 +25,15 @@ else | |||
| error("ImageMagick not properly installed. Please run Pkg.build(\"ImageMagick\") then restart Julia.") | |||
| end | |||
|
|
|||
| const libmagick = Ref{Ptr{Void}}() | |||
|
|
|||
| const MagickWandGenesis = Ref{Ptr{Void}}() | |||
There was a problem hiding this comment.
I'm pretty sure that we don't need those shenanigans anymore, now that the windows binaries are pretty much identical to the unix ones - instead of using this weird windows installer, that messes with the registry and loads everything dynamically from an xml file^^
And I've been mainly testing this on windows and appveyor passes, so this might just be the end to windows being crazy with ImageMagick :)
I'm testing to build the binaries with OpenMP disabled right now which might fix this issue :) |
|
This passes tests for me. |
|
Yeah, it seems like most linux distributions have GOMP_4 installed... Might be coming with a modern version of gcc, or so ;) But seems like a real problem on old / clean distributions like travis, so I hope the new binaries I'm compiling right now will fix it! |
deps/build.jl
Outdated
| # it's a bit faster to run the build in an anonymous module instead of | ||
| # starting a new julia process | ||
| m = Module(:__anon__) | ||
| eval(m, :(include($(joinpath(@__DIR__, elem))))) |
There was a problem hiding this comment.
You can use Main.include to prevent an UndefVarError: include not defined on 0.7.
And Core.eval will avoid a warning.
Both work on 0.6.
|
Looking close now! |
|
Oh boi, tests are turning green :) |
|
Weird, wasn't coverage improving before?! In any case, I mostly deleted code, so I don't really see how this can be relevant to this PR |
|
Hm, still can't query the options. Also, it doesn't give me any debug output, even though I see some logging calls in the C code I'm hitting... julia> ENV["MAGICK_DEBUG"] = "All"
"All"
julia> using ImageMagick
julia> option = "LIB_VERSION_NUMBER"
"LIB_VERSION_NUMBER"
julia> p = ccall((:MagickQueryConfigureOption, ImageMagick.libwand), Ptr{UInt8}, (Ptr{UInt8},), option)
Please submit a bug report with steps to reproduce this fault, and any error messages that follow (in their entirety). Thanks.
Exception: EXCEPTION_ACCESS_VIOLATION at 0x11310e64 -- MagickQueryConfigureOption at /workspace/srcdir/ImageMagick6\wand\magick-wand.c:424
while loading no file, in expression starting on line 0
MagickQueryConfigureOption at /workspace/srcdir/ImageMagick6\wand\magick-wand.c:424
anonymous at .\<missing> (unknown line)
jl_call_fptr_internal at /home/Administrator/buildbot/worker/package_win64/build/src/home/Administrator/buildbot/worker/package_win64/build/src\julia_internal.h:339 [inlined]
jl_call_method_internal at /home/Administrator/buildbot/worker/package_win64/build/src/home/Administrator/buildbot/worker/package_win64/build/src\julia_internal.h:358 [inlined]
jl_toplevel_eval_flex at /home/Administrator/buildbot/worker/package_win64/build/src/home/Administrator/buildbot/worker/package_win64/build/src\toplevel.c:589
jl_toplevel_eval_in at /home/Administrator/buildbot/worker/package_win64/build/src/home/Administrator/buildbot/worker/package_win64/build/src\builtins.c:496
eval at .\boot.jl:235
unknown function (ip: 0000000004B5E3A5)
jl_call_fptr_internal at /home/Administrator/buildbot/worker/package_win64/build/src/home/Administrator/buildbot/worker/package_win64/build/src\julia_internal.h:339 [inlined]
jl_call_method_internal at /home/Administrator/buildbot/worker/package_win64/build/src/home/Administrator/buildbot/worker/package_win64/build/src\julia_internal.h:358 [inlined]
jl_apply_generic at /home/Administrator/buildbot/worker/package_win64/build/src/home/Administrator/buildbot/worker/package_win64/build/src\gf.c:1926
eval_user_input at .\REPL.jl:66
unknown function (ip: 0000000004BF4EC5)
jl_call_fptr_internal at /home/Administrator/buildbot/worker/package_win64/build/src/home/Administrator/buildbot/worker/package_win64/build/src\julia_internal.h:339 [inlined]
jl_call_method_internal at /home/Administrator/buildbot/worker/package_win64/build/src/home/Administrator/buildbot/worker/package_win64/build/src\julia_internal.h:358 [inlined]
jl_apply_generic at /home/Administrator/buildbot/worker/package_win64/build/src/home/Administrator/buildbot/worker/package_win64/build/src\gf.c:1926
macro expansion at .\REPL.jl:97 [inlined]
#1 at .\event.jl:73
unknown function (ip: 00000000105527E3)
jl_call_fptr_internal at /home/Administrator/buildbot/worker/package_win64/build/src/home/Administrator/buildbot/worker/package_win64/build/src\julia_internal.h:339 [inlined]
jl_call_method_internal at /home/Administrator/buildbot/worker/package_win64/build/src/home/Administrator/buildbot/worker/package_win64/build/src\julia_internal.h:358 [inlined]
jl_apply_generic at /home/Administrator/buildbot/worker/package_win64/build/src/home/Administrator/buildbot/worker/package_win64/build/src\gf.c:1926
jl_apply at /home/Administrator/buildbot/worker/package_win64/build/src/home/Administrator/buildbot/worker/package_win64/build/src\julia.h:1424 [inlined]
start_task at /home/Administrator/buildbot/worker/package_win64/build/src/home/Administrator/buildbot/worker/package_win64/build/src\task.c:267
Allocations: 4461360 (Pool: 4459572; Big: 1788); GC: 7 |
|
It is sometimes a little strange. For example, the I do note that julia> ImageMagick.queryoptions("*")
3-element Array{String,1}:
"FEATURES"
"NAME"
"QuantumDepth"
julia> ImageMagick.queryoption("FEATURES")
"OpenMP "
julia> ImageMagick.queryoption("NAME")
"ImageMagick"
julia> ImageMagick.queryoption("QuantumDepth")
"16"So it's really just that diff --git a/src/libmagickwand.jl b/src/libmagickwand.jl
index 6b3c982..61b871d 100644
--- a/src/libmagickwand.jl
+++ b/src/libmagickwand.jl
@@ -543,5 +543,6 @@ end
# queries the value of a particular option
function queryoption(option::AbstractString)
p = ccall(MagickQueryConfigureOption[], Ptr{UInt8}, (Ptr{UInt8},), option)
+ p == C_NULL && error("option $option not found")
unsafe_string(p)
endto the current I guess if everyone is using the same binaries then it isn't too much of a concern. But I worry that once https://github.com/JuliaIO/ImageMagickBuilder/releases has multiple versions available, this is going to cause problems---we'll ask people to tell us what version they are running and the answer will be unreliable in terms of its usefulness. If nothing else, can you make sure the |
|
Hm, I'm surprised that this is working: julia> run(`convert.exe -version`)
Version: ImageMagick 6.9.10-4 Q16 x86_64 2018-07-09 https://www.imagemagick.org
Copyright: ┬® 1999-2018 ImageMagick Studio LLC
License: https://www.imagemagick.org/script/license.php
Features: Cipher DPC Modules
Delegates (built-in): gslib jng jpeg png ps tiff zlib |
|
This looks great, thanks! I just tested it locally and it works flawlessly. Can folks on other platforms give this a whirl? Pkg.checkout("ImageMagick") # hopefully this will fetch the updated list of available branches
Pkg.checkout("ImageMagick", "sd/binaryprovider") # this is the branch you actually want
Pkg.build("ImageMagick")
Pkg.test("ImageMagick") |
Flawless here as well :) |
|
|
|
Anything we still want to address before merging? |
|
I'm good with it. But are there any OSX testers out there? |
|
works for me on OS X |
|
Thanks to all testers. I'm personally satisfied if you are, merge at will @SimonDanisch (probably with a squash, or manually separate commits into logical chunks). |
SimonDanisch
force-pushed
the
sd/binaryprovider
branch
from
f52d4c1 to
a1ab825
Compare
|
removed the Images -> ImageCore commit and squashed the rest into one commit! |
|
Thanks a ton for doing this! Let's hope it ends the flakiness. |
|
That was a lot of closed issues, @SimonDanisch 🎆 |
|
Yeah, that was satisfying! :) Was quite a bit of work, but actually still much less work than I invested into trying to make BinDeps + that flaky windows installer work ;) Btw, a lot of thanks to @staticfloat & @RalphAS to fix windows + osx issues! |
|
Cool, seems like the configuration issue got fixed: |
No description provided.