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.

Sign up for GitHub

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

General build and init fixes and improvements #92

Closed
wants to merge 5 commits into from
Closed

General build and init fixes and improvements #92

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a8c74e1
More resilient osx build fix
Allardvm Jun 3, 2017
5e53a36
More consistent fix to support libraries in the system path
Allardvm Jun 3, 2017
4c76d75
Readme now uses Homebrew.jl interface to fix borked installations
Allardvm Jun 3, 2017
ee8e36f
Made fix compatible with precompilation + update libversion on init
Allardvm Jun 3, 2017
dd35780
Avoid global const in __init__
Allardvm Jun 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,14 @@ some of your workarounds might interfere with the new approach. You can reset yo

```julia
using Homebrew
run(`brew remove imagemagick@6`)
run(`brew prune`)
Homebrew.rm("imagemagick@6")
Homebrew.brew(`prune`)
Pkg.build("ImageMagick")
```

You may also find [debugging
Homebrew](https://github.com/JuliaLang/Homebrew.jl/wiki/Debugging-Homebrew.jl)
useful.
useful.

Finally, an alternative to ImageMagick on OS X is
[QuartzImageIO](https://github.com/JuliaIO/QuartzImageIO.jl).
Expand Down
78 changes: 19 additions & 59 deletions deps/build.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,31 +15,17 @@ libwand = library_dependency("libwand", aliases = aliases)

mpath = get(ENV, "MAGICK_HOME", "") # If MAGICK_HOME is defined, add to library search path
if !isempty(mpath)
init_fun =
"""
function init_deps()
ccall((:MagickWandGenesis, libwand), Void, ())
end
"""

provides(Binaries, mpath, libwand, preload = init_fun, onload = "init_deps()")
provides(Binaries, joinpath(mpath, "lib"), libwand, preload = init_fun, onload = "init_deps()")
provides(Binaries, mpath, libwand)
provides(Binaries, joinpath(mpath, "lib"), libwand)
end


if is_linux()
init_fun =
"""
function init_deps()
ccall((:MagickWandGenesis, libwand), Void, ())
end
"""

provides(AptGet, "libmagickwand4", libwand, preload = init_fun, onload = "init_deps()")
provides(AptGet, "libmagickwand5", libwand, preload = init_fun, onload = "init_deps()")
provides(AptGet, "libmagickwand-6.q16-2", libwand, preload = init_fun, onload = "init_deps()")
provides(Pacman, "imagemagick", libwand, preload = init_fun, onload = "init_deps()")
provides(Yum, "ImageMagick", libwand, preload = init_fun, onload = "init_deps()")
provides(AptGet, "libmagickwand4", libwand)
provides(AptGet, "libmagickwand5", libwand)
provides(AptGet, "libmagickwand-6.q16-2", libwand)
provides(Pacman, "imagemagick", libwand)
provides(Yum, "ImageMagick", libwand)
end


Expand All @@ -61,12 +47,10 @@ if is_windows()
magick_url = "$(magick_base)/$(magick_exe)"
magick_libdir = joinpath(BinDeps.libdir(libwand), OS_ARCH)
innounp_url = "https://bintray.com/artifact/download/julialang/generic/innounp.exe"
init_fun =
preloads =
"""
function init_deps()
ENV["MAGICK_CONFIGURE_PATH"] = \"$(escape_string(magick_libdir))\"
ENV["MAGICK_CODER_MODULE_PATH"] = \"$(escape_string(magick_libdir))\"
end
init_envs["MAGICK_CONFIGURE_PATH"] = \"$(escape_string(magick_libdir))\"
init_envs["MAGICK_CODER_MODULE_PATH"] = \"$(escape_string(magick_libdir))\"
"""

provides(BuildProcess,
Expand All @@ -81,50 +65,26 @@ if is_windows()
`innounp.exe -q -y -b -e -x -d$(magick_libdir) $(magick_exe)`
end
end),
libwand, os = :Windows, unpacked_dir = magick_libdir, preload = init_fun,
onload = "init_deps()")
libwand, os = :Windows, unpacked_dir = magick_libdir, preload = preloads)
end


if is_apple()
using Homebrew
imagemagick_prefix = Homebrew.prefix("staticfloat/juliadeps/imagemagick@6")
init_fun =
homebrew_prefix = Homebrew.prefix()
preloads =
"""
function init_deps()
ENV["MAGICK_CONFIGURE_PATH"] = joinpath("$(imagemagick_prefix)",
"lib", "ImageMagick", "config-Q16")
ENV["MAGICK_CODER_MODULE_PATH"] = joinpath("$(imagemagick_prefix)",
"lib", "ImageMagick", "modules-Q16", "coders")
ENV["PATH"] = joinpath("$(imagemagick_prefix)", "bin") * ":" * ENV["PATH"]

ccall((:MagickWandGenesis,libwand), Void, ())
end
init_envs["MAGICK_CONFIGURE_PATH"] = joinpath("$(homebrew_prefix)",
"lib", "ImageMagick", "config-Q16")
init_envs["MAGICK_CODER_MODULE_PATH"] = joinpath("$(homebrew_prefix)",
"lib", "ImageMagick", "modules-Q16", "coders")
init_envs["PATH"] = joinpath("$(homebrew_prefix)", "bin") * ":" * ENV["PATH"]
"""
provides(Homebrew.HB, "staticfloat/juliadeps/imagemagick@6", libwand, os = :Darwin,
preload = init_fun, onload = "init_deps()")
provides(Homebrew.HB, "homebrew/core/imagemagick@6", libwand, os = :Darwin, preload = preloads)
end


@BinDeps.install Dict([(:libwand, :libwand)])

# Hack-fix for issue #12
# Check to see whether init_deps is present, and if not add it
if isempty(search(readstring(joinpath(dirname(@__FILE__),"deps.jl")), "init_deps"))
open("deps.jl", "a") do io
write(io, init_fun)
end
end

module CheckVersion
include("deps.jl")
p = ccall((:MagickQueryConfigureOption, libwand), Ptr{UInt8}, (Ptr{UInt8}, ),
"LIB_VERSION_NUMBER")
vstr = string("v\"", join(split(unsafe_string(p), ',')[1:3], '.'), "\"")
open(joinpath(dirname(@__FILE__), "versioninfo.jl"), "w") do file
write(file, "const libversion = $vstr\n")
end
end


is_windows() && pop!(BinDeps.defaults)
14 changes: 8 additions & 6 deletions src/libmagickwand.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,27 +20,29 @@ export MagickWand,

# Find the library
depsfile = joinpath(dirname(@__FILE__), "..", "deps", "deps.jl")
versionfile = joinpath(dirname(@__FILE__), "..", "deps", "versioninfo.jl")

init_envs = Dict{String,String}()
if isfile(depsfile)
include(depsfile)
else
error("ImageMagick not properly installed. Please run Pkg.build(\"ImageMagick\") then restart Julia.") # now that this is decoupled from images, should this be an error?
end
if isfile(versionfile)
include(versionfile)
end

const have_imagemagick = isdefined(:libwand)

# Initialize the library
function __init__()
init_deps()
for (key, value) in init_envs
ENV[key] = value
end
!have_imagemagick && warn("ImageMagick utilities not found. Install for more file format support.")
ccall((:MagickWandGenesis, libwand), Void, ())
p = ccall((:MagickQueryConfigureOption, libwand), Ptr{UInt8}, (Ptr{UInt8}, ),
"LIB_VERSION_NUMBER")
global const libversion = VersionNumber(join(split(unsafe_string(p), ',')[1:3], '.'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

global const?
does this really need to be done at run-time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's global in the module's namespace, so only accessible with ImageMagick.libversion—not with libversion. I would expect users to interpret it as reflecting the version of the library that's currently loaded, which suggests that we should define it at run-time.

Nevertheless, the check at https://github.com/JuliaIO/ImageMagick.jl/blob/master/src/ImageMagick.jl#L155 is still precompiled, so that still won't reflect the real library version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is precompile friendly i.e. global const within __init__

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JuliaLang/julia#12010 indeed suggests that it's better not to use this. We could define it at compile-time, but I don't think it'll be clear to users that libversion might not reflect the actual version of the library.

We could also follow PyCall's solution: https://github.com/JuliaPy/PyCall.jl/blob/1d755f27fd440a43b9a792919fee0531495754e0/src/pyinit.jl#L50, where the version number seems to be defined at compile time, but checked at runtime.

Another alternative is to declare libversion = Ref{VersionNumber}() and update its value during __init__. This would mean that users have to call ImageMagick.libversion[] to get the version number.

Or maybe you have some other alternative?

Copy link
Contributor Author

@Allardvm Allardvm Jun 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make the last case a bit more user-friendly with:

const _libversion = Ref{VersionNumber}()

function __init__()
    _libversion[] = ...
end

libversion() = _libversion[]

Copy link
Member

@musm musm Jun 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

global const Ref +1
const _libversion = Ref{VersionNumber}()

end



# Constants
# Storage types
const CHARPIXEL = 1
Expand Down