First stab at reviving kgraphviewer

[adridg]

Summary

revive port which is now released by riddell and frinring

Test Plan

builds on my 12-CURRENT laptop

[rakuco]

Almost there IMO. One important thing to keep in mind is that, when landing, you're supposed to svn cp the old kgraphviz port and land your changes on top of it instead of svn add'ing it from scratch.

graphics/kgraphviewer/Makefile

10 ↗

(On Diff #33530)

Are you sure this isn't GPLv2+?

23 ↗

(On Diff #33530)

Extra empty line.

graphics/kgraphviewer/files/patch-findgraphviz.cmake

1 ↗

(On Diff #33530)

Is the old-fashioned machinery that's part of this release actually broken? If not, I'd just leave things be and automatically get the new machinery with a new release.

graphics/kgraphviewer/files/patch-src_part_CMakeLists.txt

1 ↗

(On Diff #33530)

Hmm.

I'm looking at FindGraphviz.cmake here, and it seems to to the right thing when it comes to setting graphviz_LIBRARIES:

if ( graphviz_INCLUDE_DIRECTORIES AND
      graphviz_GVC_LIBRARY AND graphviz_CDT_LIBRARY AND
      graphviz_GRAPH_LIBRARY AND graphviz_PATHPLAN_LIBRARY )
  set ( graphviz_FOUND 1 )
  set ( graphviz_LIBRARIES
      "${graphviz_GVC_LIBRARY};${graphviz_GRAPH_LIBRARY};"
      "${graphviz_CDT_LIBRARY};${graphviz_PATHPLAN_LIBRARY}"
      CACHE FILEPATH "Libraries for graphviz" )

graphviz_GVC_LIBRARY and friends are set via find_library(), so you should have full paths there and not need graphviz_LDFLAGS.

Unless something's funny going on because pkg_check_modules( graphviz ${REQUIRED} libgvc libcdt libcgraph libpathplan ) also sets variables with the graphviz_ prefix (including graphviz_LIBRARIES).

Have you tried just renaming graphviz to something like PC_GRAPHVIZ in the pkg_check_modules() call (like many other find-modules) do?

[adridg]

graphics/kgraphviewer/Makefile

10 ↗

(On Diff #33530)

Yes. The COPYING file is GPLv2 (naturally .. there is not a file that actually says GPLv2+), and each source file I have looked at says

KGraphViewer is free software; you can redistribute it and/or
modify it under the terms of the GNU General Public
License as published by the Free Software Foundation, version 2.

graphics/kgraphviewer/files/patch-findgraphviz.cmake

1 ↗

(On Diff #33530)

I suppose we could stick to just fixing the linking problem, leaving the find-machinery unchanged, yes.

graphics/kgraphviewer/files/patch-src_part_CMakeLists.txt

1 ↗

(On Diff #33530)

It's the pkg_check_mnodules() call indeed, which fiulls in the libraries already which causes find_library() to not, actually, look for them. Renaming as suggested to pc_graphviz solves a bunch.

graphics/kgraphviewer/pkg-descr

3 ↗

(On Diff #33530)

Should be https

[adridg]

Simplify patches following rakuco@'s advice, and don't introduce
non-essential changes that will trickle down from upstream with next release.

[adridg]

Update with full "resurrection" diff against kgraphviewer@346396

[tcberner]

graphics/kgraphviewer/Makefile

20 ↗

(On Diff #33556)

I assume this would need at least qmake_build

graphics/kgraphviewer/pkg-plist

87 ↗

(On Diff #33556)

I don't like partial subs -- can you replace this here with the explicit share/data/kgraphviewer (not that they don't slip in occasionally...)?

[adridg]

Missing build-deps, suspected by tcberner@

[adridg]

Patch up the partial plist-subs

[rakuco]

graphics/kgraphviewer/Makefile

1 ↗

(On Diff #33556)

You still need this line -- the commit hooks won't let you land this file if it lacks the $FreeBSD$ tag.

graphics/kgraphviewer/files/patch-cmake_FindGraphviz.cmake

14–17 ↗

(On Diff #33562)

I don't follow what's being done here. In general, the idea is to use pkg-config's output as a hint for the actual find_*() CMake calls. See https://cgit.kde.org/extra-cmake-modules.git/tree/find-modules/FindEGL.cmake, for example: it calls pkg_check_modules() and then just passes some of the values as HINTS in the subsequent find_path and find_library calls.

You could also fix it upstream so we inherit it in the next release and just drop the pkg-config bits from here in the meantime (assuming graphviz isn't installed into crazy paths on FreeBSD).

29 ↗

(On Diff #33562)

... which would then allow you to drop this part of the patch as well. The *_LIBRARY variables should be full paths anyway, so I don't see the reason to do this in this review request (it makes sense to do so upstream though).

graphics/kgraphviewer/files/patch-src_part_CMakeLists.txt

8 ↗

(On Diff #33562)

I know what's being done here, but can you add some context to the patch for others who aren't as familiar with CMake?

[adridg]

graphics/kgraphviewer/files/patch-cmake_FindGraphviz.cmake

14–17 ↗

(On Diff #33562)

This tries to change things in one place, instead of fixing HINTS in several places elsewhere. There's some tension here between making the find-module "right" and modern, and just getting it to work in the port. Upstream a much larger part of the find-module is being replaced. You are right, though, that this does too much.

29 ↗

(On Diff #33562)

No, this code in the find module is just wrong, creating a single filepath cache entry with semi-colon separated full paths to libraries. Using the existing code later expands to -lgvc instead of /path/to/libgvc.so .

[rakuco]

I've added one inline comment about the CMake bit, but what I have in mind is basically this: https://paste.kde.org/pl55sr7am/ludwj5 I've tried that one out (together with your src/parts/CMakeLists.txt change) and it all built fine.

Can you give it a try? If so, I think we can use that diff instead, and I can take care of upstreaming it.

graphics/kgraphviewer/files/patch-cmake_FindGraphviz.cmake

29 ↗

(On Diff #33562)

The FILEPATH bit is wrong, but it's just used as a hint by CMake to decide which UI to show when the user wants to edit this cache variable with cmake-gui or ccmake.

What worries me more is the -lgvc part you mentioned: lists in CMake are just semicolon-separated strings anyway, but the values should be full paths given they should've been set by find_library().

[adridg]

Update patches following rakuco@'s investigation.

[adridg]

I may have been getting bedazzled by errors both in libkgraphviewer and kgraphviewerpart, and misinterpreted the latter as something weird in the link line for the former. Following rakuco@'s advice on the find-mnodule works fine.

[adridg]

Update patches with rationale.

[tcberner]

graphics/kgraphviewer/Makefile

6 ↗

(On Diff #33588)

^ I'm not sure, wheter this should now be DISTVERSION -- but that's probably irrelevant until you update it to 2.4.3-rc1

[rakuco]

thanks for working on this!

[tcberner]

Builds fine on 10 and 12, ship it.