py-breathe: fix version constraints to avoid concretizing old breathe#31828
py-breathe: fix version constraints to avoid concretizing old breathe#31828sethrj merged 1 commit intospack:developfrom
Conversation
sethrj
force-pushed
the
py-breathe-version-fix
branch
from
a344804 to
2ff5b4f
Compare
| depends_on("py-sphinx@:3", type=("build", "run"), when="@:4.20") | ||
| depends_on("py-sphinx@3:3.2", type=("build", "run"), when="@4.21:4.32") | ||
| depends_on("py-sphinx@3:4", type=("build", "run"), when="@4.33:") | ||
| depends_on("py-sphinx@3:", type=("build", "run"), when="@4.33:") |
There was a problem hiding this comment.
This is incorrect, the setup.py for breathe 4.33.1 does not support sphinx 5:
requires = ["Sphinx>=3.0,<5", "docutils>=0.12"] @alalazo why was this PR merged without maintainer approval?
There was a problem hiding this comment.
@adamjstewart It builds and works with sphinx 5 for me, and didn't work with the previous concretization. Not sure what's up with the embedded version file.
There was a problem hiding this comment.
why was this PR merged without maintainer approval?
I guess out of a misunderstanding? We all have merge power here, and I only approved the PR after just checking the syntax. @sethrj merged, possibly thinking that I might have forgot to hit merge. If there's anything to fix we can do it in a follow up PR.
There was a problem hiding this comment.
In terms of merging: I usually wait up to a week for official maintainers to review a PR before merging. In the case of Python, that means me. Idk if we have any official policy on this, but might be worth implementing.
In terms of breathe/sphinx: more broadly, this is a question of do we trust documented dependency versions or tested/known-working versions. I've mostly been following the former since it's impossible to actually test all versions. Currently, we're using pip to install packages, and purposefully using a flag to disable checking of dependency versions. If we remove that flag, or switch to build/installer, these unsupported versions will break.
What was the concretization issue? It might be better to cap old versions at :5 since even newer versions don't claim to support it yet. That will ensure that pip/build are happy and still use the latest version of breathe by default. If you need to use breathe and Sphinx 5, then we'll need to add a newer version of breathe to the package.
There was a problem hiding this comment.
Sorry @adamjstewart , I saw the reviewer "approval" and thought everything was kosher for merging. This one's on me not @alalazo . I'll wait for you for further python merges.
The last time I updated py-breathe I did so based on the setup.py dependency. It looks like without this update, I get [email protected] ^[email protected] which definitely fails.
There was a problem hiding this comment.
Your first line should fix that (assuming that you found that it works with 3.5.4 but not 4.0.0.
Your second line should probably be reverted.
3 participants
No description provided.