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 from

Conversation

lamby
Copy link
Contributor

@lamby 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
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
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
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,

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
Copy link
Contributor Author

lamby commented Feb 6, 2019

Rebased against 1.8 branch.

@mitya57
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 February 7, 2019 12:33
@lamby
Copy link
Contributor Author

lamby commented Feb 7, 2019

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

@mitya57
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
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
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
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
Fix #6028: graphviz: Ensure the graphviz filenames are reproducible
@tk0miya
Copy link
Member

tk0miya commented Feb 27, 2019

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants