Rob Hart [Sat, 19 Dec 2020 22:14:22 +0000 (14:14 -0800)]
Add regression test
Edit by Matthew Fernandez: moved this commit ahead of the one that fixes the bug
and marked the test case as expected to fail. This allows us to more accurately
confirm the bug exists and its fix works as intended, while still leaving the
commit series bisectable.
This reverts commits 429718cb387092b5bf81850ac97d18ca34149055 but with a
slightly different variant of the macro hack that avoids subsequent sed
replacement steps. I had several false starts at this fix, so let's detail what
did not work:
1. We cannot implement an always-no-answering isatty() locally or redirect
isatty() to another function, because Flex calls isatty(fileno(f)) and our
"FILE" from SFIO does not provide this. On operating systems where fileno()
references an offset in the FILE struct (e.g. macOS) this causes a
segfault. So we need to suppress the entire argument to isatty(), not just
the call to it.
2. Versions of Flex prior to 2.5.39 emit a spurious extern declaration [0]. If
we reverted 429718cb387092b5bf81850ac97d18ca34149055 as-is, we would have
also had to revert 3b00c1fc6b949cc9744d075e0d2b2ed4b1c46763. However, even
this would not be enough because we now support a CMake build, so we would
have to introduce a corresponding find-and-replace step in
lib/cgraph/CMakeLists.txt. Rather than do this, I came up with a different
macro replacement that still makes isatty() calls evaluate to 0 but does
not require other find-and-replace steps.
3. Although Flex 2.5.39 was released almost a decade ago, we cannot easily
drop support for prior versions of Flex. The latest version of Apple XCode
ships Flex 2.5.35 and is unlikely to upgrade any time soon. This means most
macOS users have Flex 2.5.35 without realizing it. Compounding the problem,
the files generated by Flex #include headers that ship with Flex, so even
macOS users who have a newer version of Flex via Homebrew/Macports may
accidentally have the Flex 2.5.35 headers in their include path. The path
of least irritation to our downstream macOS users appear to be retaining
Flex 2.5.35 support.
Fixes #1910. The underlying problem here still exists: with `never-interactive`
set in the lib/cgraph/scan.l lexer, we are somehow not resetting internal state
in between lexer invocations. I suspect this will return as an issue in future
and we will have to more thoroughly debug this.
revert changes to add non-local names to internal map
Merge Request !1489 made a change to which names were stored in the internal
map. Following this, non-local names (user-provided ones; not starting with '%')
were stored in the internal map as well as local names. This inadvertently broke
some fdp and circo assumptions (#1876, #1877, !1676).
fix: require exactly \d+.\d+.\d+ for a release version
This regex was incorrectly checking for anything *starting* with this pattern.
As a result, versions like "2.44.2~dev.20210109.1932" would pass this check.
Related to #1892.
fix: handle --version not being passed to deployment script
b443b70a828a34c79c71e41af9cb2eab7b950a14 incorrectly did not anticipate neither
the --force nor --version flags being passed to the deployment script.
Unfortunately I failed to notice this because I was testing using --force and
there was no easy way to test the final work flow until merging to master.
There is also no easy way to test this commit before being merged. I have run
pylint, but after that just need to cross my fingers.
rewrite deployment CI task to create releases on Gitlab
Prior to this commit, the deployment CI task runs on a private runner and
uploads release artifacts to www2.graphviz.org. The SSH steps in the deployment
task are currently failing due to connectivity issues. The current maintainers
are unable to fix this as we do not have enough knowledge of how this work flow
was intended to function.
Apart from these concerns, Gitlab have recently introduced support for so-called
“generic packages” [0] to host binary artifacts directly on Gitlab (or its
underlying CDN) as well as the ability to programmatically create releases [1]
on the releases page of a project. The decision was made to simplify the
Graphviz infrastructure and host future releases (both stable and development)
on Gitlab.
This change rewrites the deployment task to (1) run on the Gitlab shared runner
using a release-cli Docker image, (2) upload package artifacts to Graphviz'
generic package, and (3) create a release from this for stable version numbers.
To comprehend the role the generic package container is playing here, note that
generic package version numbers are synthesized from commit SHA and are not
intended to be exposed to users. Users can browse the generic package page [2],
but are only ever expected to be interested in the latest version available
here. In contrast, version numbers on the release page [3] are intended to
correspond to Graphviz stable version numbers. ci/deploy.py handles mapping a
release on this page to its related (synthesized) generic package version.
Fix missing return NULL and remove 1 -Wunused-but-set-variable warning in mmio.c
Fixes https://gitlab.com/graphviz/graphviz/-/issues/1875 although this
problem does not currently manifest itself since the
mm_typecode_to_str function is only used in functions writing Matrix
Market files which none of the Graphviz tools currently does.
Most certainly the unitialized values were not intended to used by the
callers since the return value of the function in this case is set to
a "large" value to indicate failure and the callers have assertions
that seems to guard from using such values by mistake (although a
thorough understanding of the algorithm and minimum ink values are
needed to prove this).
Altough the original author seems to have intended to set them to the
mid point, perhaps as a precautionary measure, the assigment has been
removed and new assertions has been added to the relevant call sites
to garantee that they are never used.
Correct assertion and remove 1 -Wparentheses warning in BinaryHeap.c
Note the operator is changed from assigment to *inequality*. From the
surrounding code it's apparent that the intent was to check that no -1
values are left in the mask array (equality to 1 would also have been
correct).
fix: anticipate no non-normal edges when computing direction with concentrate
With concentrate enabled (`concentrate=true`), the direction of edges is
analyzed in lib/dotgen/conc.c. The loops in `samedir()` did not anticipate that
they may not find a single non-normal edge before hitting the end of an edge
chain. This change removes this assumption.
When using fdp to process a graph, it would try to canonicalize all node names,
regardless of whether they were hosted in refstr_ts or not. The problem with
this is that the canonicalization logic assumes the character pointer passed
into it is within a refstr_t and that therefore it can do pointer subtraction to
get a pointer to the refstr_t itself. This was not true for internally
constructed node names like "%4".
We change this to always go through the refstr logic in agstrdup() when
canonicalizing names. This ensures that the pointer we pass into the
canonicalization logic *is* within a refstr_t. This is based on canon() in
lib/cgraph/output.c. Fixes #236, #1290, #1865.
projects / graphviz / log