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

docs: make documentation reproducible #24

Merged
merged 5 commits into from Aug 6, 2022
Merged

docs: make documentation reproducible #24

merged 5 commits into from Aug 6, 2022

Conversation

asarhaddon
Copy link
Contributor

Sphinx fixes typographical errors, or does not, depending on the
current locale, preventing reproducible builds.
This suggestion makes the output more deterministic.

It avoids introducing non-ASCII characters, see
#23.

@t-14 t-14 requested a review from fedor-rybin May 31, 2022 09:27
@t-14
Copy link
Collaborator

t-14 commented May 31, 2022

Thanks, we'll check internally if we have any issue with our doc generation.

Can you clarify what is the original motivation? I am a bit puzzled why is having doc deterministic across build systems (essentially, how ellipsis is rendered) important to you?

@t-14
Copy link
Collaborator

t-14 commented Jul 23, 2022

Hi. Discussing this a bit further, we don't think that making such modifications in the document source goes in the right direction. Forbidding "doesn't" and "isn't" is certainly going too far. Can you give us a bit more context, such as, which doc formats are causing problems to you with respect to reproducibility? We can e.g. consider turning off "smartquotes" in text formats like txt/html if that helps?

Copy link
Collaborator

@t-14 t-14 left a comment

Choose a reason for hiding this comment

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

This is not practical as suggested.

A comma before an ellipsis is common in mathematical typography but
quite rare in english.
The ReStructured Text variant used by sphinxdoc recommends two
consecutive backquotes for raw text.  Single backquote or double
quotes sometimes work, but interact poorly with each other.
@asarhaddon
Copy link
Contributor Author

Hello.

I have updated and split the suggestion.

  • The two first commits fix cosmetic, unrelated bugs. I expect them to be consensual.
  • Last commit is a suggestion inspired by Improve punctuation in documentation #23. If you want .rst sources to contain only ASCII characters, you may move the non-ASCII character to a separate Ada source.
  • The original suggestion is now isolated in the third commit.

We would like all build results reproducible, for easyer comparison after two builds, but also because nowadays docs often contains executable code.

The original issue with xmlada is described at sphinx-doc/sphinx#9778. One could argue that the bug is in the smartquote module, but the module is not, and probably cannot be, fully specified. Even its documentation recommends to use the proper character directly when possible.

Disabling smartquotes in all formats (not only text and html) would ensure reproductibility, but break the typography. Using the proper character, via either via UTF8 sources or RST substitutions, gives both.

@t-14
Copy link
Collaborator

t-14 commented Jul 25, 2022

The original suggestion is now isolated in the third commit

While the first two bullet points on the list are consensual, this one ("insert unicode characters when smartquote fails") is really controversial, as it basically requires people to stop writing the doc in natural English and start writing it in some sort of pseudocode. This is not going to work well for us unfortunately. Is this really the only practical recourse for any project trying to use sphinx to generate its docs?!

@asarhaddon
Copy link
Contributor Author

There seem to be no easy fix for the reproducibility issue.
I suggest to check/apply the five unrelated cosmetic improvements found on the way, then close this merge request.
Thanks for your answers.

@adacore-bot adacore-bot merged commit c2a843e into AdaCore:master Aug 6, 2022
@t-14
Copy link
Collaborator

t-14 commented Aug 6, 2022

Thank you for your contribution! Sorry we couldn't find a fully satisfactory solution :(

@asarhaddon
Copy link
Contributor Author

asarhaddon commented Oct 11, 2022 via email

@t-14
Copy link
Collaborator

t-14 commented Oct 11, 2022

Yes, but I want to point out that there are three conditions in the "How" part -

First, the build system needs to be made entirely deterministic: transforming a given source must always create the same result. For example, the current date and time must not be recorded and output always has to be written in the same order.

Second, the set of tools used to perform the build and more generally the build environment should either be recorded or pre-defined.

Third, users should be given a way to recreate a close enough build environment, perform the build process, and validate that the output matches the original build.

Emphasis on the second point mine. I.e. this initiative doesn't expect the builds to be reproducible when the build environment is not controlled. Whereas your patch attempts to remove the constraint on the build environment by constraining the project itself instead. It looks to me therefore not in the spirit of this initiative.

@t-14
Copy link
Collaborator

t-14 commented Oct 11, 2022

(I assume we all agree that difference in how ellipsis is rendered is fully due to the difference of the build environment?)

@asarhaddon
Copy link
Contributor Author

Sorry for the delay, I am not at ease with github workflows and have lost track of this discussion. I thought merged PR could not be discussed afterwards.
You had already answered my concerns. Sorry if my poor english wording has given a different impression.
My suggestion was working around an inconsistency in the smartquote module (which does not fully specify how it depends on the build context). There is no problem if we cannot work around this in xmlada.
Thank you for your constructive answer, and again, sorry for not answering sooner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants