-
Notifications
You must be signed in to change notification settings - Fork 37
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
General build and init fixes and improvements #92
Conversation
src/libmagickwand.jl
Outdated
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], '.')) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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__
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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[]
There was a problem hiding this comment.
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}()
Alright, I have used a For some reason, the tests don't pass when I call ENV["MAGICK_CONFIGURE_PATH"] = [my system's path]
ENV["MAGICK_CODER_MODULE_PATH"] = [my system's path]
using ImageMagick then the tests always pass, but setting the environment variables after |
Closing this in favor of #93. |
This PR provides a more resilient build process.
__init__()
to make sure it always reflects the library's version—even when the library is updated post-build.deps.jl
no longer defines aninit_deps()
function. The initialization of the library is handled in__init__()
. This function sets the environment variables using theinit_envs
dictionary that is set on-demand during the build process (to allow precompilation), and then callsccall((:MagickWandGenesis, libwand), Void, ())
. As far as I can see, the fix in Re-fixes issue #12, reintroduced due to 286138d #91 is no longer necessary with this setup.