Use Yggdrasil to host binary #58
Conversation
|
Now the build script downloads binaries from https://github.com/JuliaBinaryWrappers/Libtask_jll.jl/releases, I think this will fix #53 and #57. |
KDr2
force-pushed
the
prebuilt-binary
branch
2 times, most recently
from
de40af6 to
14531ff
Compare
|
I don't think this is the ideal solution yet since we still have to keep the In general, I think we could just remove the commented out or unneeded parts since they are still available in the git history anyways. In the build Github action the tagging part is not needed either anymore I guess? At least I think it caused problems in the last PR. I.e., I would suggest removing at least Maybe we could also switch from building binaries in every PR to just building binaries if one of the files on:
push:
paths:
- 'deps/Makefile'
- 'deps/libtask.c'
- 'deps/build_tarballs.jl'
- 'deps/build_dylib.sh'The Julia integration, however, won't be tested (Libtask.jl will still use the binaries served from Libtask_jll), so maybe it would actually make sense to move these files to Yggdrasil. Additionally, maybe the build CI should run with a newer Julia version? I also ran the script to generate the |
|
BTW the logic for picking the correct version depending on the Julia version could also be moved to |
|
You are right, I did some updates of this PR:
About the artifacts system and About where to place the source and build scripts (task.c Makefile, build_tarballs.jl, dylib_build.sh), I didn't find an ideal way either. No one puts code source to Yggdrasil, and a separate repo for it is also too heavy because we don't need one to release and host the binary lib now. |
|
About generating $ julia generate_buildjl.jl L/Libtask/build_tarballs.jl
[ Info: Build tarballs script: L/Libtask/build_tarballs.jl
[ Info: Repo name: JuliaBinaryWrappers/Libtask_jll.jl
Updating registry at `~/.julia/registries/General`
Updating git-repo `https://github.com/JuliaRegistries/General.git`
ERROR: LoadError: MethodError: no method matching registered_paths(::Pkg.Types.EnvCache, ::Base.UUID)
Closest candidates are:
registered_paths(::Pkg.Types.Context, ::Base.UUID) at /home/kdr2/Work/julia/julia/usr/share/julia/stdlib/v1.5/Pkg/src/Types.jl:1190
Stacktrace:
[1] top-level scope at /home/kdr2/Work/julia/Yggdrasil/generate_buildjl.jl:164
[2] include(::Function, ::Module, ::String) at ./Base.jl:381
[3] include(::Module, ::String) at ./Base.jl:369
[4] exec_options(::Base.JLOptions) at ./client.jl:289
[5] _start() at ./client.jl:491
in expression starting at /home/kdr2/Work/julia/Yggdrasil/generate_buildjl.jl:157On Julia 1.3: But we I think we should modify the generate |
It worked on Julia 1.3 for me (after fixing the problem with |
| name: TagBot | ||
| on: | ||
| schedule: | ||
| - cron: 0 * * * * |
There was a problem hiding this comment.
You still want to keep TagBot.yml since that will tag a new release when the PRs to the registry are merged. Maybe just change it to once per day (I think it's not needed to run it hourly):
- cron: '0 0 * * *'There was a problem hiding this comment.
Sorry, not intentionally to delete this, I confused it with the auto-tag job, updated in a new commit.
Sure, for the time being I guess it's easiest if we keep using the However, I think the version selection should not be moved to
So I think we should just add the following to src/Libtask.jl after loading the @static VERSION >= v"1.4"
const libtask = libtask_1_4
elseif VERSION >= v"1.3"
const libtask = libtask_1_3
elseif VERSION >= v"1.2"
const libtask = libtask_1_2
elseif VERSION >= v"1.1"
const libtask = libtask_1_1
else
const libtask = libtask_1_0
end |
Yes, most packages download source code from some external repository but it's very common to include patches in the subdirectory |
|
It has an issue putting the version selection to |
|
Now, I managed to generate So I decided to revert the commit and use my version. now:
Is this OK? |
Isn't the only check loading the libraries? Surely the incompatible libraries can't be used but would it even fail to open the libraries? The third reason for why I thought it might be preferable to not handle the selection in the |
Yeah, it checks if every lib file exists and try to load them by calling |
|
The errors are unrelated and explained in the Yggdrasil README:
After adding |
deps/build.jl
Outdated
| lib_versions = map(p -> p.libnames[1][9:end], products) | ||
| lib_path = Sys.iswindows() ? "bin" : "lib" | ||
| deps_source = """ | ||
| if isdefined((@static VERSION < v"0.7.0-DEV.484" ? current_module() : @__MODULE__), :Compat) | ||
| import Compat.Libdl | ||
| elseif VERSION >= v"0.7.0-DEV.3382" | ||
| import Libdl | ||
| end | ||
| function include_build_script(version_str, try_prev=false) | ||
| build_script_url = "https://github.com/TuringLang/Libtask.jl/releases/download/v$(version_str)/build_LibtaskDylib.v$(version_str).jl" | ||
| build_script = joinpath(@__DIR__, "tmp-build.jl") | ||
| build_script = try download(build_script_url, build_script) catch end | ||
| if build_script == nothing && try_prev # no such file | ||
| version_str = find_prev_tag("v$version_str") |> strip |> (x) -> lstrip(x, ['v']) | ||
| return include_build_script(version_str, false) | ||
| end | ||
| install_products_filter(build_script) | ||
| include(build_script) | ||
| const lib_versions = $(lib_versions) | ||
| proper_ver = filter(lib_versions) do v | ||
| endswith(v, "\$(VERSION.major)_\$(VERSION.minor)") | ||
| end | ||
| proper_ver = length(proper_ver) == 0 ? lib_versions[end] : proper_ver[1] | ||
| const libtask = joinpath(dirname(@__FILE__), "usr/$(lib_path)/libtask_\$(proper_ver).\$(Libdl.dlext)") | ||
| function get_version_str() | ||
| path = joinpath(@__DIR__, "../Project.toml") | ||
| version_reg = r"version\s*=\s*\"(.*)\"" | ||
| open(path) do file | ||
| lines = readlines(file) | ||
| for line in lines | ||
| m = match(version_reg, line) | ||
| if isa(m, RegexMatch) return m.captures[1] end | ||
| end | ||
| function check_deps() | ||
| global libtask | ||
| if !isfile(libtask) | ||
| error("\$(libtask) does not exist, Please re-run Pkg.build(\\"Libtask.jl\\"), and restart Julia.") | ||
| end | ||
| if Libdl.dlopen_e(libtask) in (C_NULL, nothing) | ||
| error("\$(libtask) cannot be opened, Please re-run Pkg.build(\\"Libtask.jl\\"), and restart Julia.") | ||
| end | ||
| end | ||
| version_str = get_version_str() |> strip |> (x) -> lstrip(x, ['v']) | ||
| include_build_script(version_str, true) | ||
| """ | ||
| write(joinpath(@__DIR__, "deps.jl"), deps_source) |
There was a problem hiding this comment.
I still think if possible we should not write deps.jl explicitly. For instance, if you compare this with the implementation of write_deps_file in https://github.com/JuliaPackaging/BinaryProvider.jl/blob/81f03dad25dfd76845f4fbc0eea1a383280f334c/src/Products.jl#L397-L507 which is usually used in builds.jl, you see that in the standard version there are many more checks and a much more complicated logic for deriving the correct paths on each OS. IMO if possible we shouldn't try to outsmart this implementation.
If we want a custom check_deps() function it might be better to just add a custom check in src/Libtask.jl after including deps/deps.jl and not running check_deps().
|
So I think the following approach could work:
In that way we don't mess with the generation of the deps.jl file (in particular not with paths) but still just check and use the version that we want to use. |
src/Libtask.jl
Outdated
|
|
||
| if Libdl.dlopen_e(libtask) in (C_NULL, nothing) | ||
| error("$(libtask) cannot be opened, Please re-run Pkg.build(\"Libtask.jl\"), and restart Julia.") | ||
| end |
There was a problem hiding this comment.
I suggest you move the checks to an __init__() method so that they are only executed once after the module has been loaded. See https://github.com/JuliaPackaging/BinaryProvider.jl/blob/master/test/LibFoo.jl/src/LibFoo.jl for an example setup.
BTW with the additional definitions of libtask inow it might might be easier to just check if the file does not exist, as shown in the example linked above, and remove the second branch, i.e., just execute everything if it didn't error.
|
I am sorry that this way doesn't work...
So we have to modify |
|
Yes due to line https://github.com/JuliaPackaging/BinaryProvider.jl/blob/55aeaea2652c1b43ba882c2f833196e99f10e2db/src/Products.jl#L418 IMO then the second best solution would be to filter the correct product before calling |
OK, they don't make a big difference to me, I will filter the products list before call |
| products_tmp = filter(products) do prod | ||
| endswith(prod.libnames[1], "$(VERSION.major)_$(VERSION.minor)") | ||
| end | ||
| length(products_tmp) == 0 && (products_tmp = [products[end]]) | ||
| products = products_tmp |
There was a problem hiding this comment.
Looks good, I guess we don't have to care about version jumps? In that case, something like
| products_tmp = filter(products) do prod | |
| endswith(prod.libnames[1], "$(VERSION.major)_$(VERSION.minor)") | |
| end | |
| length(products_tmp) == 0 && (products_tmp = [products[end]]) | |
| products = products_tmp | |
| lastminor = findlast(products) do p | |
| x = match(r"v(\d)_(\d)$", string(p.variable_name)) | |
| x !== nothing && VERSION.major >= parse(Int, x.captures[1]) && VERSION.minor >= parse(Int, x.captures[2]) | |
| end | |
| products = products[[lastminor]] |
would provide a the last compatible version.
src/Libtask.jl
Outdated
| elseif VERSION < v"1.2.9999" # [v1.2, v1.3) | ||
| const libtask = libtask_v1_2 | ||
| else # [v1.3, +) | ||
| const libtask = libtask_v1_3 |
There was a problem hiding this comment.
| const libtask = libtask_v1_3 | |
| const libtask = libtask_v1_3 |
|
Thank you @devmotion. |
No description provided.