Skip to content
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

Use BinaryProvider to install Cairo and Pango #292

Merged
merged 6 commits into from
Sep 6, 2019

Conversation

giordano
Copy link
Contributor

@giordano giordano commented Sep 2, 2019

This PR installs Cairo and Pango library using BinaryProvider.jl.

I think it's good to keep this open for some time to let users play with it. So, please do test this PR and report back any issue you face. To do this, just run

]add https://github.com/giordano/Cairo.jl.git#binary-builder

in Julia REPL. I recommend then to clean the directory Cairo/deps/ from build.log and deps.jl, and then you can run ]build Cairo.

Building these libraries has been a great stress-test for the new BinaryBuilder.jl framework that will land in Julia v1.3. A huge thanks to @staticfloat! 🐋 🎅 I'll probably update the URLs of some build.jl files later this week (I'll use the "official" ones that will be in https://github.com/JuliaBinaryWrappers), but the content of the downloaded tarballs should be the more or less the same.

When merged, this PR fixes #105, fixes #121, fixes #148, fixes #162, fixes #165, fixes #185, fixes #187, fixes #203, fixes #207, fixes #214, fixes #230, fixes #239, fixes #256, fixes #258, fixes #261, fixes #265, fixes #266, fixes #271, fixes #279, fixes #284, fixes #286, fixes #287. It supersedes #149, #196, #289.

@tknopp
Copy link
Contributor

tknopp commented Sep 2, 2019

So lets talk about how to get this in. I think one way would be to merge the Cairo changes but NOT making a release until Gtk.jl is ready. What do people think?

@tknopp
Copy link
Contributor

tknopp commented Sep 2, 2019

And thank you so much for doing this!

@SimonDanisch
Copy link
Member

Ubuntu 19.04:

Test Summary:     |
IOBuffer Rooting  | No tests
Test Summary:   | Pass  Total
Image Surface   |    7      7
Test Summary:   | Pass  Total
Conversions     |    5      5
Test Summary:   | Pass  Total
TexLexer        |    1      1
Test Summary:   | Pass  Total
Samples         |   33     33
Test Summary:   | Pass  Total
Bitmap Painting |    6      6
Test Summary:   | Pass  Total
Vector Surfaces |   12     12
Test Summary:   | Pass  Total
Bitmap Surfaces |    3      3
Test Summary:   | Pass  Total
Assert/Status   |    7      7
Test Summary:   | Pass  Total
reset_transform |    6      6
   Testing Cairo tests passed 

Will test Windows when I'm home ;)

@coveralls
Copy link

coveralls commented Sep 2, 2019

Coverage Status

Coverage increased (+0.05%) to 91.081% when pulling fd9c6a0 on giordano:binary-builder into ee4f16c on JuliaGraphics:master.

@giordano
Copy link
Contributor Author

giordano commented Sep 2, 2019

I can add scripts to do testing on Cirrus CI (for FreeBSD) and Drone CI (for Linux on ARM32 and ARM64). I did it on my fork and tests are passing everywhere. If someone is willing to activate this repository on those services, I can add the scripts also here.

@tknopp
Copy link
Contributor

tknopp commented Sep 2, 2019

@SimonDanisch: How can I test this locally? checkout Cairo#???

@giordano
Copy link
Contributor Author

giordano commented Sep 2, 2019

@tknopp this is on my fork, I'm not sure if this can be done easily within Julia. What I would do is to add a remote with URL "https://github.com/giordano/Cairo.jl.git" and then check-out binary-builder branch from there.

If anyone knows an easier way to do this (maybe all within Julia), please share 🙂

@SimonDanisch
Copy link
Member

@giordano I set them up (I think :D ) ;)

@codecov-io
Copy link

codecov-io commented Sep 2, 2019

Codecov Report

Merging #292 into master will increase coverage by 0.03%.
The diff coverage is 95.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #292      +/-   ##
==========================================
+ Coverage   92.12%   92.16%   +0.03%     
==========================================
  Files           1        1              
  Lines         432      434       +2     
==========================================
+ Hits          398      400       +2     
  Misses         34       34
Impacted Files Coverage Δ
src/Cairo.jl 92.16% <95.69%> (+0.03%) ⬆️

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 ee4f16c...fd9c6a0. Read the comment docs.

@SimonDanisch SimonDanisch reopened this Sep 2, 2019
@SimonDanisch
Copy link
Member

@tknopp I just did ]add https://github.com/giordano/Cairo.jl.git#binary-builder

@SimonDanisch
Copy link
Member

Also finally works in Nextjournal without any additional shenanigans:
https://nextjournal.com/a/LbT3bpaYHLuDMNQMrXziD?token=2HUEh46gr4vUgd3Xu2QrEq

@tknopp
Copy link
Contributor

tknopp commented Sep 2, 2019

@SimonDanisch: that's exactly what I was looking for :-)

@tknopp
Copy link
Contributor

tknopp commented Sep 2, 2019

Test woking here on OSX Mojave (10.14.6)!

@timholy
Copy link
Member

timholy commented Sep 2, 2019

Installation and tests pass without a hiccup on

Julia Version 1.2.1-pre.0
Commit 93929550b6* (2019-08-20 05:33 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-6.0.1 (ORCJIT, skylake)
Environment:
  JULIAFUNCDIR = /home/tim/juliafunc
  JULIA_CPU_THREADS = 2

So lets talk about how to get this in. I think one way would be to merge the Cairo changes but NOT making a release until Gtk.jl is ready. What do people think?

The new package manager makes this a breeze. We should place upper bounds on the Cairo version for all previous Gtk releases. Then anyone who has Gtk installed in their current environment won't get the new version of Cairo until Gtk makes a new release that allows the new Cairo. For project environments that don't have Gtk installed, they will use the new version of Cairo and thus immediately get the benefits. In other words, even a single user might be using both depending upon which project environment is active.

@tknopp
Copy link
Contributor

tknopp commented Sep 2, 2019

@timholy: yes thought about that as well. It requires of course to make releases of all direct and indirect dependencies of Cairo. So Winston has to be capped to the version where Gtk is capped ...

@tknopp
Copy link
Contributor

tknopp commented Sep 2, 2019

but I am fine with that.

@StefanKarpinski
Copy link
Contributor

Remember that compatibility of existing registered versions can be modified in the registry without needing to make any new releases! Just ask on #pkg-registration about how to do it although it’s pretty simple: just edit the registry files and make a PR.

@jkrumbiegel
Copy link

All tests succeeding on my system as well, Mojave 10.14.6 :)

@jonathanBieler
Copy link
Contributor

jonathanBieler commented Sep 2, 2019

Awesome, works on a Mojave 10.14.4 and Windows 10 machines. It's also pretty fast to install compared to homebrew.

@asinghvi17
Copy link

Works on Mojave 10.14.5 as well. Thanks a ton, @giordano!

@ghost
Copy link

ghost commented Sep 3, 2019

All tests passing on NixOS with Julia 1.0.3, that is about as picky of a distribution as one can have, so well done!

Test Summary:     |
IOBuffer Rooting  | No tests
Test Summary:   | Pass  Total
Image Surface   |    7      7
Test Summary:   | Pass  Total
Conversions     |    5      5
Test Summary:   | Pass  Total
TexLexer        |    1      1
Test Summary:   | Pass  Total
Samples         |   33     33
Test Summary:   | Pass  Total
Bitmap Painting |    6      6
Test Summary:   | Pass  Total
Vector Surfaces |   12     12
Test Summary:   | Pass  Total
Bitmap Surfaces |    3      3
Test Summary:   | Pass  Total
Assert/Status   |    7      7
Test Summary:   | Pass  Total
reset_transform |    6      6
   Testing Cairo tests passed
julia> versioninfo()
Julia Version 1.0.3
Commit 099e826241 (2018-12-18 01:34 UTC)
Platform Info:
  OS: Linux (x86_64-unknown-linux-gnu)
  CPU: Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-6.0.0 (ORCJIT, skylake)

@rfourquet
Copy link

All tests passing on NixOS with Julia 1.0.3, that is about as picky of a distribution as one can have

Same here with Julia 1.3 and 1.4 🎉 I only had little hope that it worked (until I saw @ninjin message)... BinaryProvider is a game changer for Nixos user :) thanks so much for your effort here!

@tknopp
Copy link
Contributor

tknopp commented Sep 3, 2019

yes absolutely, in those cases, I propose integrating all those functions into Cairo.jl, where they belong to.

@lobingera
Copy link
Contributor

@tknopp @asinghvi17 This takes an interesting turn. Adding the cairo_ft would mean, that we make FreeType.jl (or FreeTypeAbstractions.jl) a mandatory dependency (or pray for faster implementation of optional dependencies) - as we need the type correctly. btw: who had the great idea to move from _jl_libxxx to .libxxx directly?

@asinghvi17
Copy link

I believe that's just a function of going to BinaryProvider? It's really easy to patch this on the CairoMakie end, so it's not an issue (I imagine that's why they were there in the first place).

@tknopp
Copy link
Contributor

tknopp commented Sep 3, 2019

yes, I think those lib names should be standardized (by convention of BB).

@giordano
Copy link
Contributor Author

giordano commented Sep 3, 2019

The name of the variable can be easily changed here:

LibraryProduct(prefix, "libcairo", :libcairo),
if necessary

@asinghvi17
Copy link

I guess we could just alias it for backward compatibility...is it possible to throw a depwarn on use of a variable?

@SimonDanisch
Copy link
Member

I think it should be libcairo...
So i'd vote for:

const _jl_libcairo = libcairo
@deprecate_binding _jl_libcairo # or however that was done^^

CairoMakie should just make a PR to Cairo, to incorporate the newly wrapped functions ;)

@staticfloat
Copy link
Contributor

staticfloat commented Sep 5, 2019

Regarding library names, BB allows you to define your own mapping, but the semantic meaning behind a LibraryProduct() is becoming a little bit stricter; if you want to be able to access a library, you should define a LibraryProduct for it via LibraryProduct(["filename1", "filename2", ...], :variablename). Then, after the recipe has been built, BB will look inside the prefix for libraries matching any of the given filenames, and when it finds one, it will bind it to :variablename inside of the autogenerated JLL package (or deps.jl, if you're using BinaryProvider).

EDIT: Hit "submit" a little too quickly.

For JLL packages, each LibraryProduct listed will be dlopen()'ed, which means that libraries from other packages that want to link against the libraries in this package can find them; so in a sense, listing a LibraryProduct exports it such that it can be opened by other packages. If you don't list it, it may be loaded by one of your other LibraryProducts or not, it depends on the dependency graph of your package. So the best bet is, if another package requires a library from your package, then list it as a LibraryProduct.

For BinaryProvider, this doesn't matter, because all libraries get duplicated for every installation, and they sit next to eachother in the same folder, so they can find eachother easily.

@giordano giordano changed the title [WIP] Use BinaryProvider to install Cairo and Pango Use BinaryProvider to install Cairo and Pango Sep 5, 2019
@giordano
Copy link
Contributor Author

giordano commented Sep 5, 2019

This is no more WIP and can be merged whenever you want. Now I'm using the same binaries as those created by @staticfloat for #293.

The last change I pushed can cause some local conflicts if you had checked out my original version of the PR. My recommendation is to just remove all deps/build_*.jl files. I'm sorry if this will cause some annoyance to the brave users who tested this PR, but I think that having all build.jl files checked in gives us more control (it's makes it easier to understand why some library won't work) and it also makes building the package a bit faster, as these files are already there.

@tknopp
Copy link
Contributor

tknopp commented Sep 6, 2019

This is excellent work @giordano! I of course cannot comment on all the BB and BP stuff but the result is that it again works on all platforms.

I would merge this if there are no objections.

@timholy
Copy link
Member

timholy commented Sep 6, 2019

Yep. @tknopp, do you need help putting retroactive bounds on the Cairo version for Gtk? I recently submitted some docs on the registry format: JuliaLang/Pkg.jl#1349.

@tknopp
Copy link
Contributor

tknopp commented Sep 6, 2019

@timholy: It would be best if you do that. I personally would wait with the release but am fine that you do it, if you promise that things don't break ;-)

@tknopp tknopp merged commit a472c0c into JuliaGraphics:master Sep 6, 2019
@timholy
Copy link
Member

timholy commented Sep 6, 2019

I can't see how they would break, as long as people install what they want (e.g., ProfileView) rather than starting from the ground up (Cairo). And all they would get is a Pkg conflict, which once resolved would fix everything.

@tknopp
Copy link
Contributor

tknopp commented Sep 6, 2019

yes, I know Tim. Just wanted to express that this should be done by someone with some more experience.

@dietercastel
Copy link

Well that worked. Thanks for your efforts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment