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 Artifacts and JLL packages on Julia 1.3+ #293

Merged
merged 6 commits into from
Nov 28, 2019

Conversation

staticfloat
Copy link
Contributor

This is an example PR that showcases how, using the capabilities in Julia 1.3+, binary installation can be accomplished both more quickly and with less developer hassle. No more build.jl files, no need for a Pkg.build() step at all, in fact. Simply write using Glib_jll, and you will import bindings for libgobject, libglib, etc... Similarly, using Cairo_jll will import a binding for libcairo. To learn more about these JLL packages, read this WIP blog post. To learn more about the Pkg.Artifacts system that this is all built on top of, read the relevant docs.

@coveralls
Copy link

coveralls commented Sep 3, 2019

Coverage Status

Coverage increased (+0.7%) to 92.052% when pulling db8401b on staticfloat:sf/jll_pkg into 15706f2 on JuliaGraphics:master.

@codecov
Copy link

codecov bot commented Sep 4, 2019

Codecov Report

Merging #293 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

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

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 15706f2...db8401b. Read the comment docs.

@staticfloat
Copy link
Contributor Author

Huzzah! It's working!

@staticfloat
Copy link
Contributor Author

To help frame this in light of #292, this branch only works on Julia 1.3+, and it uses the new Artifacts installation code which has many improvements over Binary Provider for complicated use cases exactly like Cairo.jl. Examples of things that using the Artifacts system will get you:

  • Upgrading Cairo versions no longer requires a rebuild; because artifacts are stored outside of the package directory, you no longer need to re-install libcairo just because a new version of Cairo.jl was released.

  • Fewer Pkg.build() errors; because no code is run upon package install, (and no deps.jl file is generated) package install should be much more reliable.

  • Smarted dependency handling; Mosè has done a great job handcrafting the build.jl, but there's some unfortunate fragility within that file; installing dependencies in the wrong order will break, sometimes you have to disable a safety feature ("isolated dlopen") to get things to unpack properly, etc... JLL packages avoid these problems by doing all necessary inspection at build-time, then encoding the implicit ordering of build products into the dependency graph of JLL packages. In short, you don't need to worry about this, the complexity is abstracted away from you.

  • Better executable product handling; Executables need certain environment variables set to find their dependencies (LD_LIBRARY_PATH, PATH, etc...) the new JLL packages provide a convenient interface to walk the recursive dependency tree and set up the requisite environment variables without you ever needing to think about it.

For these reasons, I encourage a Julia 1.3+ release of Cairo, however due to the importance of this package a 1.2- branch will undoubtedly be relied upon for a while yet. Thus, this PR simply makes it easy to launch the 1.3+ branch when the developers are ready.

@tknopp
Copy link
Contributor

tknopp commented Sep 6, 2019

This is great @staticfloat!

So this is an alternative to #292 or can #292 first be merged and then after that this one?

Regarding releases, I would prefer that we make simultaneous releases of Cairo and Gtk, once the later has migrated to BB/BP. I am fine making both 1.3 exclusive. But then the release can only be made once 1.3 is out.

@staticfloat
Copy link
Contributor Author

#292 should be merged first, as that is compatible with the largest number of users right now. IMO, it's best to make a release of Cairo with these new binaries vendored through BinaryProvider, have that be compatible with Julia 1.0-1.2, then split that commit off into a compat-1.2 branch, and merge this into master. Then, by the time 1.3 is released, we will have Cairo, Gtk, Rsvg, etc... all ready and working on 1.3+.

I do not think we're going to create a 1.0-compatible release for Gtk, we're just going to skip to 1.3+ directly for those packages, because it's a fair amount of developer work to get it working, and we'd rather just make quick headway with the new tools. Cairo.jl was an exception because it's an extremely high-visibility package, and I personally get a lot of mail from people complaining that it's not working on 1.0. :)

@timholy
Copy link
Member

timholy commented Sep 7, 2019

I'm fine with going forward with Julia 1.3+.

I suspect Gtk will become the source of your mail now. At least for me, all my reports are of the form "can't use ImageView" or "can't use ProfileView" and that ultimately traces back to Cairo. Now we've moved a big step up the chain. Hoping that the new infrastructure makes Gtk not too bad. Anything we mere mortals can do to help out usefully? This is not an area of strength for me, but I feel bad about begging for help without doing something useful to pitch in.

@tknopp
Copy link
Contributor

tknopp commented Sep 7, 2019

I agree with Tim, that most Cairo issues reported are likely indirect issues with Gtk. Cairo of course stands on its own, but if you want to draw not only to a file but to the screen, something like Gtk is necessary

@lobingera
Copy link
Contributor

@timholy If you don't work on the binaries but want to contribute, there is still something you can do for adapting interfaces on the julia level e.g. GiovineItalia/Gadfly.jl#1309

src/Cairo.jl Outdated Show resolved Hide resolved
src/Cairo.jl Outdated Show resolved Hide resolved
Copy link
Contributor

@davidanthoff davidanthoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be awesome to get this over the finishing line! I made some inline comments, and then in addition I think we need to add bounds for the jll package to [compat], right?

Manifest.toml Outdated
@@ -0,0 +1,256 @@
# This file is machine-generated - editing it directly is not advised
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this whole file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you not want to include a Manifest.toml? It can be helpful to track down why, for instance, improper upper bounds caused a test to fail in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a big believer in not including Manifest.tomls in packages :) In my mind they record things that shouldn't be recorded in a package.

Not sure I understand the upper bound scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a big believer in not including Manifest.tomls in packages :)

It's your package, so by all means, manage it how you like! I'll remove it.

Not sure I understand the upper bound scenario.

The Manifest.toml records a "known good point" within the entire space of packages that the resolver can choose. If you use Pkg.add("Cairo"), you do not necessarily get a working setup for Cairo.jl, since the resolver is free to choose among a potentially very wide number of packages. If the dependency upper bounds are chosen perfectly, Pkg.add() should always work. However, it's pretty common that somebody updates a package somewhere in your deps tree that breaks something higher up in the deps tree, eventually causing Cairo.jl to break. This doesn't even have to be Cairo.jl's fault; it could be a transitive dependency that causes the problem.

Bundling a Manifest.toml provides two tools to help out:

  • If you are in the "just work, darnit" frame of mind, you can instantiate the Manifest.toml to get the versions that worked in the past (and thereby should always work, especially in light of JLL packages which extend instantiation's effect to binary packages).
  • If you are trying to debug what versions of a dependency should work, the Manifest at least gives you a known good point; you don't need to go and look at the Registry to see what versions would have been installed on your machine six months ago when you last modified this package; all that information is stored here.

Manifest.toml files bundled with packages aren't used for resolution or loading in any way during normal usage; you have to explicitly start Julia with --project= pointing at Cairo.jl for them to be used, and they'll only install their versions if you explicitly instantiate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's your package, so by all means, manage it how you like! I'll remove it.

Actually, it is not :) Not sure who is in charge, probably @lobingera?

Thanks for the explanation of the benefits, interesting. I think one main reason why I don't like to have Manifest.tomls in my packages is that VS Code will auto-activate an env if there is a Manifest.toml in a package folder, and that is something I don't want, so it throws a wrench into my normal workflow. Also, it just seems that almost all packages don't have it in the root, and I'm a fan of just following common practices with this kind of choice :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I had no idea VS Code did that. Interesting. I can see the reason it does it, but yes that would be frustrating. This is where the concept of an "application" versus a "package" start to conflict a little bit. :P

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the VS Code extension point of view, it would be great if a package project was called Package.toml, and an app project was called Project.toml, even if they have exactly the same syntax/content. Or some other way to distinguish a package from an app. It would just be really convenient for tools to be able to look at a folder and just say "ah, this is a package", or "ah, this is an app"...

Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
staticfloat and others added 5 commits November 25, 2019 14:41
This will allow for easier/faster binary installation, but only supports
Julia 1.3+
Co-Authored-By: David Anthoff <anthoff@berkeley.edu>
@staticfloat staticfloat force-pushed the sf/jll_pkg branch 2 times, most recently from d5ea6c8 to 77f9c12 Compare November 25, 2019 22:45
@staticfloat
Copy link
Contributor Author

Alright there was some confusion because I re-named a branch halfway through, but I think this should be good now.

Copy link
Contributor

@davidanthoff davidanthoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Can we merge? Who can actually merge? @lobingera or @timholy?

I would also suggest that we tag this version as 1.0.0, so that we enter sane semver territory.

@tknopp tknopp merged commit 9439bb1 into JuliaGraphics:master Nov 28, 2019
@tknopp
Copy link
Contributor

tknopp commented Nov 28, 2019

To move forward I merged this now. Will prepare the next release if there are no objections from @lobingera. Regarding version number. I don't care but I do also not see the pressing advantage of 1.0.0. Currently any bump of the 0.x is breaking. Afterwards any change of the first version number is breaking. I am fine with both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants