Work going forward

[magjac]

Thanks for your answers.

@smattr

I have questions about the choice of C++ though. Why? With Python you get batteries included (e.g. capable of parsing XML-based docs like SVG without any third party libs), more readable code and no need to manually manage memory. I don’t want to be too negative, but as someone who’s debugged a number of Graphviz segfaults in recent history, I really do not want to be debugging segfaults in the test suite as well.

@magjac

1. I get to practice my C++ skills which have gone from zero to…, well slight more than zero, during the last 6 month since I started a new assignment together with a very skilled colleague who is eager to help me learn.

To be honest, this is the primary reason. I’m going to give it a try, but if it doesn’t turn out well, I’ll switch to Python. Perhaps I will do a basic Python version later on anyway, if not only to test our Python bindings.

My colleague says he hasn’t debugged a memory related problem in C++ since 2013. I think he means in his own code though and I’m not entirely sure that he’s not exaggerating slightly.

[mark]

OK. Some more thoughts:

If the goal is fast tests, I think given how slow graphviz is on large inputs, parallelism will be the key to get faster test runs. Graphviz isn’t thread safe (global variables) so will need multiprocess to get test parallelism. This is kinda orthogonal to language choice: moving to C++ alone probably won’t make our tests much faster (the time spent inside the test framework is probably tiny compared to the time spent rendering): something like automatic test sharding (pytest-shard or bazel test sharding) might?

Just throwing out another idea: we already have some C++ code already that’s in need of upgrading to new idioms (eg smart pointers/automatic memory management): see Matthews recent MR on VisioRender as an example. Perhaps that would also fit your learning goals?

And we of course need some C++ tests for the C++ bindings.

none of this is to stop you, just doing a saturday morning brain dump

[magjac]

@mark

saturday morning

I knew your timezone was way ahead of mine, but not that it was by that much

[mark]

Hahahaha — I really thought it was Saturday! It’s easter friday public holiday here! Feels like “logical saturday” if not physical saturday

[smattr]

@magjac

To be honest, this is the primary reason. I’m going to give it a try, but if it doesn’t turn out well, I’ll switch to Python.

Fair enough. I retain my skepticism, but will happily be proven wrong

@mark

moving to C++ alone probably won’t make our tests much faster

Yes, this is immaterial. I would expect any test suite to be dominated by I/O. Torture tests that run expensive graphs may be an exception, but as you’ve said the test suite language has no effect here. I will be so bold as to say with no evidence: no language choice will make an impactful difference on test suite runtime. We should be optimizing for test case writing ergonomics, not execution speed.

@mark

we already have some C++ code already that’s in need of upgrading to new idioms

This is a great idea. Magnus, if you want to take this on I can step away from the C++ parts of Graphviz and act only as a reviewer of your MRs there. Plenty of work for me to do in the C parts of the code base.

[magjac]

Thanks again. I’ve been a bit unclear so I’ll try to clarify.

1. I think the best use of my time with Graphviz would be to create a new test suite so I would like to focus on that. This is primarily an altruistic reason. If we already had such a suite, I would’ve liked to focus on fixing bugs and/or reducing warnings, otherwise implementing enhancement requests or something else.
2. I would like to focus on something where I get to practice my C++ skills. Primarily because it’s fun to learn new things, but also because I will benefit from it professionally. This is of course a pure selfish reason.
3. So, unless you can convince me that writing a new test suite in C++ is in itself a bad idea and wouldn’t be accepted as an MR, that’s what I’m going to do. Perhaps I will create a basic one just focusing on our C++ bindings, then do the same for our Python bindings. We can then compare them in terms of readability, speed and whatnot and have a new discussion about the best way forward.

@smattr

I would expect any test suite to be dominated by I/O

The test suite I’m imagining wouldn’t have any I/O at all, but perhaps that’s not possible?

@mark

…given how slow graphviz is on large inputs…

The test suite I have in mind would focus on verifying all aspects (graph type, subgraphs, clusters, attributes and combinations thereof) of small graphs.

@smattr

Magnus, if you want to take this on I can step away from the C++ parts of Graphviz and act only as a reviewer of your MRs there. Plenty of work for me to do in the C parts of the code base

Thanks, I really appreciate this, but since I also think that

@smattr

an improved test suite would probably be the single most beneficial thing to Graphviz right now

, even if it turns out to be mostly Python in the end; that’s what I’m going to do. Python is my favorite language so I know I will enjoy that too. It’s just that learning new things is my favorite occupation and trumps coding in my favorite language.

@mark

It’s easter friday public holiday here!

Here too. (No easter bunny in Sweden)

[smattr]

OK, seems you’re pretty set on this direction. Here are some deficiencies with the existing test setup beyond coverage:

• tests/ vs rtest/ split is confusing
• make check doesn’t work
• all the reference tests in tests/reference fail
• the (barely there) unit tests in tests/unit_tests are disabled
• Graphviz fetches Criterion as a submodule but AFAIK it is unused

Is your plan expected to have an impact on any/all of the above?

[magjac]

No, my plan is to build something better from scratch. I might do something about your points anyway, but the work I’m currently planning is not going to have any immediate effect on them. In the end some of the old tests could possibly be removed if the new test suite covers the same functionality.

@mark

And we of course need some C++ tests for the C++ bindings

Do we actually have specific C++ bindings? I can only find the C code.

[mark]

Sorry I don’t know where I got the idea about C++ bindings, I must have just been guessing, we probably just use C functions from C++.

[magjac]

I assumed likewise. Perhaps we should create them? Some Internet search did not give anything obvious available from somewhere else.

[smattr]

Ha, I’d just assumed they existed since Mark referred to them! We could add some, but I don’t think this is a priority.

Magnus, if you want to write tests in C++, how about testing the C API (libcgraph)?

[schmoo2k]

@magjac - you might want to consider using: ctest(1) — CMake 3.20.0 Documentation

[magjac]

@smattr

how about testing the C API (libcgraph)?

Yes, that’s what I’m planning to do. And libgvc.

@schmoo2k

you might want to consider using: ctest(1)

Yes. We decided a while back to use pytest to drive all our tests with, but ctest + Catch2 comes in very handy for what I want to do.

[magjac]

@smattr

We could add some, but I don’t think this is a priority.

I think that part of my work will be to make a C++ abstraction layer on top of libgvc and libcgraph so we might get parts of it as a bonus. I’m quite aware that supporting a full-blown public API is a different beast than making a thin layer for test, but it would be interesting the hear your opinion about where the files should reside and how they should be named given that we pursue it. For now I’m going the create wrapper next to the tests.

[magjac]

Perhaps just within the already existing #ifdef __cplusplus in their respective header file?

[smattr]

@magjac

it would be interesting the hear your opinion about where the files should reside

Probably in tclpkg/gv. A first step would be to make sense of the existing SWIG bindings there.

[magjac]

@smattr

Probably in tclpkg/gv. A first step would be to make sense of the existing SWIG bindings there.

Can SWIG be used to create C++ from C? That seems a bit odd. Is that what we really want? I’m thinking that we should have a more high-level C++ interface. I’ve started experimenting with something like:

diff --git a/lib/cgraph/cgraph.h b/lib/cgraph/cgraph.h
index 01c83d4a1..576a46715 100644
--- a/lib/cgraph/cgraph.h
+++ b/lib/cgraph/cgraph.h
@@ -457,5 +459,21 @@ and edges are embedded in main graph objects but allocated separately in subgrap
 
 #ifdef __cplusplus
 }
+
+class AGraph
+{
+public:
+    explicit AGraph(const char* dot);
+    ~AGraph();
+    Agraph_t* get() const
+    {
+        return m_g;
+    };
+
+private:
+    Agraph_t* const m_g = nullptr;
+};

and:

index 28909f063..ca6849ebe 100644
--- a/lib/gvc/gvc.h
+++ b/lib/gvc/gvc.h
@@ -119,6 +122,22 @@ extern int gvToolTred(graph_t *g);
 
 #ifdef __cplusplus
 }
+
+class GVContext
+{
+public:
+    explicit GVContext();
+    ~GVContext();
+    GVC_t* get() const
+    {
+        return m_gvc;
+    }
+    void layout(AGraph& g, const char* engine);
+
+private:
+    GVC_t* const m_gvc = nullptr;
+    AGraph* m_graph = nullptr;
+};
 #endif
 
 #endif                 /* GVC_H */

[magjac]

@smattr Just a friendly reminder in case you missed this question.

[smattr]

Sorry, I didn’t miss it, I just didn’t know the answer. Looking up the SWIG web page, the answer is no.

Your proposed C++ API looks reasonable to me, but I would use smart pointers, std::string, and put it in a separate header. If you want a nit, the explicit on the GVContext constructor is unnecessary.

To reiterate my opinion on this: I do not think this is a priority and we would be better off paying down the technical debt in the existing C++ code.

[magjac]

Thanks. I’m not doing this because I want to provide our users with a C++ API, but because I want to be able to write the new test suite using modern C++ constructs on top of it.