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

[new release] ocamlgraph_gtk and ocamlgraph (2.0.0) #17344

Merged
merged 6 commits into from
Oct 8, 2020

Conversation

backtracking
Copy link
Contributor

CHANGES:

  • port to dune and opam 2.0
    • ❗ opam package now split into two packages: ocamlgraph and ocamlgraph_gtk
    • [WeakTopological] fixed incorrect use of generic hash tables
      (Replace generic hash table by functorial in WeakTopological backtracking/ocamlgraph#99, Tomáš Dacík)
    • [Oper] fixed transitive_reduction (Transitive reduction is incorrect backtracking/ocamlgraph#91)
    • fix incorrect uses of polymorphic equality (Steffen Smolka, Boris Yakobowski)
    • [Coloring] fixed generation of OCamlDoc documentation
      (contributed by Earnestly)
    • ❗ [Coloring] functions now fail if the graph is directed
    • ❗ [Coloring] now uses a single, global exception [NoColoring]
    • [Coloring] new function two_color to 2-color a graph (or fail)
    • ❗ [Fixpoint] Take initial labeling of nodes into account (Johannes Kloos)

CHANGES:

- port to dune and opam 2.0
  - ❗ opam package now split into two packages: ocamlgraph and ocamlgraph_gtk
  - [WeakTopological] fixed incorrect use of generic hash tables
    (backtracking/ocamlgraph#99, Tomáš Dacík)
  - [Oper] fixed transitive_reduction (backtracking/ocamlgraph#91)
  - fix incorrect uses of polymorphic equality (Steffen Smolka, Boris Yakobowski)
  - [Coloring] fixed generation of OCamlDoc documentation
    (contributed by Earnestly)
  - ❗ [Coloring] functions now fail if the graph is directed
  - ❗ [Coloring] now uses a single, global exception [NoColoring]
  - [Coloring] new function two_color to 2-color a graph (or fail)
  - ❗ [Fixpoint] Take initial labeling of nodes into account (Johannes Kloos)
@backtracking
Copy link
Contributor Author

Please do not merge, even if CI passes. (We still need to check with the Frama-C team is this is fine for them.)

@camelus
Copy link
Contributor

camelus commented Oct 2, 2020

Commit: 7d6c312

@backtracking has posted 30 contributions.

☀️ All lint checks passed 7d6c312
  • These packages passed lint tests: dose3.5.0.1, llvmgraph.0.2, not-ocamlfind.0.07, ocamlgraph.2.0.0, ocamlgraph_gtk.2.0.0, opam-lib.1.3.1, why3.1.2.1

☀️ Installability check (+2)
  • new installable packages (2): ocamlgraph.2.0.0 ocamlgraph_gtk.2.0.0

@backtracking
Copy link
Contributor Author

The Travis CI build failed with OCaml 4.09 with the following error:

[ERROR] The compilation of conf-gnomecanvas failed at
        "/usr/local/bin/pkg-config libgnomecanvas-2.0".

Is this something I can do anything about?

@mseri
Copy link
Member

mseri commented Oct 2, 2020

Do you know if conf-gnomecanvas can be made compatible with freebsd?

@kit-ty-kate
Copy link
Member

kit-ty-kate commented Oct 2, 2020

Do you know if conf-gnomecanvas can be made compatible with freebsd?

I just tested locally and there is a libgnomecanvas package. I'll make a separate PR

@mseri
Copy link
Member

mseri commented Oct 5, 2020

There is only a weird failure on the tests:

#=== ERROR while compiling ocamlgraph.2.0.0 ===================================#
# context              2.0.7 | linux/x86_64 | ocaml-base-compiler.4.08.1 | pinned(https://github.com/backtracking/ocamlgraph/releases/download/2.0.0/ocamlgraph-2.0.0.tbz)
# path                 ~/.opam/4.08/.opam-switch/build/ocamlgraph.2.0.0
# command              ~/.opam/4.08/bin/dune build -p ocamlgraph -j 72 @install @runtest
# exit-code            1
# env-file             ~/.opam/log/ocamlgraph-29-ebe0e0.env
# output-file          ~/.opam/log/ocamlgraph-29-ebe0e0.out
### output ###
# File "examples/dune", line 3, characters 23-31:
# 3 |  (libraries graph unix graphics threads))
#                            ^^^^^^^^
# Error: Library "graphics" not found.
# Hint: try:
#   dune external-lib-deps --missing -p ocamlgraph -j 72 @install @runtest
# test_chaotic: all tests succeeded.
# dot: all tests succeeded.
# test_fixpoint: all tests succeeded.
# strat: all tests succeeded.
# test_wto: all tests succeeded.
# test_johnson: all tests succeeded.
# basic: all tests succeeded.
# test_components: all tests succeeded.
# test_bf: all tests succeeded.
# test_topsort alias tests/runtest
# test topsort: all tests succeeded.
#        check alias tests/runtest
# check: all tests succeeded

Should we add graphics as a with-test dependency? Not sure why it fails only on 4.08. If it is not critical, we can just ignore the failure or disable the tests for that case.

@pascutto
Copy link
Contributor

pascutto commented Oct 5, 2020

It's not clear to me why this fails; it is already added as a test dependency in 4461cfc.

@kit-ty-kate
Copy link
Member

I did a bit of digging here and it seems to be because the graphics package does not install any META file itself and instead let ocamlfind do it (but in the case of OCaml 4.08 ocamlfind wasn't needed) :/ This is a bit weird. @dra27 is that expected?

@backtracking
Copy link
Contributor Author

I added a constraint <= "1.8.8" for the 5 packages that did not compile anymore.

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

A couple of constraint suggestions. The alternative, notwithstanding an update to the graphics packages, is to add "ocamlfind" {with-test}, but it's a shame to switch to Dune and still actively depend on ocamlfind!

@@ -26,7 +26,7 @@ remove: [
]
depends: [
"ocaml"
"ocamlgraph" {>= "1.8.6"}
"ocamlgraph" {>= "1.8.6" & <= "1.8.8"}
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to allow the possibility of a 1.8.x patch release and make this:

Suggested change
"ocamlgraph" {>= "1.8.6" & <= "1.8.8"}
"ocamlgraph" {>= "1.8.6" & < "2.0.0"}

(and others, if you agree)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

depends: [
"ocaml" {>= "4.03.0"}
"stdlib-shims"
"dune" {>= "2.0"}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"dune" {>= "2.0"}
"dune" {>= "2.0" & !with-test | >= "2.8"}

This fixes the issue with the graphics package (which at present means ocamlgraph.2.0.0 can't be installed with --with-test on 4.08 and earlier)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"lablgtk"
"conf-gnomecanvas"
"ocamlgraph"
"dune" {>= "2.0"}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"dune" {>= "2.0"}
"dune" {>= "2.0" & !with-test | >= "2.8"}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Many thanks @dra27 for all these suggestions. You have to pardon me for my inexperience with dune and opam packaging; I'm relatively new to these things.

Copy link
Member

Choose a reason for hiding this comment

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

No problem at all! The graphics/Dune thing is an unusual corner-case anyway. It's lovely to have ocamlgraph ported to build with Dune (having maintained patches for it in opam for quite a while!)

@mseri
Copy link
Member

mseri commented Oct 7, 2020

I will wait for the big CI to run, but I think this is now good to merge

@mseri mseri merged commit 3072a29 into ocaml:master Oct 8, 2020
@mseri
Copy link
Member

mseri commented Oct 8, 2020

The CI runs look good. Thank you all very much

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

Successfully merging this pull request may close these issues.

6 participants