Updates for Julia 1.0 #101

ararslan · 2018-08-15T22:21:54Z

Now info and warn are only imported and extended from Base when they're defined in Base. Travis and AppVeyor have been updated to handle versions 0.6 through 1.0.

Supersedes #100.

codecov · 2018-08-15T22:33:12Z

Codecov Report

Merging #101 into master will decrease coverage by 0.29%.
The diff coverage is 83.33%.

@@            Coverage Diff            @@
##           master     #101     +/-   ##
=========================================
- Coverage   97.53%   97.24%   -0.3%     
=========================================
  Files          11       11             
  Lines         284      290      +6     
=========================================
+ Hits          277      282      +5     
- Misses          7        8      +1

Impacted Files	Coverage Δ
src/Memento.jl	`100% <ø> (ø)`	⬆️
src/records.jl	`94.44% <83.33%> (-5.56%)`	⬇️
src/stdlib.jl	`25% <0%> (+12.5%)`	⬆️

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 f491d13...4199c14. Read the comment docs.

ararslan · 2018-08-15T22:41:35Z

Wow, the userimage tests segfault on 0.7 and 1.0. 😬

ararslan · 2018-08-15T22:46:38Z

Since we're using TestSetExtensions in the tests here, we might need ssfrr/TestSetExtensions.jl#7 before this will work properly. 😕

omus · 2018-08-16T12:36:14Z

.travis.yml

+        VERSION >= v"0.7.0-DEV.5183" || cd(Pkg.dir("Memento"))
+        Pkg.add("Documenter")
+        include(joinpath("docs", "make.jl"))
+      '


I sometimes combine these lines so I don't require the double VERSION checks

I personally prefer having coverage submission and documentation deployment as separate steps, even if it means duplicating some logic until we drop 0.6.

omus · 2018-08-16T12:37:13Z

.travis/test.sh

+    julia --color=yes -e '
+        if VERSION >= v"0.7.0-DEV.3382"
+            using Libdl
+        end


I'd make this a one-liner

omus · 2018-08-16T12:39:28Z

src/records.jl

+if isdefined(Base, :iterate)
+    function Base.iterate(rec::T, state=0) where T <: Record
+        state >= fieldcount(T) && return nothing
+        new_state = state + 1


Just reuse state?

state += 1

I was just reusing what was there in the Base.next definition. I agree that's nicer.

omus · 2018-08-16T12:40:25Z

src/records.jl

+        state >= fieldcount(T) && return nothing
+        new_state = state + 1
+        return (fieldname(T, new_state) => get(getfield(rec, new_state)), new_state)
+    end


These two iterate methods have the same body. Combine with T <: Union{Record, AttributeRecord}

Close but not quite: Note that AttributeRecord needs to call get on the result of getfield.

omus · 2018-08-16T12:42:27Z

test/handlers.jl

@@ -106,7 +106,7 @@

        @testset "Sample Usage w/ File" begin
            filename = tempname()
-            info("Path to log file: $filename")
+            Compat.@info("Path to log file: $filename")


Personal preference but I prefer doing using Compat: @info so I don't have to have Compat.* peppered throughout the code. Will make dropping 0.6 support later easier.

iamed2

The userimg tests segfaulting might actually indicate issues with the library. The root cause should be identified and documented.

@rofinn liked the indication of progress that TestSetExtensions gives. He may prefer removing the global testset to return the old behaviour.

iamed2 · 2018-08-16T23:01:45Z

test/io.jl

@@ -9,7 +9,7 @@
            "fubar" => 50
        )
        roller_prefix = tempname()
-        info("Path to roller_prefix: $roller_prefix")
+        Compat.@info("Path to roller_prefix: $roller_prefix")


Doesn't have to be Compat. anymore since you imported it in test/runtests.jl right?

Oh whoops, I missed that one.

iamed2 · 2018-08-16T23:04:11Z

test/runtests.jl

-Base.next(cr::ConstRecord, state) = next(_props(cr), state)
-Base.done(cr::ConstRecord, state) = done(_props(cr), state)
+if isdefined(Base, :iterate)
+    Base.iterate(cr::ConstRecord, state=1) = iterate(_props(cr), state)


You can just do state... and pass it right through

and pass it right through

How do you mean?

Base.iterate(cr::ConstRecord, state...) = iterate(_props(cr), state...)

I see. Is that preferable?

It doesn't assume iteration state, so yeah. Performance doesn't matter for this but I think there are splatting optimizations for small tuples.

ararslan · 2018-08-16T23:29:22Z

The userimg tests segfaulting might actually indicate issues with the library. The root cause should be identified and documented.

It's unrelated to Memento:

$ cat thingo.jl 
VERSION >= v"0.7.0-DEV.3382" && using Libdl
BINDIR = VERSION < v"0.7.0-DEV.3073" ? Base.JULIA_HOME : Sys.BINDIR
LIB_PATH = abspath(BINDIR, Base.LIBDIR)
sysimg_lib = joinpath(LIB_PATH, "julia", "sys.$(Libdl.dlext)")
userimg_o = "userimg.o"
userimg_lib = "userimg.$(Libdl.dlext)"
run(`$(Base.julia_cmd()) --output-o $userimg_o --sysimage $sysimg_lib --startup-file=no -e "println(1)"`)
$ julia-0.6 thingo.jl
1
$ ls
thingo.jl  userimg.o
$ julia-0.7 thingo.jl

signal (11): Segmentation fault
in expression starting at no file:0
uv_write2 at /buildworker/worker/package_linux64/build/deps/srccache/libuv-ed3700c849289ed01fe04273a7bf865340b2bd7e/src/unix/stream.c:1432
uv_write at /buildworker/worker/package_linux64/build/deps/srccache/libuv-ed3700c849289ed01fe04273a7bf865340b2bd7e/src/unix/stream.c:1524
jl_uv_write at /buildworker/worker/package_linux64/build/src/jl_uv.c:454
uv_write_async at ./stream.jl:810
uv_write at ./stream.jl:778
unsafe_write at ./stream.jl:834
macro expansion at ./gcutils.jl:87 [inlined]
write at ./strings/io.jl:152 [inlined]
show at ./show.jl:568 [inlined]
print at ./strings/io.jl:31
print at ./strings/io.jl:42
println at ./strings/io.jl:69
unknown function (ip: 0x7f77483c2507)
jl_fptr_trampoline at /buildworker/worker/package_linux64/build/src/gf.c:1829
jl_apply_generic at /buildworker/worker/package_linux64/build/src/gf.c:2182
println at ./coreio.jl:4
unknown function (ip: 0x7f77483c2453)
jl_fptr_trampoline at /buildworker/worker/package_linux64/build/src/gf.c:1829
jl_apply_generic at /buildworker/worker/package_linux64/build/src/gf.c:2182
do_call at /buildworker/worker/package_linux64/build/src/interpreter.c:324
eval_value at /buildworker/worker/package_linux64/build/src/interpreter.c:428
eval_stmt_value at /buildworker/worker/package_linux64/build/src/interpreter.c:363 [inlined]
eval_body at /buildworker/worker/package_linux64/build/src/interpreter.c:686
jl_interpret_toplevel_thunk_callback at /buildworker/worker/package_linux64/build/src/interpreter.c:799
unknown function (ip: 0xfffffffffffffffe)
unknown function (ip: 0x7f774b6c5b8f)
unknown function (ip: 0xffffffffffffffff)
jl_interpret_toplevel_thunk at /buildworker/worker/package_linux64/build/src/interpreter.c:808
jl_toplevel_eval_flex at /buildworker/worker/package_linux64/build/src/toplevel.c:831
jl_toplevel_eval_in at /buildworker/worker/package_linux64/build/src/builtins.c:633
eval at ./boot.jl:319
jl_fptr_trampoline at /buildworker/worker/package_linux64/build/src/gf.c:1829
jl_apply_generic at /buildworker/worker/package_linux64/build/src/gf.c:2182
macro expansion at ./logging.jl:317 [inlined]
exec_options at ./client.jl:229
_start at ./client.jl:432
jl_fptr_trampoline at /buildworker/worker/package_linux64/build/src/gf.c:1829
jl_apply_generic at /buildworker/worker/package_linux64/build/src/gf.c:2182
jl_apply at /buildworker/worker/package_linux64/build/ui/../src/julia.h:1538 [inlined]
true_main at /buildworker/worker/package_linux64/build/ui/repl.c:112
main at /buildworker/worker/package_linux64/build/ui/repl.c:233
__libc_start_main at /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
_start at /home/alex/Projects/past-julia/0.7/bin/julia (unknown line)
Allocations: 726978 (Pool: 726579; Big: 399); GC: 0
ERROR: LoadError: failed process: Process(`/home/alex/Projects/past-julia/0.7/bin/julia -Cnative -J/home/alex/Projects/past-julia/0.7/lib/julia/sys.so --compile=yes --depwarn=yes --output-o userimg.o --sysimage /home/alex/Projects/past-julia/0.7/lib/julia/sys.so --startup-file=no -e 'println(1)'`, ProcessSignaled(11)) [0]
Stacktrace:
 [1] error(::String, ::Base.Process, ::String, ::Int64, ::String) at ./error.jl:42
 [2] pipeline_error at ./process.jl:712 [inlined]
 [3] #run#509(::Bool, ::Function, ::Cmd) at ./process.jl:670
 [4] run(::Cmd) at ./process.jl:668
 [5] top-level scope at none:0
 [6] include at ./boot.jl:317 [inlined]
 [7] include_relative(::Module, ::String) at ./loading.jl:1038
 [8] include(::Module, ::String) at ./sysimg.jl:29
 [9] exec_options(::Base.JLOptions) at ./client.jl:239
 [10] _start() at ./client.jl:432
in expression starting at /home/alex/Projects/test/thingo.jl:7

Apparently as of JuliaLang/julia#24410, the --output-o option is to be considered unstable and was hidden from the --help output. It seems that it's since bitrotted.

iamed2 · 2018-08-16T23:34:22Z

We'll need to change it to use build_sysimg I guess then

ararslan · 2018-08-16T23:53:29Z

build_sysimg has also bitrotted. See JuliaLang/julia#27629. The recommendation is to use PackageCompiler, but that too does not support 0.7 or newer.

iamed2 · 2018-08-16T23:55:48Z

So the documented function doesn't work? https://docs.julialang.org/en/latest/devdocs/sysimg/#Building-the-Julia-system-image-1

That's...worrying

ararslan · 2018-08-17T00:04:48Z

I didn't realize we had it documented. I think once JuliaLang/julia#21849 landed, rebuilding the sysimage became pretty uncommon, so no one paid much attention to the machinery in place that worked around the initial limitation.

rofinn

I'll note that I was considering using @includetests from TestSetExtensions for running a subset of the tests (e.g., benchmarks). The @includetests macro displays each file file being tested.

rofinn · 2018-08-17T05:36:30Z

.travis/test.sh

@@ -2,20 +2,61 @@

 set -ev

+# Set up the build environment across Julia versions
+# This portion is the same as the default Travis test script


Maybe include a link to the default Travis test script?

https://github.com/travis-ci/travis-build/blob/master/lib/travis/build/script/julia.rb

rofinn · 2018-08-17T05:39:36Z

.travis/test.sh

+if [[ -a .git/shallow ]]; then
+    git fetch --unshallow
+fi
+if [[ -f Project.toml || -f JuliaProject.toml ]]; then


Is this condition entirely necessary while we don't have a Project.toml? Unrelated, but is there a reason there are two *Project.toml files now?

JuliaProject.toml is for when you have some other system that requires Project.toml. If it exists it'll be used instead.

No the condition isn't necessary but it seemed simpler to me to duplicate the Travis logic wholesale and not have to remember to update things here when we do add a Project.toml file.

Now `info` and `warn` are only imported and extended from Base when they're defined in Base. Further, Travis and AppVeyor have been updated to handle versions 0.6 through 1.0.

It currently does not (and likely cannot) work on Julia versions 0.7 and 1.0.

The --output-o option is hidden as of 0.7 and no longer appears to work properly: what works in 0.6 segfaults in 0.7.

Apparently `Base.reinit_stdio()` is required to avoid libuv segfaulting

ararslan · 2018-08-17T20:16:53Z

Well, I fixed the segfault but the userimage tests still don't work. ¯\_(ツ)_/¯

iamed2 · 2018-08-18T02:23:14Z

Seems like the script that runs it just needs to tell Pkg3 where to find Memento?

ararslan · 2018-08-18T21:36:04Z

Maybe, but I'm not sure; in testing I've been using the stdlib Test rather than Memento, which should be easier for Julia to find, but it still errors saying it can't find it. Kristoffer was helping me diagnose the other day but we didn't come to a conclusion.

ararslan requested a review from omus August 15, 2018 22:21

ararslan force-pushed the aa/1.0 branch from 0658271 to 49b93b6 Compare August 15, 2018 22:37

ararslan force-pushed the aa/1.0 branch 3 times, most recently from 9b9018a to a3026dc Compare August 16, 2018 04:17

omus reviewed Aug 16, 2018

View reviewed changes

ararslan force-pushed the aa/1.0 branch from a3026dc to 9302e76 Compare August 16, 2018 18:37

omus approved these changes Aug 16, 2018

View reviewed changes

iamed2 requested changes Aug 16, 2018

View reviewed changes

ararslan force-pushed the aa/1.0 branch from 9302e76 to a507417 Compare August 16, 2018 23:57

rofinn reviewed Aug 17, 2018

View reviewed changes

ararslan added 5 commits August 17, 2018 11:57

Updates for Julia 1.0

698062e

Now `info` and `warn` are only imported and extended from Base when they're defined in Base. Further, Travis and AppVeyor have been updated to handle versions 0.6 through 1.0.

Use the new iteration protocol when defined

b5164c1

Remove test dependency on TestSetExtensions

be2140e

It currently does not (and likely cannot) work on Julia versions 0.7 and 1.0.

Use at-async instead of at-schedule

61b7546

Allow Travis failures for 0.7/1.0 userimage tests

5d94dfb

The --output-o option is hidden as of 0.7 and no longer appears to work properly: what works in 0.6 segfaults in 0.7.

ararslan force-pushed the aa/1.0 branch from a507417 to 5d94dfb Compare August 17, 2018 18:57

Try to fix userimage test segfaults on 0.7 and 1.0

4199c14

Apparently `Base.reinit_stdio()` is required to avoid libuv segfaulting

iamed2 mentioned this pull request Aug 24, 2018

userimg test segfaults on 1.0 #103

Closed

iamed2 merged commit 10d93e9 into master Aug 24, 2018

ararslan deleted the aa/1.0 branch August 24, 2018 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates for Julia 1.0 #101

Updates for Julia 1.0 #101

ararslan commented Aug 15, 2018 •

edited

Loading

codecov bot commented Aug 15, 2018 •

edited

Loading

ararslan commented Aug 15, 2018

ararslan commented Aug 15, 2018

omus Aug 16, 2018

ararslan Aug 16, 2018

omus Aug 16, 2018

omus Aug 16, 2018

ararslan Aug 16, 2018

omus Aug 16, 2018

ararslan Aug 16, 2018

omus Aug 16, 2018

omus Aug 16, 2018

iamed2 left a comment

iamed2 Aug 16, 2018

ararslan Aug 16, 2018

iamed2 Aug 16, 2018

ararslan Aug 16, 2018

iamed2 Aug 16, 2018

ararslan Aug 16, 2018

iamed2 Aug 16, 2018

ararslan commented Aug 16, 2018

iamed2 commented Aug 16, 2018

ararslan commented Aug 16, 2018

iamed2 commented Aug 16, 2018

ararslan commented Aug 17, 2018

rofinn left a comment

rofinn Aug 17, 2018

omus Aug 17, 2018

rofinn Aug 17, 2018 •

edited

Loading

iamed2 Aug 17, 2018

ararslan Aug 17, 2018

ararslan commented Aug 17, 2018

iamed2 commented Aug 18, 2018

ararslan commented Aug 18, 2018

Updates for Julia 1.0 #101

Updates for Julia 1.0 #101

Conversation

ararslan commented Aug 15, 2018 • edited Loading

codecov bot commented Aug 15, 2018 • edited Loading

Codecov Report

ararslan commented Aug 15, 2018

ararslan commented Aug 15, 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

iamed2 left a 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

ararslan commented Aug 16, 2018

iamed2 commented Aug 16, 2018

ararslan commented Aug 16, 2018

iamed2 commented Aug 16, 2018

ararslan commented Aug 17, 2018

rofinn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rofinn Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Aug 17, 2018

iamed2 commented Aug 18, 2018

ararslan commented Aug 18, 2018

ararslan commented Aug 15, 2018 •

edited

Loading

codecov bot commented Aug 15, 2018 •

edited

Loading

rofinn Aug 17, 2018 •

edited

Loading