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

RELEASE: v0.15.1 + BUG: Issue warning for sphinxcontrib-bibtex + docutils>=0.18,<0.20 #1965

Merged
merged 8 commits into from
Mar 14, 2023

Conversation

mmcky
Copy link
Member

@mmcky mmcky commented Mar 9, 2023

This PR includes a minor bug fix due to interactions between sphinxcontrib-bibtex and docutils>=0.18 causing malformed html pages when using sphinx-book-theme as recorded mcmtroffaes/sphinxcontrib-bibtex#322.

This minor update includes a warning message to jupyter-book users if using these versions of docutils.

This also includes updates to CHANGELOG and make a minor version increment in preparation for a new release.

  • check tests
  • review docs (make a note re: docutils)

fixes #1948

@mmcky mmcky requested a review from AakashGfude March 9, 2023 01:06
@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.06 🎉

Comparison is base (a17924d) 91.42% compared to head (e3d73c2) 91.48%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1965      +/-   ##
==========================================
+ Coverage   91.42%   91.48%   +0.06%     
==========================================
  Files           7        7              
  Lines         688      693       +5     
==========================================
+ Hits          629      634       +5     
  Misses         59       59              
Flag Coverage Δ
pytests 91.48% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
jupyter_book/__init__.py 100.00% <100.00%> (ø)
jupyter_book/config.py 94.47% <100.00%> (+0.14%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mmcky
Copy link
Member Author

mmcky commented Mar 9, 2023

@AakashGfude @agoose77 @choldgraf if you had time to take a look at this that would be helpful. I will leave this open tonight and plan to do some final testing tomorrow.

@mmcky
Copy link
Member Author

mmcky commented Mar 9, 2023

The pin fixes the single page on our docs that is broken

https://jupyter-book--1965.org.readthedocs.build/en/1965/content/citations.html

I am just wondering if pinning docutils is going to cause issues elsewhere when resolving environments. I haven't come across any issues when building environments locally today.

@agoose77
Copy link
Collaborator

agoose77 commented Mar 9, 2023

Hi @mmcky! I think it's great that you're thinking about this, and appropriate that you're concerned about the downstream problems. When I read the issue, I also had a hypothesis that this would not work as intended, but I wanted to validate that with a local devpi instance to be sure.

If we pin docutils<X in a newer release of Jupyter Book, then there are two possible outcomes:

  • User has pinned docutils>=X, so they get an old version of jupyter-book that doesn't have this stricter upper bound on docutils
  • User has not pinned docutils>=X, so they get the new version of jupyter-book, and docutils<X

I created a local package repo, and performed a locking solve for the following with pdm:

[tool.pdm]

[project]
name = ""
version = ""
description = ""
authors = [
    {name = "Angus Hollands", email = "goosey15@gmail.com"},
]
dependencies = ["jupyter-book", "docutils"]
requires-python = ">=3.10"
license = {text = "MIT"}

[build-system]
requires = ["pdm-pep517>=1.0.0"]
build-backend = "pdm.pep517.api"

[[tool.pdm.source]]
url = "http://localhost:3141/testuser/dev/+simple"
verify_ssl = false
name = "dev"

This gave me docutils<18.0 and jupyter-book==0.15.1.

This makes sense, because locking solvers usually build a topology of dependencies, and start at the top-level "root" nodes. However, this approach may introduce problems for users who are themselves pinning docutils with an upper cap. If someone later pins docutils>=0.18.0, Poetry and friends will backsolve to an older version of jupyter-book, which will still fail with docutils. e.g. the lockfile for this:

[tool.pdm]

[project]
name = ""
version = ""
description = ""
authors = [
    {name = "Angus Hollands", email = "goosey15@gmail.com"},
]
dependencies = ["jupyter-book", "docutils>=0.18.0"]
requires-python = ">=3.10"
license = {text = "MIT"}

[build-system]
requires = ["pdm-pep517>=1.0.0"]
build-backend = "pdm.pep517.api"

[[tool.pdm.source]]
url = "http://localhost:3141/testuser/dev/+simple"
verify_ssl = false
name = "pypi"

produces jupyter-book==0.15.0, docutils==0.18.1, which is also broken.

So, if we were distributing jupyter-book, and end-users weren't able to install e.g. sphinx extensions, then this would be fine. But, we are part of an ecosystem, and users may want to have jupyter-book alongside other libraries that may ultimately do this. In both cases, users will run into a bug, it just happens at different times (solve time vs usage time). The difference with doing it at runtime is that we can set a nice message that directly tells the user what is wrong, rather than a cryptic solve error / working out why jupyter-book is behaving badly.

I think we really need to do this at runtime, so that the end user can set the appropriate pins (docutils<0.18.0). If we try to do this with immutable package metadata, we run into the problems outlined here. I've re-pinged the sphinxcontrib-bibtex maintainer to review the PR that you referenced, which hopefully will mean that new installations of jupyter-book will get this latest version of sphinxcontrib-bibtex that warns for docutils.

@mmcky
Copy link
Member Author

mmcky commented Mar 9, 2023

thanks for your detailed response and thoughts @agoose77 -- that makes good sense.

I'll rework this to provide some runtime feedback with a message on how to fix this should a user want to use sphinxcontrib-bibtex or bibliographies. (or maybe add a message if a bibliography directive/node is parsed in the tree).

I will wait to see if that upstream PR is merged first.

That is a good recommendation.

@mmcky
Copy link
Member Author

mmcky commented Mar 13, 2023

@agoose77 this has been reworked to issue a warning when using sphinxcontrib-bibtex and docutils>=0.18,<0.20.

I don't see much harm in having this here while we wait for upstream changes etc.

@mmcky mmcky changed the title RELEASE: v0.15.1 RELEASE: v0.15.1 + BUG: Issue warning for sphinxcontrib-bibtex + docutils>=0.18,<0.20 Mar 13, 2023
@agoose77
Copy link
Collaborator

I think this is a good solution; we're already seeing user run into this bug, so it would be good to try and get ahead of it. I really wish we had more oversight into what the docutils release process. It looks like we could be waiting for some time, which is really not ideal.

@mmcky
Copy link
Member Author

mmcky commented Mar 14, 2023

thanks @agoose77 for your thoughts and comments. Greatly appreciated.

I will merge this once the tests pass and I will organise a new release. Given this is a minor change I will go ahead and merge.

@mmcky mmcky merged commit 04b7b65 into master Mar 14, 2023
@mmcky mmcky deleted the prep-0.15.1 branch March 14, 2023 01:16
casperdcl added a commit to casperdcl/jupyter-book that referenced this pull request Aug 21, 2023
@casperdcl casperdcl mentioned this pull request Aug 21, 2023
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.

unstructured html outputs v0.15.1
2 participants