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

Fix copying images under parallel execution #11100

Merged
merged 6 commits into from Jan 7, 2023

Conversation

AA-Turner
Copy link
Member

cc @hugovk @CAM-Gerlach; xref python/peps#2950

This (hopefully) fixes copying images when running Sphinx under parallel-mode.

A

Copy link
Contributor

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Testing locally with the PEPs repo (Python 3.11 on macOS), this fixes the image copy problem from python/peps#2950.

Thanks!

@CAM-Gerlach
Copy link
Contributor

Maybe worth adding some kind of regression test for this?

@AA-Turner AA-Turner merged commit a1cd19e into sphinx-doc:6.1.x Jan 7, 2023
@AA-Turner AA-Turner deleted the images-parallel branch January 7, 2023 17:35
@marxin
Copy link
Contributor

marxin commented Jan 8, 2023

Hey @AA-Turner, I've just updated the openSUSE package to 6.1.2 and I noticed the newly added tests fail under python-3.8:

[  473s] FAILED tests/test_build_epub.py::test_copy_images - AssertionError: assert {'...
[  473s] FAILED tests/test_build_latex.py::test_copy_images - AssertionError: assert {...
[  473s] FAILED tests/test_build_texinfo.py::test_copy_images - AssertionError: assert...
...
[  160s] app = <SphinxTestApp buildername='texinfo'>
[  160s] status = <_io.StringIO object at 0x7ffff346ba60>
[  160s] warning = <_io.StringIO object at 0x7ffff3601ca0>
[  160s] 
[  160s]     @pytest.mark.sphinx('texinfo', testroot='images')
[  160s]     def test_copy_images(app, status, warning):
[  160s]         app.build()
[  160s]     
[  160s]         images_dir = Path(app.outdir) / 'python-figures'
[  160s]         images = {image.name for image in images_dir.rglob('*')}
[  160s] >       assert images == {
[  160s]             'img.gif',
[  160s]             'img.pdf',
[  160s]             'img.png',
[  160s]             'python-logo.png',
[  160s]             'rimg.png',
[  160s]             'rimg1.png',
[  160s]             'svgimg.pdf',
[  160s]             'svgimg.svg',
[  160s]             'testimäge.png',
[  160s]         }
[  160s] E       AssertionError: assert {'img.gif', '...img.pdf', ...} == {'img.gif', '...mg1.png', ...}
[  160s] E         Extra items in the right set:
[  160s] E         'python-logo.png'
[  160s] E         Full diff:
[  160s] E           {
[  160s] E            'img.gif',
[  160s] E            'img.pdf',
[  160s] E            'img.png',...
[  160s] E         
[  160s] E         ...Full output truncated (8 lines hidden), use '-vv' to show

which is exactly the same as what can be seen for Sphinx CI:
https://github.com/sphinx-doc/sphinx/actions/runs/3863082468/jobs/6585053084

Can you please take a look?

@AA-Turner
Copy link
Member Author

There should be nothing Python 3.8-specific about the failures; from CI is when I misconfigured the tests initially.

Do you have pytest output with -vv?

A

@marxin
Copy link
Contributor

marxin commented Jan 8, 2023

Oh, got it. It's caused by fact that our builders don't have an access to the internet:

[  160s] # warning: 
[  160s] WARNING: Could not fetch remote image: https://www.python.org/static/img/python-logo.png [HTTPSConnectionPool(host='www.python.org', port=443): Max retries exceeded with url: /static/img/python-logo.png (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7ffff25840a0>: Failed to establish a new connection: [Errno -3] Temporary failure in name resolution'))]
...

@marxin
Copy link
Contributor

marxin commented Jan 8, 2023

Thus I'm going to disable test_copy_images for the builders and I'm done. Thanks, all is fine.

@CAM-Gerlach
Copy link
Contributor

@AA-Turner I'd suggest making the test more specific and robust by doing a membership (in) test (rather than full equality (==)) on the original expected images you had in 4c8b590 . That way, it's still testing if the full range of expected test images of different types are copied, while not depending on whether python-logo.png can be downloaded, if other images are added later, or there happen to be other files matching for other reasons, etc.

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