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

Add Graph tests #237

Closed
isaacsas opened this issue Aug 3, 2020 · 24 comments
Closed

Add Graph tests #237

isaacsas opened this issue Aug 3, 2020 · 24 comments
Assignees

Comments

@isaacsas
Copy link
Member

isaacsas commented Aug 3, 2020

I think we will also need a Graph call update for an API change on Catlab master soon as in:

https://github.com/mehalter/Petri.jl/pull/32/files

@isaacsas isaacsas self-assigned this Aug 3, 2020
@isaacsas
Copy link
Member Author

isaacsas commented Aug 4, 2020

Test from #242

Still need to update Graph syntax for after a new Catlab release is made.

@isaacsas isaacsas closed this as completed Aug 5, 2020
@isaacsas
Copy link
Member Author

isaacsas commented Aug 5, 2020

Can't add tests due to needing a commandline Graphviz install.

@isaacsas isaacsas reopened this Aug 5, 2020
@ChrisRackauckas
Copy link
Member

Maybe @christopher-dG can look into building an artifact for that. Catlab.jl should really ship with the binary (@jpfairbanks)

@jpfairbanks
Copy link

I don’t know how to make that work, but given binary builder’s reputation it seems possible. What do we need to do to make it happen?

@ChrisRackauckas
Copy link
Member

Someone who knows a bit of binary magic.

@isaacsas
Copy link
Member Author

isaacsas commented Aug 5, 2020

That would be great to have, and would help us both with CI.

@isaacsas
Copy link
Member Author

isaacsas commented Aug 5, 2020

If binary builder proves problematic, another option is it appears available through Anaconda. Maybe it can be pulled in using Conda.jl?

@ChrisRackauckas
Copy link
Member

Conda is scary and leads to maintenance issues over time. I'd 100% try to avoid that if possible.

@christopher-dG
Copy link
Contributor

I'll have a crack at building it with BB today. Looks straightforward unless there are some dependencies we don't yet have.

@jpfairbanks
Copy link

jpfairbanks commented Aug 5, 2020

Awesome I’ll ping @epatters to make sure he is on the lookout for any Catlab PRs

@christopher-dG
Copy link
Contributor

Wait... Graphviz_jll is already a thing. Have you guys tried that?

@ChrisRackauckas
Copy link
Member

fastest binary building in the west!

@jpfairbanks
Copy link

It wasn’t working/sustainable at the time we adopted the current set up. Not sure if that has changed. I’d be happy to modernize that if necessary/feasible

@christopher-dG
Copy link
Contributor

What was wrong with it?

@jpfairbanks
Copy link

We’ll have to wait for the west coast to wake up to ask Evan :)

@isaacsas
Copy link
Member Author

isaacsas commented Aug 5, 2020

@jpfairbanks Having not used jll's before I found this helpful:

https://github.com/JuliaBinaryWrappers/Graphviz_jll.jl

@isaacsas
Copy link
Member Author

isaacsas commented Aug 5, 2020

Maybe the issue is that Catlab wants to call more of the executables than are wrapped? It seems to want to call any of:

"dot","neato","fdp","sfdp","twopi","circo"

see

https://github.com/AlgebraicJulia/Catlab.jl/blob/7c0313f011d6178433a76c05d64eb138da1330c9/src/graphics/Graphviz.jl#L121

@christopher-dG
Copy link
Contributor

christopher-dG commented Aug 5, 2020

I just ran Graphviz's build_tarballs.jl manually and it does build all of those executables, they just aren't declared as products for the wrapper. I can update the build script to include all of those products.

Here's the full list of new files in $bindir after building Graphviz:

acyclic
bcomps
ccomps
circo
cluster
dijkstra
dot
dot2gxl
dot_builtins
edgepaint
fdp
gc
gml2gv
graphml2gv
gv2gml
gv2gxl
gvcolor
gvgen
gvmap
gvmap.sh
gvpack
gvpr
gxl2dot
gxl2gv
mm2gv
neato
nop
osage
patchwork
prune
sccmap
sfdp
tred
twopi
unflatten

@isaacsas
Copy link
Member Author

isaacsas commented Aug 5, 2020

Awesome, thanks for updating that! Hopefully with that change it will be a small tweak to the Catlab open call here

https://github.com/AlgebraicJulia/Catlab.jl/blob/7c0313f011d6178433a76c05d64eb138da1330c9/src/graphics/Graphviz.jl#L122

to get this working now.

@epatters
Copy link

epatters commented Aug 5, 2020

So the history is: originally I used GraphViz.jl, which provides bindings to the Graphviz C API, but it became unmaintained and I couldn't port it myself, so then I switched to just calling Graphviz through its CLI, assuming that the user had it installed.

Great to hear that somebody has built Graphviz using BinaryBuilder! Looks like its only a few months old. Once it supplies all the right binaries we can definitely switch to that at Catlab.

@christopher-dG
Copy link
Contributor

Nice, I'll get that PR up to Yggdrasil shortly.

@giordano
Copy link

giordano commented Aug 5, 2020

Looks straightforward

😂 😂 JuliaPackaging/Yggdrasil#351 has been open for 5 months, look at the patches 😂 And it doesn't even build for Windows because the entire Graphviz is completely messed up

@christopher-dG
Copy link
Contributor

Yeah... I may have choked a bit when I saw the existing builder 😅

@isaacsas
Copy link
Member Author

isaacsas commented Aug 7, 2020

@christopher-dG @giordano Thanks for all your help with this! With the new jll setup this will fix our issues here.

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

No branches or pull requests

6 participants