Add support for Julia v1.0 #282

tlnagy · 2018-04-04T19:34:59Z

This finishes the Nullable{T} ==> Union{T, Nothing} transition started in #279. I decided that it wasn't worth using the Nullables.jl because it's a dead-end archive package and we might as well transition to the new representation of uninitialized values.

8dd674d is a temporary hack because LOAD_CACHE_PATH was deleted in v0.7. I only did this to get tests to pass locally on both v0.6 and v0.7 and I want to make sure CI passes. I'll revert this commit once an actual solution is found (see JuliaLang/Compat.jl#527) before merging

codecov-io · 2018-04-04T21:01:17Z

Codecov Report

Merging #282 into master will increase coverage by 0.03%.
The diff coverage is 45.16%.

@@            Coverage Diff             @@
##           master     #282      +/-   ##
==========================================
+ Coverage   34.69%   34.73%   +0.03%     
==========================================
  Files          17       17              
  Lines        2908     2908              
==========================================
+ Hits         1009     1010       +1     
+ Misses       1899     1898       -1

Impacted Files	Coverage Δ
src/Compose.jl	`6.09% <ø> (ø)`	⬆️
src/table.jl	`0% <ø> (ø)`	⬆️
src/cairo_backends.jl	`32.36% <0%> (+0.16%)`	⬆️
src/measure.jl	`36.36% <0%> (ø)`	⬆️
src/batch.jl	`9.58% <100%> (ø)`	⬆️
src/pgf_backend.jl	`40.96% <58.33%> (ø)`	⬆️
src/container.jl	`58.22% <75%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a65bc4...4161200. Read the comment docs.

bjarthur · 2018-04-28T14:32:10Z

do you want to try to finish this before we tag?

tlnagy · 2018-04-29T00:10:42Z

The primary hang up is I'm waiting for JuliaGraphics/Cairo.jl#224 to be resolved. I think we should go ahead and tag. 0.7 support can wait.

tlnagy · 2018-04-29T18:16:46Z

It looks like tests are failing partially due to isinstalled breaking on 0.7

On 0.7

julia> include("src/Compose.jl")

## omitted errors

julia> Compose.isinstalled("Fontconfig")
true

On 0.6

julia> include("src/Compose.jl")
Compose

julia> Compose.isinstalled("Fontconfig")
false

Fontconfig isn't installed in either environment so something else must've broken on 0.7

tlnagy · 2018-04-29T18:22:30Z

It looks specifically that

Compose.jl/src/Compose.jl

Lines 39 to 46 in bda8068

    
           ver == nothing && try 
        
               # Assume the version is new enough if the package is in LOAD_PATH 
        
               ex = Expr(:import, Symbol(pkg)) 
        
               @eval $ex 
        
               return true 
        
           catch 
        
               return false 
        
           end

the @eval throws an ArgumentError on v0.6 and not on v0.7 so the catch clause isn't triggered on v0.7.

EDIT: Not sure why this is happening to asked Discourse

vtjnash · 2018-07-10T20:12:27Z

The isinstalled logic should be replaced by Requires.jl (#226). Would be great to have this finished!

tlnagy · 2018-07-29T22:50:11Z

On @vtjnash's suggestion I started trying to get Requires.jl working, but I ran into some problems with Pkg.resolve since Compose does not have Cairo in its REQUIRE file. See JuliaPackaging/Requires.jl#52 for details.

tlnagy · 2018-08-17T01:04:21Z

Thanks to @RalphAS's help and with another round of cleanup, all tests pass on Julia v0.7 (NOTE: This is not backwards compatible due to the iteration protocol changes!). There are a couple silent deprecations that cause the build to crash on v1.0, which I have not yet fixed. Couple things remaining:

Fix silent 0.7 deprecations
Fix documentation building on v1.0 (looks like this is still in flux ATM)
Tag a release prior to merging this PR

tlnagy · 2018-08-17T02:07:59Z

This PR now successfully passes tests on Julia v1.0

tlnagy · 2018-08-17T21:20:18Z

@bjarthur and @Mattriks, I think this is basically ready to go. All tests are passing and docs are building. We can get started on getting Gadfly up to speed.

bjarthur · 2018-08-18T14:20:06Z

src/cairo_backends.jl

@@ -369,8 +372,8 @@ end
 function push_vector_properties(img::Image, idx::Int)
    save_property_state(img)
    for (propertytype, property) in img.vector_properties
-        isnull(property) && continue
-        primitives = get(property).primitives
+        (property === nothing) && continue


the parens aren't necessary here, though they don't hurt

bjarthur · 2018-08-18T14:27:41Z

src/cairo_backends.jl

-    y = Array{Float64}(1)
-    ccall((:cairo_get_current_point, Cairo._jl_libcairo), Void,
-          (Ptr{Void}, Ptr{Float64}, Ptr{Float64}), img.ctx.ptr, x, y)
+    x = Array{Float64}(undef, 1)


do you know what the thinking is behind julia 1.0 requiring undef here? is there something else that could be supplied as the first argument?

I think the thinking is that we should be very clear that we're allocating uninitialized memory, i.e. that x contains junk values. Here's a thorough discussion: JuliaLang/julia#24595.

bjarthur · 2018-08-18T14:59:49Z

.travis.yml

+    - |
+        julia -e '
+            using Compose, Pkg
+            pkg"add Colors Documenter"


should Cairo be removed from docs/REQUIRE?

I forgot about docs/REQUIRE so I could just load everything that is there or just delete it. Thoughts?

loading everything that is there would be one less thing to remember to change if docs/REQUIRE were to change in future. the code to do that is already in master.

bjarthur · 2018-08-18T15:21:01Z

src/Compose.jl

@@ -204,6 +180,7 @@ try
    getfield(Compose, :Cairo) # throws if Cairo isn't being used
    show(io::IO, ::MIME"image/png", ctx::Context) =
        draw(PNG(io, default_graphic_width, default_graphic_height), ctx)


could these three lines be added to cairo_backends.jl? seems better to conditionally include them via Requires.jl instead of a try/catch.

Good catch. I'll move it.

bjarthur · 2018-08-18T15:29:53Z

src/cairo_backends.jl

+             :CairoPDFSurface, :CairoSVGSurface, :CairoImageSurface)
+    val = getfield(Cairo,name)
+    @eval const $name = $val
+end


what's the reason for the change here? does using Cairo: ... not work with Requires?

This is from @RalphAS's original approach that worked on both v0.6 and v0.7, but I think we can revert it to the simpler case since we're dropping support for v0.6 going forward.

i don't understand why using Cairo: ... doesn't work on both 0.6 and 0.7...

What was previously written should work exactly as intended as far back as 0.3, if not earlier.

Changing this back leads to warnings (maybe @RalphAS can help us understand why?)

julia> using Compose [ Info: Loading Cairo backend into Compose.jl ┌ Warning: Package Compose does not have Cairo in its dependencies: │ - If you have Compose checked out for development and have │ added Cairo as a dependency but haven't updated your primary │ environment's manifest file, try `Pkg.resolve()`. │ - Otherwise you may need to report an issue with Compose └ Loading Cairo into Compose from project dependency, future warnings for Compose are suppressed.

bjarthur · 2018-08-18T15:34:59Z

src/Compose.jl

+#global PDF
+PNG(::Any, args...) = error(@missing_cairo_error "PNG")
+PS(::Any, args...) = error(@missing_cairo_error "PS")
+PDF(::Any, args...) = error(@missing_cairo_error "PDF")


could these three lines be simplified to PNG = error... ? none of the input args are actually used.

I think I had tried that originally and I was getting illegal constant redefinitions.

bjarthur · 2018-08-18T15:52:09Z

src/Compose.jl

 include("svg.jl")
 include("pgf_backend.jl")

 # If available, pango and fontconfig are used to compute text extents and match
 # fonts. Otherwise a simplistic pure-julia fallback is used.

-if isinstalled("Fontconfig")
+include("fontfallback.jl")


to the extent that fontfallback.jl mimics pango well, we should really hype up the fact that Gadfly can generate SVG with pure julia. maybe add a comment in the Compose README or docs.

bjarthur · 2018-08-18T15:55:28Z

looks great! thanks for working on this.

we should probably wait to tag a final julia 0.6 release of compose until we can do so simultaneously for gadfly.

once this merges i'll run the scripts to see what other gadfly dependencies are still not working on julia 0.7.

bjarthur · 2018-08-19T17:58:55Z

REQUIRE

@@ -1,7 +1,7 @@
 julia 0.6


should change this to 0.7 if it's not backwards compatible due to the new iteration protocol

bjarthur · 2018-08-28T02:14:56Z

make sure to close #226 when this is merged

tlnagy · 2018-09-05T18:16:28Z

not sure whether the problem is in compose or gadfly. hard to track down without a stack trace.

Have you tried asking over on discourse? That's basically impossible to track down with so little info.

i wouldn't be adverse to merging the three relevant PRs and then advertising a gadfly beta on discourse for field testing before tagging a release for julia 0.7.

Agreed. While the regressions are worrisome, they aren't showstoppers. We need to at least have a beta at this point. I'm in favor of merging into master and then letting people take it for a spin.

bjarthur · 2018-09-07T11:46:57Z

the broadcast / Ref() warning is now fixed, and i believe all the regression errors are due to the change in Random. so... let's do this!

Mattriks · 2018-09-07T12:23:56Z

Note that there is a more general solution than wrapping things in Ref(). e.g. in Julia 0.7, the following results in the same error above:

using Measures
a = [1mm,2mm,4mm]
a .+ 1mm

but this will work after implementing:

Broadcast.broadcastable(x::T) where T<:Measure = Ref(x)

which makes things easier for general use.

this is a follow up to #279 temp hack for missing load_cache_path on 0.7 See JuliaLang/Compat.jl#527 for details replace more voids that slipped past the first round more deprecation fixes prelim work on adding optional runtime backend loading use Compat's @info so we don't break v0.6

This version works if callers of PNG etc. import Cairo themselves.

update ci and doc gen; drop support for julia v0.6 allow failures on nightly

drop Compat.jl and vestigial compat layers fixes for doc building and coverage using is required to load pkg_str macro fix for silly parenthesis error drop 0.6 support and simplify travis config

tlnagy · 2018-09-07T17:07:41Z

I just squashed and rebased. This ready to merge.

~~We definitely need to resolve #282 (comment) soon, but I'm okay with this getting merged and the fix for that being a separate PR.~~

@simonschoelly

@simonschoelly identified this problem in #282 (comment) where calling show would lead to a stackoverflow error due to infinite recursion. The solution is to explicitly invoke Base's abstract show function when the non-compact form is requested to prevent the runaway recursion.

tlnagy · 2018-09-07T17:42:29Z

b731974 fixes the problem identified by @simonschoelly so now printing works as expected in the noncompact/default case too.

bjarthur · 2018-09-07T17:49:59Z

@Mattriks care to submit a PR to Measures.jl with your solution? much better than mine!

tlnagy · 2018-09-07T17:57:47Z

AFAIK, this PR is ready to be merged. I've tried to address all points raised in the review.

tlnagy · 2018-09-07T22:06:55Z

Looks like there are no major objections so I'm going to go ahead and pull the trigger.

We should let 1.0 support stew on master for a week give or take and then tag a new release. This'll give people time to put it through its paces and make sure that everything is working well.

@Mattriks

This solution was identified by @Mattriks over in GiovineItalia/Compose.jl#282 (comment)

@Mattriks

This solution was identified by @Mattriks over in GiovineItalia/Compose.jl#282 (comment)

tlnagy closed this Apr 29, 2018

tlnagy reopened this Apr 29, 2018

bjarthur mentioned this pull request Jul 29, 2018

Julia 0.7.0-alpha is out GiovineItalia/Gadfly.jl#1158

Closed

tlnagy mentioned this pull request Jul 29, 2018

Problem with loading optional dependencies JuliaPackaging/Requires.jl#52

Closed

tlnagy force-pushed the tn/more-v07-fixes branch from 6add91b to 4e20db6 Compare July 29, 2018 22:44

tlnagy mentioned this pull request Aug 16, 2018

Accommodate Requires for Julia v0.7 #289

Merged

tlnagy force-pushed the tn/more-v07-fixes branch from d7a53e6 to 8858adb Compare August 16, 2018 23:53

tlnagy changed the title ~~[WIP] More v0.7 fixes~~ Add support for Julia v1.0 Aug 17, 2018

tlnagy requested a review from bjarthur August 17, 2018 02:08

tlnagy force-pushed the tn/more-v07-fixes branch from 8a0c1ba to 37a9be5 Compare August 17, 2018 20:38

bjarthur reviewed Aug 18, 2018

View reviewed changes

bjarthur reviewed Aug 19, 2018

View reviewed changes

Mattriks mentioned this pull request Sep 5, 2018

Draw a circle and a text produce strange result (black line) #292

Closed

tlnagy and others added 5 commits September 7, 2018 10:02

fix up use of Requires for Julia v0.7

999a688

This version works if callers of PNG etc. import Cairo themselves.

more fixes for 0.7

0a051ac

update ci and doc gen; drop support for julia v0.6 allow failures on nightly

update show compact logic to work on Julia v1.0

2c2c0c5

more updates for v1.0

909f22f

drop Compat.jl and vestigial compat layers fixes for doc building and coverage using is required to load pkg_str macro fix for silly parenthesis error drop 0.6 support and simplify travis config

tlnagy force-pushed the tn/more-v07-fixes branch from a9cfdad to 909f22f Compare September 7, 2018 17:03

tlnagy added 2 commits September 7, 2018 10:50

load PNG show functions conditionally when Cairo is imported

658a980

fix incorrect setting of uninitialized keywords

a2c07d2

tlnagy merged commit 69be7b7 into master Sep 7, 2018

tlnagy mentioned this pull request Sep 7, 2018

Julia 1.0.0 support? GiovineItalia/Gadfly.jl#1185

Closed

tlnagy added a commit to JuliaGraphics/Measures.jl that referenced this pull request Sep 7, 2018

add support for measure broadcasting

f6ae785

This solution was identified by @Mattriks over in GiovineItalia/Compose.jl#282 (comment)

tlnagy mentioned this pull request Sep 7, 2018

Unbreak broadcast support JuliaGraphics/Measures.jl#18

Merged

tlnagy added a commit to JuliaGraphics/Measures.jl that referenced this pull request Sep 7, 2018

add support for measure broadcasting

6d29436

This solution was identified by @Mattriks over in GiovineItalia/Compose.jl#282 (comment)

tlnagy deleted the tn/more-v07-fixes branch September 7, 2018 23:24

tlnagy mentioned this pull request Sep 8, 2018

Fixes for 0.7 GiovineItalia/Hexagons.jl#10

Merged

bjarthur mentioned this pull request Sep 8, 2018

support julia 1.0 GiovineItalia/Gadfly.jl#1189

Merged

tlnagy mentioned this pull request Sep 8, 2018

use Requires #226

Closed

This was referenced Sep 27, 2018

splitting out cairo backend #250

Closed

Static compilation fails on Compose.jl #221

Closed

bjarthur mentioned this pull request Oct 17, 2018

Precompilation causes Compose to not notice if Cairo is later installed #139

Closed

This pull request was closed.

Add support for Julia v1.0 #282

Add support for Julia v1.0 #282

Conversation

tlnagy commented Apr 4, 2018

codecov-io commented Apr 4, 2018 • edited Loading

Codecov Report

bjarthur commented Apr 28, 2018

tlnagy commented Apr 29, 2018

tlnagy commented Apr 29, 2018

tlnagy commented Apr 29, 2018 • edited Loading

vtjnash commented Jul 10, 2018

tlnagy commented Jul 29, 2018

tlnagy commented Aug 17, 2018 • edited Loading

tlnagy commented Aug 17, 2018

tlnagy commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjarthur Aug 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjarthur commented Aug 18, 2018

Choose a reason for hiding this comment

bjarthur commented Aug 28, 2018

tlnagy commented Sep 5, 2018

bjarthur commented Sep 7, 2018

Mattriks commented Sep 7, 2018

tlnagy commented Sep 7, 2018 • edited Loading

tlnagy commented Sep 7, 2018

bjarthur commented Sep 7, 2018

tlnagy commented Sep 7, 2018 • edited Loading

tlnagy commented Sep 7, 2018

codecov-io commented Apr 4, 2018 •

edited

Loading

tlnagy commented Apr 29, 2018 •

edited

Loading

tlnagy commented Aug 17, 2018 •

edited

Loading

bjarthur Aug 19, 2018 •

edited

Loading

tlnagy commented Sep 7, 2018 •

edited

Loading

tlnagy commented Sep 7, 2018 •

edited

Loading