Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix gvRenderData bytes output #424

Merged
merged 6 commits into from
Jul 10, 2022

Conversation

Nagael
Copy link
Contributor

@Nagael Nagael commented Jul 5, 2022

This PR fixes the AGraph._draw(format='png') case. The PNG format can contain NULL bytes, so this call could result in an assertion error:

File "[...]/pygraphviz/agraph.py", line 1729, in _draw
    assert len(dot_string) == length
AssertionError

This fix correctly uses SWIG's %cstring_output_allocate_size(parm, szparm, release) instead of %cstring_output_allocate(parm, release) (as explained in 11.3.4 of https://www.swig.org/Doc4.0/Library.html)

Unfortunately my local SWIG installation is 4.0.1, so the regenerated SWIG files are more different than strictly necessary. The first commit is the main one, though.

…correct result even if the output contains NULL bytes

For example, this allows AGraph._draw() to return a correct result with the "png" format
My local SWIG installation is 4.0.1, so this contains additional modifications.
This also required an additional compile variable in setup.py. It is probably not necessary if the files are regenerated with SWIG 4.0.2 again.
Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Nagael , this seems good. The one missing piece is a test that illustrates the failure; i.e. something that fails with the current code that passes with this patch. Do you have any example cases where you hit this problem that could be added to the test suite?

@Nagael
Copy link
Contributor Author

Nagael commented Jul 8, 2022

Thanks for your answer. I just added a commit with a test that fails on the current version. It asks for the png format, and at first I thought that the test should not assume that this format is available. But since other tests also use this format, I removed the check in a second commit. You can use whichever version is more convenient for the project.

@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2022

Codecov Report

Merging #424 (1dcb195) into main (8d15432) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #424      +/-   ##
==========================================
- Coverage   80.87%   80.86%   -0.02%     
==========================================
  Files           5        5              
  Lines        1255     1254       -1     
==========================================
- Hits         1015     1014       -1     
  Misses        240      240              
Impacted Files Coverage Δ
pygraphviz/agraph.py 81.29% <100.00%> (-0.02%) ⬇️
pygraphviz/graphviz.py 76.53% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d15432...1dcb195. Read the comment docs.

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @Nagael . I went ahead and re-generated graphviz.py and graphviz_wrap.c with SWIG 4.0.2. I'm not too familiar with SWIG, so if you wouldn't mind taking a quick look at the generated graphviz_wrap.c code to verify that everything looks as expected that'd be great.

I also went ahead and renamed/added a comment to the test so we have a better idea of what it's designed to catch. If you have any wording/name changes you think would be better, please go ahead and make them. Other than that, this LGTM! We always squash-merge so no need to worry about cleaning up the history.

@Nagael
Copy link
Contributor Author

Nagael commented Jul 10, 2022

Thanks @rossbar, everything looks just as expected and works smoothly. Looks good to me too!

@rossbar
Copy link
Contributor

rossbar commented Jul 10, 2022

Great, then in this goes (the failing CI is a known issue). Thanks again @Nagael !

@rossbar rossbar merged commit f8f2061 into pygraphviz:main Jul 10, 2022
@jarrodmillman jarrodmillman added this to the 1.10 milestone Aug 12, 2022
rossbar added a commit to rossbar/pygraphviz that referenced this pull request Oct 3, 2023
* Bugfix: correctly use the size returned by gvRenderData, returning a correct result even if the output contains NULL bytes

For example, this allows AGraph._draw() to return a correct result with the "png" format

* Regenerate SWIG-generated files.

* Add test demonstrating failure with png output format due to NULL in cstring

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants