Skip to content

Maint: Fix app.builder.outdir as Sphinx now using pathlib#1155

Merged
lucyleeow merged 3 commits intosphinx-gallery:masterfrom
lucyleeow:main_sphinxdev
Aug 1, 2023
Merged

Maint: Fix app.builder.outdir as Sphinx now using pathlib#1155
lucyleeow merged 3 commits intosphinx-gallery:masterfrom
lucyleeow:main_sphinxdev

Conversation

@lucyleeow
Copy link
Copy Markdown
Contributor

@lucyleeow lucyleeow commented Jul 31, 2023 •
edited
Loading

CI is failing with sphinx dev with error:
(see: https://dev.azure.com/sphinx-gallery/sphinx-gallery/_build/results?buildId=1368&view=logs&j=18f51a03-7fab-57d7-f724-30d2d83bc222&t=7c53da0f-8616-5987-83c8-9378d265c3e8&l=1124)

ERROR at setup of test_timings ________________________
/usr/share/miniconda/lib/python3.10/site-packages/sphinx/events.py:96: in emit
    results.append(listener.handler(self.app, *args))
sphinx_gallery/docs_resolv.py:471: in embed_code_links
    _embed_code_links(app, gallery_conf, gallery_dir)
sphinx_gallery/docs_resolv.py:324: in _embed_code_links
    doc_resolvers[this_module] = SphinxDocLinkResolver(
sphinx_gallery/docs_resolv.py:146: in __init__
    if doc_url.startswith(('http://',/ 'https://'))/:
E   AttributeError: 'PosixPath' object has no attribute 'startswith'

The above exception was the direct cause of the following exception:
sphinx_gallery/tests/test_full.py:51: in sphinx_app
    return _sphinx_app(tmpdir_factory, 'html')
sphinx_gallery/tests/test_full.py:83: in _sphinx_app
    app.build(False, [])
/usr/share/miniconda/lib/python3.10/site-packages/sphinx/application.py:355: in build
    self.events.emit('build-finished', None)
/usr/share/miniconda/lib/python3.10/site-packages/sphinx/events.py:107: in emit
    raise ExtensionError(__("Handler %r for event %r threw an exception") %
E   sphinx.errors.ExtensionError: Handler <function embed_code_links at 0x7f163d981990> for event 'build-finished' threw an exception (exception: 'PosixPath' object has no attribute 'startswith')

I think sphinx started using pathlib in: sphinx-doc/sphinx#11526

Potentially in future we may want to check if outdir is a Path object (instead of checking if str starts with http..) but have just used str for now.

@larsoner
Copy link
Copy Markdown
Contributor

larsoner commented Jul 31, 2023

Failure looks real and related

https://dev.azure.com/sphinx-gallery/sphinx-gallery/_build/results?buildId=1370&view=logs&j=18f51a03-7fab-57d7-f724-30d2d83bc222&t=7c53da0f-8616-5987-83c8-9378d265c3e8&l=262

I think str(outdir) is a fine fix! Alternatively we could do Path(outdir) and use paths where possible. Whichever is easier I'd go with...

@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Aug 1, 2023 •
edited
Loading

Have just gone with str here. What do you think about moving from os to pathlib in another PR? (bigger job than initially thought, 380 results...)

@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Aug 1, 2023

moving from os to pathlib

It would be a really big job, but I wonder if it would fix things like: #440

@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Aug 1, 2023

merging as CI is green, will consider migrating to pathlib if you support it @larsoner

@lucyleeow lucyleeow merged commit ae89d2d into sphinx-gallery:master Aug 1, 2023
@lucyleeow lucyleeow deleted the main_sphinxdev branch August 1, 2023 09:25
@larsoner
Copy link
Copy Markdown
Contributor

larsoner commented Aug 1, 2023

Migrating to pathlib could be helpful. I'm not sure if it's more likely to resolve/fix bugs or create new ones. But could ultimately be worthwhile!

@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Aug 2, 2023

create new ones.

The more I think about it, the more I think this is true. I think it is okay to have only some parts of the code use pathlib, I will amend the app.builder.outdir code to use Path as it is cleaner and we probably want to move this way overall.

@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Aug 2, 2023

(In any case should be done when one has the time to fix the resulting bugs...!)

@lucyleeow lucyleeow mentioned this pull request Aug 2, 2023
Merged
clrpackages referenced this pull request in clearlinux-pkgs/pypi-sphinx_gallery Aug 22, 2023
@fenrus75
pypi-sphinx_gallery: Autospec creation for update from version 0.13.0…
4b75a9d 
… to version 0.14.0

v0.14.0
-------

**Implemented enhancements:**

-  MAINT Update backreferences docs and add tests `#1154 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1154>`__ (`lucyleeow <https://github.com/lucyleeow>`__)
-  Remove extra spaces in reported running time `#1147 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1147>`__ (`stefanv <https://github.com/stefanv>`__)

**Fixed bugs:**

-  MAINT: Fix for Sphinx 7.2 `#1176 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1176>`__ (`larsoner <https://github.com/larsoner>`__)
-  updated mpl gui warning catcher to new error message `#1160 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1160>`__ (`story645 <https://github.com/story645>`__)
-  Ensure consistent encoding for md5sum generation `#1159 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1159>`__ (`sdhiscocks <https://github.com/sdhiscocks>`__)
-  Maint: Fix ``app.builder.outdir`` as Sphinx now using pathlib `#1155 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1155>`__ (`lucyleeow <https://github.com/lucyleeow>`__)
-  Make \_LoggingTee compatible with TextIO `#1151 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1151>`__ (`o-laurent <https://github.com/o-laurent>`__)

(NEWS truncated at 15 lines)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug maintenance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lucyleeow @larsoner