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

bpo-34081: Fix wrong example link that was linking to distutils #8248

Merged
merged 5 commits into from Oct 21, 2018

Conversation

tirkarthi
Copy link
Member

@tirkarthi tirkarthi commented Jul 11, 2018

This PR fixes example link in Type Objects where examples was pointing to distutils examples . Distutils also had the same label as example. This was pointed out in the docs CI with the warning but it doesn't cause any error and hence was ignored. I think it will be better to turn on warning as errors in sphinx docs configuration so that these errors are caught in the CI run.

This is present only in master introduced as part of 9e7c921 and doesn't have to be backported.

Thanks

https://bugs.python.org/issue34081

@tirkarthi tirkarthi changed the title bpo34081: Fix wrong example link that was linking to distutils bpo-34081: Fix wrong example link that was linking to distutils Jul 11, 2018
@tirkarthi
Copy link
Member Author

tirkarthi commented Jul 11, 2018

This can be turned on with -W in ALLSPHINXOPTS which gives the following error. Let me know if this needs to be added to the PR since current build is successful without warnings

(venv) ➜  Doc git:(master) ✗ make html
mkdir -p build
Building NEWS from Misc/NEWS.d with blurb
PATH=./venv/bin:$PATH sphinx-build -b html -d build/doctrees -W -D latex_elements.papersize=  . build/html
Running Sphinx v1.7.5
loading pickled environment... done
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 1 source files that are out of date
updating environment: 0 added, 2 changed, 0 removed
reading sources... [100%] whatsnew/changelog

Warning, treated as error:
/home/cpython/Doc/c-api/typeobj.rst:2327:duplicate label examples, other instance in /home/cpython/Doc/distutils/examples.rst
Makefile:43: recipe for target 'build' failed
make: *** [build] Error 2

@tirkarthi
Copy link
Member Author

tirkarthi commented Jul 13, 2018

I have added -W to have warnings as errors since it makes more sense in catching these type of errors. Let me know if it needs to be reverted.

Thanks

Copy link
Sponsor Contributor

@BoboTiG BoboTiG left a comment

Perhaps addind a NEWs entry would be a good thing, at least to let others know warnings are now activated.

@tirkarthi
Copy link
Member Author

tirkarthi commented Sep 6, 2018

Sorry that I missed this PR comment somehow. Added a NEWS entry.

Thanks

BoboTiG
BoboTiG approved these changes Sep 6, 2018
Copy link
Sponsor Contributor

@BoboTiG BoboTiG left a comment

Thank you for the patch :)

@tirkarthi
Copy link
Member Author

tirkarthi commented Oct 20, 2018

I think this is outdated now since the link was fixed in bpo-34962 along with doctest in CI but I think it's good to enable this in the makefile for docs so that errors are caught in the future while building docs locally though CI catches them too. @matrixise Thoughts?

@JulienPalard
Copy link
Member

JulienPalard commented Oct 20, 2018

@tirkarthi Hi, this is a good idea, we're already using -W while compiling the french translation.

Would you just remove the now-fixed-fix about examples and keep only the -W? I'll gladly merge this.

@tirkarthi
Copy link
Member Author

tirkarthi commented Oct 20, 2018

@JulienPalard Thanks for the feedback. I have reverted my change. Azure build fails though others pass. I have merged the latest master on my local machine and there doesn't seem to be any error on building locally. Attaching the log for reference.

17.txt

@tirkarthi
Copy link
Member Author

tirkarthi commented Oct 20, 2018

I think my Azure build uses Sphinx v1.6.7 which doesn't contain the directive though Travis uses Sphinx 1.8.1 and passes there. I will try to merge the latest master to see if it helps.

@tirkarthi
Copy link
Member Author

tirkarthi commented Oct 20, 2018

Ah, .azure-pipelines was not updated with the pinned version so there are already warnings in other PRs but since my PR makes warnings as error it fails. I think I get the error now and pin version in Azure to see if it helps :)

@tirkarthi
Copy link
Member Author

tirkarthi commented Oct 20, 2018

@JulienPalard Builds are green now :) You can review the changes.

Copy link
Member

@JulienPalard JulienPalard left a comment

LGTM

@JulienPalard JulienPalard merged commit 121eb16 into python:master Oct 21, 2018
@tirkarthi tirkarthi deleted the fix-bpo34081 branch Oct 22, 2018
yahya-abou-imran pushed a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 2018
@tirkarthi tirkarthi restored the fix-bpo34081 branch Sep 27, 2019
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

5 participants