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

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

Merged
merged 3 commits into from Aug 1, 2023

Conversation

lucyleeow
Copy link
Contributor

@lucyleeow lucyleeow commented Jul 31, 2023

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
Contributor

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
Contributor Author

lucyleeow commented Aug 1, 2023

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
Contributor Author

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
Contributor Author

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
15 checks passed
@lucyleeow lucyleeow deleted the main_sphinxdev branch August 1, 2023 09:25
@larsoner
Copy link
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
Contributor Author

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
Contributor Author

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

clrpackages pushed a commit to clearlinux-pkgs/pypi-sphinx_gallery that referenced this pull request Aug 22, 2023
… 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants