-
Notifications
You must be signed in to change notification settings - Fork 23
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
CompatHelper: bump compat for "ColorTypes" to "0.10" #128
CompatHelper: bump compat for "ColorTypes" to "0.10" #128
Conversation
As you know, ColorTypes v0.10 and Colors v0.12 are coupled. Therefore, PR #127 or PR #128 should not be merged separately. ColorVectorSpace is not compatible with ColorTypes v0.10 "now" (cf. JuliaGraphics/ColorTypes.jl#160) I will not make a decision on whether to keep or drop the support for ColorTypes v0.8 and v0.9 using FYI, the issue #124 is independent of the ColorTypes update. |
Out of curiosity, what |
I don't think this is the same problem with JuliaImages/ImageTransformations.jl#89, i.e. the obvious type piracy. |
Sorry that I was confusing. For the If we keep two copies of P.S. I did that because I can't find a way to get the version of |
Originally posted by @timholy in JuliaImages/ImageTransformations.jl#89 Have you overlooked "If this is really an issue"? |
If I understood your words correctly, you were saying something like this: # in ColorVectorSpace
if COLORTYPES_VERSION < v"0.10"
Base.isless(a::AbstractGray, b::AbstractGray) = isless(gray(a), gray(b))
end But the issue is that we don't know the version of |
I think it's better to clarify what "you" call "precompilation issue". If it is really a problem, what happens when ColorTypes goes downgraded? |
Ideally, we want If there're two definitions, then julia would complain about incremental compilation. I believe that's the reason you say "ColorVectorSpace is not compatible with ColorTypes v0.10". A feasible solution is: remove the So we need to find a way to not throwing incremental compilation warning, and to still keep compatibility to old if !is_this_isless_method_defined_in_ColorTypes()
Base.isless(a::AbstractGray, b::AbstractGray) = isless(gray(a), gray(b))
end But this is actually a runtime check; correct me if I understand it incorrectly, during precompilation, this line might be evaluated before Even it's evaluated after the loading of So one solution is to move this patch line to Precompilation is still some kind of black box to me, so please correct me if I misunderstand the problem. I'm not good at English neither, I hope I've stated my problem clearly... |
There are certainly elements I don't understand but let's try to open the box a bit. The key is that it's really just ordinary Julia but it's performing "incremental compilation": it switches into a mode where, as it's parsing its way through a The key feature that determines precompile safety is whether decisions made when it does this logging apply to any new session. So, for example, if I had a top-level construct if isdefined(Main, :usecuda) && Main.usecuda
f(x) = ... some cuda implementation
else
f(x) = ... some CPU implementation
end this would not be precompile-safe because the outcome depends upon whether my current session has defined a variable
Not possible if you've said Since precompile files require an exact version match of all package dependencies, the outcome of the particular test you're wondering about is statically-decidable. So it doesn't present a threat to precompilation. I haven't tested, but you should be able to use |
Should also add: the role of the |
This pull request changes the compat entry for the
ColorTypes
package from0.8, 0.9
to0.8, 0.9, 0.10
.This keeps the compat entries for earlier versions.
Note: I have not tested your package with this new compat entry. It is your responsibility to make sure that your package tests pass before you merge this pull request.