Skip to content

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.

4 participants