smyrna: duplicate names when creating new grid columns
This is a step towards solving a memory leak. Note that this involved rewriting
some string comparisons because the originating pointer is no longer stored in
the column’s `name`. It is not clear to me how the previous code was valid
because, e.g., it was comparing by pointer to the `Visible` constant to detect
when a column was called “Visible”. That is, nothing actually assigns a column
name to the address of `Visible`.
smyrna: squash -Wconversion warnings related to grid column count
This squashes the following compiler warnings:
tvnodes.c: In function ‘update_tree’:
tvnodes.c:294:28: warning: conversion to ‘size_t’ {aka ‘long unsigned
int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
294 | types = gv_calloc(g->count, sizeof(GType));
| ~^~~~~~~
tvnodes.c: In function ‘add_column’:
tvnodes.c:312:43: warning: conversion to ‘size_t’ {aka ‘long unsigned
int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
312 | g->columns = gv_recalloc(g->columns, g->count, g->count + 1,
| ~^~~~~~~
tvnodes.c:312:61: warning: conversion to ‘size_t’ {aka ‘long unsigned
int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
312 | g->columns = gv_recalloc(g->columns, g->count, g->count + 1,
| ~~~~~~~~~^~~
`count` is known non-negative all the time, but cannot be easily converted to a
`size_t` because we need to interact with the GTK APIs that take `int` values.
The last non-trivial call to `_sfpopen` was removed in f552c0dece1435773de62334dee3310d1728e22f. Since then, `proc` members have been
allocated and propagated around with data describing the absence of a subprocess
(`pid == -1`). We can reduce memory overhead and decrease code complexity by
removing these semantically dead code paths.
sfio: use a more portable way of determining page size
This code was using a legacy API, `getpagesize`, to opportunistically find the
system virtual memory page size. This change updates to the newer POSIX
`sysconf` API that is available almost everywhere, and adds a workaround for
Windows. This code now has an accurate understanding of the page size on
platforms that did not have `getpagesize` (of which, Windows was one), rather
than falling back on 8192.
673b9f1a7dbde9c9cc5d9a2a22ee835a08ab40ab tried to work around this in the past.
But there remained a foot gun. If you (possibly transitively) included sfio.h
but did not include sfhdr.h, you could end up facing this error.
Surprising as it may seem, nothing in the code base checks for `SF_ERROR`. So a
cleaner solution that removes the problem in perpetuity is to remove our
`SF_ERROR`. Note that `SF_FLAGS` does not need to be adjusted because `SF_ERROR`
was not a public flag.
CI: remove command line argument parsing from the deploy script
This script is called in exactly one place and always passed the `--verbose`
option. This change removes this only remaining option from the script and
unconditionally applies its effect, for a minor simplification.
CMake: stop trying to install Expat and Getopt on MinGW
MinGW does not need these third-party libraries installed; they are already
available in its ecosystem. This was uncovered through using
`--warn-uninitialized -Werror=dev` with CMake:
CMake Error (dev) at cmd/tools/CMakeLists.txt:460 (install):
uninitialized variable 'GETOPT_RUNTIME_LIBRARIES'
CMake: use legacy 'GLUT_INCLUDE_DIR' instead of 'GLUT_INCLUDE_DIRS'
The newer `GLUT_INCLUDE_DIRS` is only provided in CMake ≥ 3.23. Most users and
most of our CI environments are on a version of CMake prior to this and so get
the following when configuring with `--warn-uninitialized -Werror=dev`:
CMake Error (dev) at cmd/smyrna/CMakeLists.txt:87 (target_include_directories):
uninitialized variable 'GLUT_INCLUDE_DIRS'
b5f4448a3f3a70e14ba7d6990b81506cbda82180 made this closer to correct but still
wrong. As its message mentioned, FindDevIL.cmake is frustratingly inconsistent¹
with respect to the rest of the CMake ecosystem.
Some “extra” CI jobs like the static build have been left on Ubuntu 22.04 under
the assumption that we want to run these on the latest LTS release. But for the
ones affected in this commit, being on a distro with the latest toolchains seems
more desirable.
cdt: use 'size_t' types for counting stats instead of 'int'
This squashes two compiler warnings:
dtstat.c: In function ‘dtstat’:
dtstat.c:60:59: warning: conversion to ‘long unsigned int’ from ‘int’ may
change the sign of the result [-Wsign-conversion]
60 | if(!(Count = malloc((ds->dt_max+1)*sizeof(int))) )
| ^
dtstat.c:74:65: warning: conversion to ‘long unsigned int’ from ‘int’ may
change the sign of the result [-Wsign-conversion]
74 | if(!(Count = malloc((ds->dt_n+1)*sizeof(int))) )
| ^
It also allows these functions to now count beyond 2³¹ - 1.
dtstrhash.c: In function ‘dtstrhash’:
dtstrhash.c:36:18: warning: conversion to ‘unsigned int’ from ‘int’ may change
the sign of the result [-Wsign-conversion]
36 | return (h+n)*DT_PRIME;
| ^
`n` is guaranteed non-negative by the logic preceding this.
cdt: make intent to coerce to unsigned clearer in 'dtstrhash'
This squashes some compiler warnings on CentOS 7:
dtstrhash.c: In function 'dtstrhash':
dtstrhash.c:22:18: warning: conversion to 'unsigned int' from 'int' may change
the sign of the result [-Wsign-conversion]
h = (h + (s[0]<<8) + s[1])*DT_PRIME;
^
dtstrhash.c:28:18: warning: conversion to 'unsigned int' from 'int' may change
the sign of the result [-Wsign-conversion]
h = (h + (s[0]<<8) + s[1])*DT_PRIME;
^
dtstrhash.c:30:18: warning: conversion to 'unsigned int' from 'int' may change
the sign of the result [-Wsign-conversion]
h = (h + (s[0]<<8))*DT_PRIME;
smyrna: avoid inadvertent double promotion in 'renderSelectedNodes'
Squashes two compiler warnings:
topviewfuncs.c: In function ‘renderSelectedNodes’:
topviewfuncs.c:283:41: warning: conversion from ‘double’ to ‘GLfloat’ {aka
‘float’} may change value [-Wfloat-conversion]
283 | glVertex3f(pos.x,pos.y,pos.z+0.001);
| ~~~~~^~~~~~
topviewfuncs.c:285:50: warning: conversion from ‘double’ to ‘float’ may
change value [-Wfloat-conversion]
285 | drawCircle(pos.x,pos.y,nodeSize,pos.z+0.001);
| ~~~~~^~~~~~
smyrna: remove 'line' shadowing in 'load_attr_list'
Squashes a compiler warning:
gui/frmobjectui.c:580:10: warning: declaration of ‘line’ shadows a global
declaration [-Wshadow]
580 | char line[BUFSIZ];
| ^~~~
In file included from ./smyrnadefs.h:27,
from gui/gui.h:13,
from gui/frmobjectui.c:15:
../../lib/glcomp/glutils.h:25:7: note: shadowed declaration is here
25 | } line;
| ^~~~
smyrna: remove 'line' shadowing in 'load_attributes'
Squashes a compiler warning:
gui/gui.c: In function ‘load_attributes’:
gui/gui.c:45:10: warning: declaration of ‘line’ shadows a global declaration
[-Wshadow]
45 | char line[BUFSIZ];
| ^~~~
In file included from ./smyrnadefs.h:27,
from gui/gui.h:13,
from gui/gui.c:14:
../../lib/glcomp/glutils.h:25:7: note: shadowed declaration is here
25 | } line;
| ^~~~
smyrna: use a more appropriate type for 'ComboValuesCount'
Squashes two compiler warnings:
gui/gui.c:94:96: warning: conversion to ‘size_t’ {aka ‘long unsigned
int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
94 | attr[attrcount].ComboValuesCount - 1,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
gui/gui.c:95:78: warning: conversion to ‘size_t’ {aka ‘long unsigned
int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
95 | attr[attrcount].ComboValuesCount,
| ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
fix installing example graphs during CPack; rewrite 'SMYRNA_PATH' discovery
This is effectively two commits in one. But there seems to be no way to separate
these changes without the parts individually failing
tests/test_tools.py::test_tools[smyrna]. Smyrna auxiliary files were being
installed incorrectly prior to this commit, so installing them correctly and
fixing how Smyrna finds them needs to be done in a single change.
---
CMake: fix installing example graphs during CPack
When running `cpack`, instead of staging the Smyrna auxiliary files (share/**)
and the example graphs (graphs/**) in build-local directories, they would be
incorrectly staged into their final install paths. This was quite unexpected as
it would result in errors if you did not have permission to write to the install
directories or deleting+replacing files from a previous Graphviz installation if
you did.
This bug was introduced in 67f88eb86c9966daa4d8c3a6f25aee8cd35a046d as an
accidental side effect of enabling Smyrna in the CMake build system. A close
re-reading of the CPack¹ and `GNUInstallDirs`² docs suggests that absolute paths
should never be used in `install` rules:
`CMAKE_INSTALL_<dir>`
Destination for files of a given type. This value may be passed to the
`DESTINATION` options of `install()` commands for the corresponding file
type. It should typically be a path relative to the installation prefix so
that it can be converted to an absolute path in a relocatable way (see
`CMAKE_INSTALL_FULL_<dir>`). However, an absolute path is also allowed.
`CMAKE_INSTALL_FULL_<dir>`
The absolute path generated from the corresponding `CMAKE_INSTALL_<dir>`
value. If the value is not already an absolute path, an absolute path is
constructed typically by prepending the value of the `CMAKE_INSTALL_PREFIX`
variable. However, there are some special cases as documented below.
This change brings the behavior on Linux in line with how CPack operates on
other platforms – the default value of `CMAKE_INSTALL_DATAROOTDIR` is `share`
which is the hard coded value set on non-Linux platforms.
----
smyrna: rewrite 'SMYRNA_PATH' discovery
This aligns Smyrna on other platforms with how Smyrna on Windows locates its
templates, examples etc. This resolves a problem where a build time path was
being baked into the Smyrna binary, preventing it from being relocatable.
The code for locating our own executable in this commit is adapted from public
domain source.³
This commit also lifts the 1024 character limit on path discovery in the Windows
branch of this logic. It now dynamically expands the target buffer until the
current executable name will fit.
Gitlab: fixes #2232 Reported-by: Magnus Jacobsson <magnus.jacobsson@berotec.se>
¹ https://cmake.org/cmake/help/book/mastering-cmake/chapter/Packaging%20With%20CPack.html
² https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html
³ https://github.com/Smattr/clink/blob/main/clink/src/find_me.c as of commit 8cadfc49a74e429fa69afdc460cb2b0662d81260
tests: only accept 'which' results that are adjacent to 'dot'
As described in the code comment, this addresses a problem where testing would
find binaries from a prior Graphviz installation and then spuriously fail.
This is not ideal. It is easy for an upcoming test writer to forget or be
unaware of this quirk and introduce a new direct call to `shutil.which`. But it
seems the best option we have right now.
After this function was written, the `startswith` helper was introduced for
abbreviating the idiom `strncmp(foo, bar, strlen(bar)) == 0`. We can use it to
more concisely and clearly express the intent of this code.
Graphviz periodically runs into problems where CI test jobs exceed the maximum
timeout and recently the project as a whole has exceeded its Gitlab CI quota.
`test_2095` is by far the longest running component of the test suite. This
change is the result of applying two test case minimizers, one clever¹ and one
not so clever², to the input to this test case. The minimizers were asked to
find smaller input that still (1) provoked a segfault on 588096bd638543ea851ea22751ed91549f61a407 and (2) could be processed successfully
on 32feee561394530713292f8873020fc5feacb9fb. The result takes a ~103KB test
input to ~5KB, with execution time in an example environment dropping from
~155s to <1s.
This dictionary was only ever inserted to by code that was removed in the
previous commit as unreachable. So we know it is always empty and lookups into
it would have always returned null.
graphml2gv: remove dead code handling ending of 'attr' attribute
The code in the last branch in `endElementHandler` was dealing with a closing
`</attr>` tag in the input GraphML. However, the preceding `startElementHandler`
callback rejects an opening `<attr>` tag. So there is no way to reach this
closing logic. This change removes not only the local unreachable code but
several transitively unreachable functions.
As an aside, this was only discovered while trying to construct a test case to
provoke #2300. One path to `dict_relabel` is through `graphml2gv`, by handling a
closing `</attr>` tag. So I looked online for sample GraphML using `attr`. I
could not find any. So I went to the GraphML specification¹ and there discovered
`attr` is not a valid GraphML tag. So I thought, fine, I will figure out how
`graphml2gv` expects this non-standard tag to appear. And that is when I
discovered none of this logic is reachable.
This does not appear to be the result of changes. The very first revision of
`graphml2gv`, 1d28d7d2b4d2b2551bd1f432aa175f54a69364a4, seems to have this
limitation already. I can only conclude this was copy-pasted from gxl2gv without
taking into account the differences between GXL and GraphML.
Boost 1.74.0 deprecates some functions that svgpp is using. Without squashing
these, the CMake build fails in CI:
[ 86%] Building CXX object
tests/CMakeFiles/test_common.dir/svgpp_context.cpp.o
In file included from /usr/include/boost/config/header_deprecated.hpp:18,
from /usr/include/boost/detail/scoped_enum_emulation.hpp:15,
from /usr/include/svgpp/detail/namespace.hpp:13,
from /usr/include/svgpp/detail/attribute_name_to_id.hpp:11,
from
/usr/include/svgpp/attribute_traversal/prioritized.hpp:12,
from
/usr/include/svgpp/attribute_traversal/attribute_traversal.hpp:10,
from /usr/include/svgpp/document_traversal.hpp:10,
from /usr/include/svgpp/svgpp.hpp:1,
from /builds/graphviz/graphviz/tests/svgpp_context.h:7,
from /builds/graphviz/graphviz/tests/svgpp_context.cpp:8:
/usr/include/boost/detail/scoped_enum_emulation.hpp:17:1: note: '#pragma
message: This header is deprecated. Use <boost/core/scoped_enum.hpp>
instead.'
17 | BOOST_HEADER_DEPRECATED("<boost/core/scoped_enum.hpp>")
| ^~~~~~~~~~~~~~~~~~~~~~~
The default toolchain on Ubuntu 22.10 comes with a slightly more pedantic header
layout:
[ 87%] Building CXX object tests/CMakeFiles/test_common.dir/svg_element.cpp.o
tests/svg_element.cpp: In member function 'SVG::SVGRect
SVG::SVGElement::outline_bbox(bool)':
tests/svg_element.cpp:287:29: error: 'all_of' is not a member of 'std'
287 | auto is_vertical = std::all_of(
| ^~~~~~
tests/svg_element.cpp:290:31: error: 'all_of' is not a member of 'std'
290 | auto is_horizontal = std::all_of(
| ^~~~~~
remove 'pointfof' and replace with aggregate initialization
Now that Graphviz is compiled with C99, there does not seem to be much advantage
in retaining this helper function. We have initialization syntax that is the
same number of characters and can be understood locally without having to lookup
the definition of `pointfof`.
unconditionally use 'deflateBound' when zlib is enabled
When Graphviz was built with support for zlib-based compression, it would only
use `deflateBound` if it was discovered at build time. The `deflateBound` code
path provides a tighter estimate of final compressed size, allowing compression
to minimize its intermediate heap allocations.
The CMake build system did not attempt to discover the presence of
`deflateBound` (config-cmake.in). The Windows build hard coded `deflateBound` as
unavailable (windows/include/config.h). So both these builds would result in a
Graphviz that would use more memory than necessary when writing compressed
output formats.
`deflateBound` arrived in zlib 1.2.0, released on 2003-03-09. It seems safe to
unconditionally assume its existence in 2022. The estimate `deflateBound`
provides has improved in successive releases, with even the latest zlib 1.2.13
released on 2022-10-13 further tightening its estimate. So it is expected that
Graphviz users, with this change, will get lower memory usage during writing
compressed formats, and then lower still as newer zlib releases propagate
through the ecosystem.
projects / graphviz / log