[SRU] scaling is wrong in SVG output

[David Huggins-Daines]

[ Impact ]

graphivz output is incorrect for SVG output when the "size" attribute is set, as it implicitly is in several circumstances.

[ Test Plan ]

* sudo apt install imagemagick graphviz
* Grab fst.dot attached to this bug
* dot -Tpng fst.dot | display png:-
* Observe correct scaling
* dot -Tsvg fst.dot | display svg:-
* Observe incorrect cropping of output
* Enable proposed (https://wiki.ubuntu.com/Testing/EnableProposed)
* Upgrade graphviz to version from proposed
* dot -Tsvg fst.dot | display svg:-
* Ensure output is not cropped

[ Regression Potential ]

Low; the patched applied is from upstream and has been part of the official sources for many years now (Debian and Ubuntu are rather behind on their versions of graphviz, but it looks like this may be corrected in the oracular cycle). Nonetheless, the patch was first applied to our current version, is minimal in nature, and fixes the issue in tests without impacting other output.

There is a very small possibility someone may be relying on the incorrect truncation of SVG output, but this is almost certainly outweighed by the number of users impacted by incorrect truncation of output in, e.g. jupyter notebooks, and by the consistency of having the same (lack of) cropping in different output formats.

[ Original Description ]

SVG output brutally truncates large (and even not so large) graphs when the "size" attribute is set.

This was fixed *two years ago* upstream. Any chance that we might get more recent graphviz packages in Ubuntu at some point? Failing that, it is quite easy to fix this bug.

See https://gitlab.com/graphviz/graphviz/-/issues/1605

The fix is very simple, it seems: https://gitlab.com/graphviz/graphviz/-/commit/a5606d101af1cc949908a6f0bc19caaa4eb31159

Definitely present in 22.04, still present in the Lunar package (graphviz_2.42.2-7build3)

[David Huggins-Daines (dhuggins)]

fix here: https://code.launchpad.net/~dhuggins/ubuntu/+source/graphviz/+git/graphviz/+ref/dhd/fix_2015017_2015023

[Dave Jones (waveform)]

This looks reasonable, I'll comment further on the associated merge request.

[David Huggins-Daines (dhuggins)]

I hate to be a jerk but it's been over a year, and I even provided a complete fix and patch.

The fix is literally one line. Somewhere in the distant past, 7 years ago I think, somebody made a typo in Graphviz and committed it.

The year is 2024 and we still can't properly view graphs in Jupyter on Debian and Ubuntu because of this.

Please please please.

[Dave Jones (waveform)]

I quite understand the frustration with something that appears to be abandoned. However, in this case it seems it was actually down to some confusion over the versioning scheme which changed upstream from Debian (https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=980900 has the gory details). It also seems that Debian's now got a new watch file and version 11 is now in experimental, so I'm hoping that'll migrate to unstable at some point and we'll pick up a modern version for oracular.

Anyway, back to the fix at hand -- this definitely needs patching in oracular, noble, mantic, and jammy at the very least. Unfortunately your branch appears to have diff-markers embedded in it, so I can't accept it directly, but the patch is trivial enough I can just apply it to the package directly. I'll target this to the affected releases and start the SRU process once this is in oracular.

[Dave Jones (waveform)]

For SRU purposes, is there any chance I could ask you for a simple reproducer case? It's okay if it uses jupyter, but I'm struggling to come up with one without it and I've run out of time this evening.

[David Huggins-Daines (dhuggins)]

Sure, no problem at all! Thanks for the gracious reply, sorry about my
frustration this morning.

You can see the problem with this dotfile by running these commands - the
figure will obviously be clipped (it should have 9 nodes but you can only
see 6):

dot -Tsvg fst.dot
open fst.svg

I've attached the resulting SVG file as well. Note also that if you render
to PDF, PNG, etc, you won't see the clipping.

[Dave Jones (waveform)]

Brilliant, thanks very much for that -- I've upload the ocular version to proposed, and I'll get the SRU uploads done later this morning.

[David Huggins-Daines (dhuggins)]

Thank you, thank you! Appreciate the explanation of the long delay - I think one of the graphviz maintainers said something like "graphviz has gone through multiple different versioning schemes, all of them confusing" and the 2.42.4 release happened right around the point when they stopped explicitly saying "no this one is really actually a release".

I'll sign up for some software anger management in the future :)

[Mauricio Faria de Oliveira (mfo)]

Hi Dave (~waveform),

Thanks for all the work to review/sponsor this fix! (And several others!)
It's really appreciated.

In a comparatively small but relevant note, I'd like to humbly point out though
that some DEP-3 headers seem incorrect (IMHO) in the uploads AFAICT based in [1]
(section `Standard fields`):

+Author: David Huggins-Daines <email address hidden>
+Bug: https://gitlab.com/graphviz/graphviz/-/commit/a5606d101af1cc949908a6f0bc19caaa4eb31159
+Description: Corrects scale factor in SVG output

These headers are totally understandable with the context in this bug,
but it might not always be available when looking at it in the future.

If I understand it correctly:

`Author` isn't David (who proposed the patch; so, credit is certainly due, likely in the changelog, e.g., '[ Name ]' before the entry), but the upstream patch author
> Author or From (optional): This field can be used to record the name and email of the patch author ...

`Bug` should be an upstream bug tracker, not the commit/patch:
> Bug-<Vendor> or Bug (optional): It contains one URL pointing to the related bug ...
> The Bug field is reserved for the bug URL in the upstream bug tracker.
> Those fields can be used multiple times ...
> The vendor name is explicitely encoded in the field name"

`Description` is OK, but looks more like `Subject`, as it's single line.
(BTW, this reads way better than the upstream commit title! :)

I think the correct form should be, starting from `git format-patch` or the `.patch`-suffixed URL [2],
which already gives us:
`From:`, which replaces `Author:` ("Author or From (optional)")
`Subject:`, which replaces `Description:` with a different/unstructured format ("Description or Subject (required)")

And we would add 2 optional fields, but which are highly useful (and more standard, considering most SRUs I've seen so far).

Bug-Ubuntu: http://bugs.launchpad.net/bugs/2015017
Origin: [<upstream|backport|vendor>,] https://gitlab.com/graphviz/graphviz/-/commit/a5606d101af1cc949908a6f0bc19caaa4eb31159

[1] https://dep-team.pages.debian.net/deps/dep3/
[2] https://gitlab.com/graphviz/graphviz/-/commit/a5606d101af1cc949908a6f0bc19caaa4eb31159.patch
[3] https://ubuntu-archive-team.ubuntu.com/proposed-migration/oracular/update_excuses.html#graphviz

[Mauricio Faria de Oliveira (mfo)]

To be clear, I don't think these should block the acceptance of this SRU
(I have seen different criteria to accept SRUs regarding DEP-3 headers),
but hopefully might be of some value for future SRUs, if you agree w/ it.

I'll refrain from accepting it for now as oracular-proposed seems to have
some regressions in autopkgtests [3], so let's see how that goes first.

Thanks again!

[3] https://ubuntu-archive-team.ubuntu.com/proposed-migration/oracular/update_excuses.html#graphviz