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

Remove non-determinism #1694

Merged
merged 1 commit into from Jan 28, 2015
Merged

Remove non-determinism #1694

merged 1 commit into from Jan 28, 2015

Conversation

mitya57
Copy link
Contributor

@mitya57 mitya57 commented Jan 28, 2015

To enable packages using Sphinx to build reproducibly, its output needs to be the same from one build to another.

Its output now strips memory references such as:

<__main__.A at 0x7f68cb685710>

In addition, various generated files (objects.inv, searchindex.js, translations) are now written with their keys in a determinstic order.

Initial patch by @lamby, rebased against master by @mitya57.

@@ -268,7 +268,8 @@ def prepare_writing(self, docnames):
# html_domain_indices can be False/True or a list of index names
indices_config = self.config.html_domain_indices
if indices_config:
for domain in itervalues(self.env.domains):
for domain_name in sorted(self.env.domains.keys()):
Copy link
Member

Choose a reason for hiding this comment

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

sorted(self.env.domains) should suffice.

@birkenfeld
Copy link
Member

Good idea, apart from removing non-determinism it also makes the repr's much more readable.

@mitya57 mitya57 force-pushed the master branch 3 times, most recently from e361416 to b47a0b3 Compare January 28, 2015 18:22
@mitya57
Copy link
Contributor Author

mitya57 commented Jan 28, 2015

Updated.

To enable packages using Sphinx to build reproducibly, its output
needs to be the same from one build to another.

Its output now strips memory references such as:

  <__main__.A at 0x7f68cb685710>

In addition, various generated files (objects.inv, searchindex.js,
translations) are now written with their keys in a determinstic order.

Based on a patch by Chris Lamb <lamby@debian.org>.
@birkenfeld
Copy link
Member

LGTM, just waiting for the tests to complete.

birkenfeld added a commit that referenced this pull request Jan 28, 2015
@birkenfeld birkenfeld merged commit 6835224 into sphinx-doc:master Jan 28, 2015
@lamby
Copy link
Contributor

lamby commented Jan 28, 2015

Thanks for merging and apologies for some of the—err—rough edges in the original patch. Mea culpa.

On those lines, I think we are actually missing something. For example, take a look here:

https://reproducible.debian.net/rb-pkg/sphinx.html

... and grep for "genindex.html". Note that the command line option ordering is different (-q vs -Q). I had a quick glance at the Sphinx source to work out where this could be remedied but it was not immediately forthcoming.. More than happy to look further into this if nobody else immediately knows.

@lamby
Copy link
Contributor

lamby commented Jan 28, 2015

Did anyone have any thoughts re. making the regex "safer"? I don't like how it's applied to every output of repr, but wasn't particularly convinced by a whitelist of isinstance params when I tried that..

@birkenfeld
Copy link
Member

I think it is quite safe. We're not generating code here, so the very unlikely false positive will be much less important than the better readability for 99%+ of instances.

@lehmannro
Copy link
Contributor

If you wanted to limit it you could only apply memory_address_re when type(obj).__repr__ is object.__repr__. This falls down when you're handling collections which recursively call repr (eg. lists) or user-defined objects which indeed include id(self) somewhere.

@mitya57
Copy link
Contributor Author

mitya57 commented Jan 30, 2015

@lamby The code that generates index is in create_index method in sphinx/environment.py. The code that writes it into genindex.html file is in write_genindex method in sphinx/builders/html.py. Hope that helps.

(For everyone else: the diff @lamby was referring to is here).

lamby added a commit to lamby/sphinx that referenced this pull request Jan 31, 2015
See also <sphinx-doc#1694>

Signed-off-by: Chris Lamb <chris@chris-lamb.co.uk>
lamby added a commit to lamby/sphinx that referenced this pull request Jan 31, 2015
See also <sphinx-doc#1694>

Signed-off-by: Chris Lamb <chris@chris-lamb.co.uk>
shimizukawa added a commit that referenced this pull request Mar 9, 2015
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants