Skip to content

Fix docs build with Sphinx 1.6#8515

Merged
timgraham merged 1 commit intodjango:masterfrom
mitya57:master
May 24, 2017
Merged

Fix docs build with Sphinx 1.6#8515
timgraham merged 1 commit intodjango:masterfrom
mitya57:master

Conversation

@mitya57
Copy link
Copy Markdown
Contributor

@mitya57 mitya57 commented May 17, 2017

I was the author of sphinx-doc/sphinx#3666, which broke Sphinx docs build. This pull request should fix the build errors and warnings.

With it, #8514 should be also not needed.

@mitya57 mitya57 mentioned this pull request May 17, 2017
Merged
@timgraham timgraham mentioned this pull request May 17, 2017
Closed
timgraham
timgraham reviewed May 17, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@timgraham timgraham left a comment

Choose a reason for hiding this comment

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

sphinx 1.6+ doesn't render the smart quotes now -- can you add the needed docutils.conf?

docs/_ext/djangodocs.py Outdated
Copy link
Copy Markdown
Member

@timgraham timgraham May 17, 2017

Choose a reason for hiding this comment

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

I think we can use super() here?

docs/_ext/djangodocs.py Outdated
Copy link
Copy Markdown
Member

@timgraham timgraham May 17, 2017

Choose a reason for hiding this comment

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

# Sphinx 1.6+ is enough

@mitya57
Copy link
Copy Markdown
Contributor Author

mitya57 commented May 17, 2017

Sphinx 1.6.1 does render the smart quotes for me. Here is the screenshot from index.html:
index_html

The explicit entry in docutils.conf is needed only when you want to use a custom set of quotes.

@timgraham
Copy link
Copy Markdown
Member

timgraham commented May 17, 2017

Hm, I don't see smart quotes in that location with your branch and sphinx 1.5.3 or 1.6.1. If you can't reproduce, I'll debug it here.

@timgraham timgraham force-pushed the master branch 2 times, most recently from c164997 to d30065b Compare May 18, 2017 12:44
@timgraham
Copy link
Copy Markdown
Member

timgraham commented May 18, 2017 •
edited
Loading

Maybe you have a ~/.docutils.conf on your system that enables smart quotes? The default value is "no" so I don't see how this could work without it. I add a project docutils configuration file to this PR that enables smart quotes and it's working for me except that the spelling checker now complains about all words that have contractions in them: isn't, doesn't, etc. (https://bitbucket.org/dhellmann/sphinxcontrib-spelling/issues/13/with-sphinx-161-contractions-result-in)

To avoid blocking merging other PRs in the meantime, I changed the docs pull request builder to use sphinx 1.5.6.

@timgraham timgraham mentioned this pull request May 18, 2017
Closed
@mitya57
Copy link
Copy Markdown
Contributor Author

mitya57 commented May 19, 2017 •
edited
Loading

No, I ran in a clean environment without ~/.docutils.conf. The default value in docutils is indeed False, but Sphinx 1.6 overrides it to True by default. If you for some reason do not see the quotes, please look at that line in Sphinx code and see what is wrong there — maybe bad language code, or old docutils version?

@timgraham
Copy link
Copy Markdown
Member

timgraham commented May 19, 2017

Smart quotes don't render on my system because the language is "en-US" and that's not in the docutils.utils.smartchars.quotes dict.

@mitya57
Copy link
Copy Markdown
Contributor Author

mitya57 commented May 19, 2017

Where does that come from? en-US is not a valid language name, a valid name would be just en. And language is not set in conf.py.

@timgraham
Copy link
Copy Markdown
Member

timgraham commented May 19, 2017

Maybe from environment variables?

$ echo $LANG
en_US.UTF-8

I haven't looked into the sphinx source to figure it out.

@mitya57
Copy link
Copy Markdown
Contributor Author

mitya57 commented May 19, 2017

Ok, in the Travis log I found -D language=en_US. And Makefile passes this argument based on $LANGUAGE, so you probably have it set too (I have no such environment variable).

I believe the best thing to do would be to fix the Makefile.

@mitya57
Copy link
Copy Markdown
Contributor Author

mitya57 commented May 19, 2017

The last commit should be fine to merge.

timgraham
timgraham reviewed May 22, 2017
View reviewed changes
docs/Makefile Outdated
Copy link
Copy Markdown
Member

@timgraham timgraham May 22, 2017

Choose a reason for hiding this comment

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

"We need to" can omitted.

timgraham
timgraham reviewed May 22, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@timgraham timgraham left a comment

Choose a reason for hiding this comment

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

When I try to build the docs using make html, I get this error:
Makefile:15: *** Recursive variable `LANGUAGE' references itself (eventually).

docs/_ext/djangodocs.py Outdated
Copy link
Copy Markdown
Member

@timgraham timgraham May 22, 2017

Choose a reason for hiding this comment

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

Use super() as master only supports Python 3.

docs/conf.py Outdated
Copy link
Copy Markdown
Member

@timgraham timgraham May 22, 2017

Choose a reason for hiding this comment

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

Workaround is one word, no dash needed.

@mitya57 mitya57 force-pushed the master branch 2 times, most recently from c317843 to d0a2c39 Compare May 22, 2017 19:01
@mitya57
Copy link
Copy Markdown
Contributor Author

mitya57 commented May 22, 2017

@timgraham Fixed!

@timgraham
Copy link
Copy Markdown
Member

timgraham commented May 23, 2017

I see smartquotes with sphinx 1.6 but not with 1.5.6. Do you see the same?

@mitya57
Copy link
Copy Markdown
Contributor Author

mitya57 commented May 23, 2017

The first version of this PR supported Sphinx 1.5, but then you told me that the support can be dropped…

@pope1ni
Copy link
Copy Markdown

pope1ni commented May 23, 2017

@mitya57: I think that you misinterpreted what @timgraham was saying when he said:

# Sphinx 1.6+ is enough

He meant replace the comment, not remove support for Sphinx < 1.6

-except ImportError:  # SmartyPantsHTMLTranslator was removed in Sphinx 1.6
+except ImportError:  # Sphinx 1.6+

@timgraham
Copy link
Copy Markdown
Member

timgraham commented May 24, 2017

Correct, sorry for the confusion.

@mitya57
Copy link
Copy Markdown
Contributor Author

mitya57 commented May 24, 2017

Sorry, I missed the hash mark and got confused. Restored compatibility with 1.5 now.

@mitya57
Copy link
Copy Markdown
Contributor Author

mitya57 commented May 24, 2017

(Why does Jenkins try to check commit 472c49270f41355d124eaabb90674a9fd2ba358b instead of 0642bea76181f2dbf8e9edcb87b2090500b57331 I just pushed?)

@timgraham timgraham merged commit f370bfb into django:master May 24, 2017
@melvyn-sopacua
Copy link
Copy Markdown
Contributor

melvyn-sopacua commented Jul 5, 2017

Please note that sphinx 1.6 now requires sphinxcontrib-websupport and that requests is unbundled and also required. Should that go somewhere? setup.py docs_require? Release notes?

@timgraham
Copy link
Copy Markdown
Member

timgraham commented Jul 5, 2017

pip install sphinx installs the required dependencies as far as I see:

...
Successfully installed Jinja2-2.9.6 MarkupSafe-1.0 Pygments-2.2.0 alabaster-0.7.10
babel-2.4.0 certifi-2017.4.17 chardet-3.0.4 docutils-0.13.1 idna-2.5 imagesize-0.7.1
pytz-2017.2 requests-2.18.1 six-1.10.0 snowballstemmer-1.2.1 sphinx-1.6.3
sphinxcontrib-websupport-1.0.1 typing-3.6.1 urllib3-1.21.1

@melvyn-sopacua
Copy link
Copy Markdown
Contributor

melvyn-sopacua commented Jul 25, 2017

Yep, operator error. Didn't upgrade the port properly. Sorry for the noise.

danielhjames added a commit to danielhjames/Booktype that referenced this pull request Apr 24, 2018
@danielhjames
Update booktypedocs.py
6d7e928 
Readthedocs.io now runs Sphinx 1.6.x so our docs build fails. This config change is required, please see django/django#8515
@danielhjames danielhjames mentioned this pull request Apr 24, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@timgraham timgraham timgraham left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mitya57 @timgraham @pope1ni @melvyn-sopacua