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

Ensure the graphviz filenames are reproducible #6028

Closed
wants to merge 1 commit into
base: 1.8
from

Conversation

Projects
None yet
3 participants
@lamby
Copy link
Contributor

lamby commented Feb 6, 2019

Whilst working on the Reproducible Builds effort, we noticed that sphinx could generate output that is not reproducible.

In particular, the graphviz extension module would construct filenames based on, inter alia, the contents of the options dictionary.

As this contained the absolute build path of the source file embedded in the docname variable this meant that builds of documentation were not independent of where on a filesystem they were built from

Example filenames might be:

  -  html/_images/graphviz-9e71e0f9ba91d0842b51211b676ec4adb7e7afb8.png
  +  html/_images/graphviz-6241bbfd7ac6bd4e2ad9af451ab0dfb8719988d2.png

We fix this by limiting how much of the docname variable ends up in the final constructed filename; I assume there is a good reason for including the options dictionary in the first place, otherwise we could simply omit it.

This was originally filed in @Debian as #921513.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 6, 2019

Codecov Report

Merging #6028 into master will increase coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6028      +/-   ##
==========================================
+ Coverage   82.91%   82.93%   +0.02%     
==========================================
  Files         271      269       -2     
  Lines       39086    39054      -32     
  Branches     5847     5844       -3     
==========================================
- Hits        32408    32391      -17     
+ Misses       5330     5318      -12     
+ Partials     1348     1345       -3
Impacted Files Coverage Δ
sphinx/ext/graphviz.py 32.41% <0%> (-0.26%) ⬇️
sphinx/__init__.py
sphinx/make_mode.py

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 dc0120b...1a4362b. Read the comment docs.

@tk0miya

This comment has been minimized.

Copy link
Member

tk0miya commented Feb 6, 2019

Hmm, it seems the docname was wrong.
We have to replace current_source by self.env.docname like 00f4760 did.

@tk0miya tk0miya added this to the 1.8.5 milestone Feb 6, 2019

@tk0miya

This comment has been minimized.

Copy link
Member

tk0miya commented Feb 6, 2019

Note: this is a bugfix. so please rebase this to 1.8 branch. then I will merge this to 1.8.5 release.
Thanks,

@lamby lamby force-pushed the lamby:reproducible-graphviz branch from f6196ea to 9db1ac3 Feb 6, 2019

Ensure the graphviz filenames are reproducible.
Whilst working on the Reproducible Builds effort [0], we noticed
that sphinx could generate output that is not reproducible.

In particular, the graphviz extension module would construct
filenames based on, inter alia, the contents of the `options`
dictionary.

As this contained the absolute build path of the source file
embedded in the `docname` variable this meant that builds of
documentation were not independent of where on a filesystem they
were built from.

Example filenames might be:

  -  html/_images/graphviz-9e71e0f9ba91d0842b51211b676ec4adb7e7afb8.png
  +  html/_images/graphviz-6241bbfd7ac6bd4e2ad9af451ab0dfb8719988d2.png

We fix this by limiting how much of the `docname` variable ends up
in the final constructed filename; I assume there is a good reason
for including the `options` dictionary in the first place, otherwise
we could simply omit it.

  [0] https://reproducible-builds.org

@lamby lamby force-pushed the lamby:reproducible-graphviz branch from 9db1ac3 to 1a4362b Feb 6, 2019

@lamby

This comment has been minimized.

Copy link
Contributor Author

lamby commented Feb 6, 2019

Rebased against 1.8 branch.

@mitya57

This comment has been minimized.

Copy link
Contributor

mitya57 commented Feb 7, 2019

@lamby Please also edit this pull request and change target branch from master to 1.8.

@lamby lamby changed the base branch from master to 1.8 Feb 7, 2019

@lamby

This comment has been minimized.

Copy link
Contributor Author

lamby commented Feb 7, 2019

Ah, thanks. No wonder it was still saying "conflict"...

@mitya57

This comment has been minimized.

Copy link
Contributor

mitya57 commented Feb 17, 2019

We fix this by limiting how much of the docname variable ends up in the final constructed filename; I assume there is a good reason for including the options dictionary in the first place, otherwise we could simply omit it.

I want to note that your patch actually omits all options except the filename. Though in my opinion it is fine, as the resulting image depends only on source file name and graphviz arguments, which are present in the hash anyway.

@lamby

This comment has been minimized.

Copy link
Contributor Author

lamby commented Feb 17, 2019

the resulting image depends only on source file name

Nod. Note that it cannot include include the absolute path to the source file name, otherwise it would be unreproducible as before. ;-)

@tk0miya

This comment has been minimized.

Copy link
Member

tk0miya commented Feb 22, 2019

As I commented above, the value of docname must be wrong. So this is not only "not reproducible", but also a bug.
So we need to fix the value itself. self.env.docname is more appropriate for this case.

node['options'] = {
'docname': path.splitext(self.state.document.current_source)[0],
}

@lamby

This comment has been minimized.

Copy link
Contributor Author

lamby commented Feb 22, 2019

As I commented above, the value of docname must be wrong.

Don't follow that, sorry...

@tk0miya tk0miya closed this in 49bd372 Feb 27, 2019

tk0miya added a commit that referenced this pull request Feb 27, 2019

Merge pull request #6111 from tk0miya/6028_make_graphviz_reproducible
Fix #6028: graphviz: Ensure the graphviz filenames are reproducible
@tk0miya

This comment has been minimized.

Copy link
Member

tk0miya commented Feb 27, 2019

Merged #6111 instead.
Thank you for reporting :-)

tk0miya added a commit that referenced this pull request Feb 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.