update packaging and testing and drop EoL Pythons #41
Conversation
terencehonles
force-pushed
the
update-packaging-and-testing-and-drop-eol-pythons
branch
2 times, most recently
from
fb7efd5 to
60a396b
Compare
There was a problem hiding this comment.
I left this as its own file, but it can be moved to pyproject.toml too, but we'd need to ensure tox v4 I believe and probably will need to use a different Github Actions runner
| wheel | ||
| # last release for old versions | ||
| sphinx747: sphinx==7.4.7 | ||
| sphinx621: sphinx==6.2.1 |
There was a problem hiding this comment.
I dropped 5.x because it looks like it has issues with Python 3.13 since it imports the removed imghdr module differently than more recent versions, and I don't know if we need to go that far back. Otherwise we can just change the envlist to not include 5x for Python 3.13, but two older releases seem sufficient.
| [testenv:wheel] | ||
| minversion = 3.15.0 | ||
| commands = | ||
| python -m build --wheel . |
There was a problem hiding this comment.
Since building the wheel doesn't need to be tested in all the Python versions I split this out, and only test that it builds with the latest Python (at least on the CI), if you run locally I believe this will use the base Python.
| commands = | ||
| ./setup.py bdist_wheel | ||
| ./setup.py install |
There was a problem hiding this comment.
tox always installs the package (unless you say otherwise), so this was redundant
sphinxcontrib/__init__.py
Outdated
There was a problem hiding this comment.
pyproject.toml handles namespace packages automatically
setup.cfg
Outdated
There was a problem hiding this comment.
Universal wheels are deprecated and are only needed for Python 2, so since we're dropping Python 2 this isn't needed.
| author_email='tomasz.czyz@gmail.com', | ||
| description='Sphinx extension for thumbnails', | ||
| long_description=codecs.open('README.rst', encoding="utf8").read(), | ||
| zip_safe=False, |
There was a problem hiding this comment.
This looks like it's deprecated and doesn't seem to apply to Python 3 (or it can still break since you can install zip packages, but that's not standard)
pyproject.toml
Outdated
| ] | ||
| dependencies = [ | ||
| "requests>2.2,<3", | ||
| "setuptools", # TODO: migrate off of pkg_resources |
There was a problem hiding this comment.
This is now properly declared as a dependency until pkg_resources is not used here:
images/sphinxcontrib/images.py
Lines 285 to 288 in 30b50b8
It's no longer needed for namespace packages
terencehonles
force-pushed
the
update-packaging-and-testing-and-drop-eol-pythons
branch
3 times, most recently
from
8d4045c to
83283c7
Compare
|
I've verified that the packaging creates the same assets (aside from the previously missing
|
|
Thank you @terencehonles This appears to be what we need and your knowledge of actions/workflow/tox/packaging is more up-to-date than mine. Do you think we should just merge this here in March/April 2025 and then merge #39 after? |
|
That seems reasonable to me. I'm out of office right now and may not have any time to update anything until April 9th or 14th, but I can look to see if anything here needs an update if we're waiting until that point anyways. Otherwise, I assume everything here was clear to merge and can be updated later if there's any more recent follow-ups that make sense since I proposed this pr. |
|
I had a chance to review the PR and no new Python versions hit EoL, so this should be good to merge. I had tested this with #39 IIRC, so they could be merged independently in series or can be merged into a temporary branch and have that branch run the CI checks to confirm everything goes green. |
|
@terencehonles I've tested merging #41 followed by merging #39 in this branch https://github.com/sphinx-contrib/images/tree/pr3941 and everything looks good (https://github.com/sphinx-contrib/images/actions/runs/14230225566). Would you test https://test.pypi.org/project/sphinxcontrib-images/0.9.5rc1/ to see if everything is working as expected? For me the package is installable and it can build the documentation in I haven't really considered the version requirements you suggested in details, we should consider those a second time. Dropping support for Python<3.9 seems reasonable, but does that necessitate dropping support for sphinx 4? And are the requirements on Not that it matters much, but should it be version 0.9.5 or something else, like 1.0.0? I suppose the extension does have a public API which calls for 1.y.z (same applied in 2021 though), but bumbing the major number is normally reserved for breaking backward compability. |
I'm of the opinion the commit from #39 should be cherry-picked into this branch/PR. Hard to update packaging/testing when the tests fail due to an import error from the very get-go. |
pyright seems to be giving a hint to the issue (I'll keep looking) EDIT: fix here I believe is to remove the |
|
Converting EDIT: this was my own fault, I cloned the repo but didn't initialize the submodules |
pyproject.toml
Outdated
| "Environment :: Console", | ||
| "Environment :: Web Environment", | ||
| "Intended Audience :: Developers", | ||
| "License :: OSI Approved :: Apache Software License", |
There was a problem hiding this comment.
license specifications in the classifier I recently learned has been deprecated
EDIT: looks like they were deprecated long ago!! https://peps.python.org/pep-0639/
There was a problem hiding this comment.
Yes, I think we should fix those warnings as well - might as well do while we're moving from setup.py to project.toml
There was a problem hiding this comment.
pyproject.toml could be patched as follows
diff --git a/pyproject.toml b/pyproject.toml
index 4a2cbcc..a9ade4f 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -7,6 +7,8 @@ name = "sphinxcontrib-images"
version = "0.9.4"
requires-python = ">= 3.9"
+license = "Apache-2.0"
+license-files = ["LICENSE.txt"]
authors = [
{name = "Tomasz Czyż", email = "tomasz.czyz@gmail.com"},
]
@@ -15,7 +17,6 @@ classifiers = [
"Environment :: Console",
"Environment :: Web Environment",
"Intended Audience :: Developers",
- "License :: OSI Approved :: Apache Software License",
"Operating System :: OS Independent",
"Programming Language :: Python",
"Programming Language :: Python :: 3 :: Only",
@@ -31,7 +32,6 @@ dependencies = [
"sphinx>=5.0",
]
description = "Sphinx extension for thumbnails"
-license = {text = "Apache 2"}
readme = "README.rst"
There was a problem hiding this comment.
While PEP-639 declared the new format quite a while ago it took awhile to be implemented by the backends (see table here), and setuptools only got support this March.
Since we do control the build environment I bumped this to require setuptools >= 77.0.3, and defined the license information using the new syntax. I believe the "Apache 2" specifier I used previously probably should have been "Apache-2.0", so that's fixed regardless
|
Once I realized there was also a submodule; everything ran successfully when building the pyqtgraph docs. Only outstanding issue is the |
I agree - that would be pretty / semantic way, ensuring credit where credit is due. The quick way around it would be the ugly double merging or manually reimplementing (typing /copying) the two PRs. |
|
@terencehonles Can you cherry-pick commit fc15936 from #39 into your fork/PR? Maybe a few other changes to pyproject.toml could be made as well (it causes a few deprecation warnings at the moment). |
Can confirm that this is the correct fix. I found it myself while debugging some odd logging errors I was getting while using 0.9.5rc1, and converting Any chance a new build could be put together with this fix? |
|
@coredumperror I don't think I see any errors related to this when testing the pr3941 branch (from git or from test.pypi.org). Would you mind posting your logs? Imports like |
|
Are you using Sphinx 8.x? This doesn't seem to happen with Sphinx 7.x. Here's what I get in my output when I build the HTML version of my project's docs with 0.9.5rc1 and Sphinx 8.2.3: This same traceback gets printed a dozen times, as it seems to be part of the progress bar, and crashes every time the builder tries to print an updated progress percentage. The build itself actually does finish, tho, despite these tracebacks spamming the log, which may be why you're not seeing an error. Here's a summarized version of the entire output, to show what I mean: The |
|
@coredumperror Thank you for the further details - I'll have a look at your sphinx8-api-change hypothesis. |
|
I can reproduce this now, it happens with sphinx 8.2.3 but not sphinx 8.1.3. So something must have changed between the two versions, or something changed between python 3.10 (on which I tested sphinx 8.1.3) and python 3.12 (on which I tested sphinx 8.2.3). According to the Sphinx's list of deprecated APIs nothing was removed in 8.2: https://www.sphinx-doc.org/en/master/extdev/deprecated.html (changelog https://www.sphinx-doc.org/en/master/changes/8.2.html). |
This change I don't think was part of Sphinx's public API? Since you narrowed the two versions you could git bisect. Alternatively see if the string form of the color name works with the oldest version of sphinx you plan to support. If so you could just make the change. |
|
@j9ac9k Is it then a bug in sphinx / failure to document a change / something which worked by accident in the past? Anyway, good idea with bisection, I'll give it a try. Let me know if you find it first. |
|
I’m away for vacation right now so it will be a week or so before I can hop
on a computer to help diagnose.
I haven’t done much development of sphinx so I’m not sure where they draw
the boundary between their public and private APIs, so those color imports,
I’m not sure how they’d categorize it. That said in recent years Sphinx
has picked up the development cycle and a consequence of that is more bugs
get introduced.
Judging by the error, it really does seem that method wants a string, not a
callable, so even if no error was raised in sphinx 8.1 I would verify you
were getting the correct output. I do think the more time saving option is
see if you get the correct output using a string on the oldest version of
sphinx you want to support and if that works, just make the change and move
on.
…On Fri, Apr 11, 2025 at 22:44 Jonas Camillus Jeppesen < ***@***.***> wrote:
@j9ac9k <https://github.com/j9ac9k> Is it then a bug in sphinx / failure
to document a change / something which worked by accident in the past?
Anyway, good idea with bisection, I'll give it a try. Let me know if you
find it first.
—
Reply to this email directly, view it on GitHub
<#41 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE5Z7QX4PZB5XKVYBP7BDL2ZASUVAVCNFSM6AAAAABRAXAJL2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJXHE3TQNRZGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
*jonascj* left a comment (sphinx-contrib/images#41)
<#41 (comment)>
@j9ac9k <https://github.com/j9ac9k> Is it then a bug in sphinx / failure
to document a change / something which worked by accident in the past?
Anyway, good idea with bisection, I'll give it a try. Let me know if you
find it first.
—
Reply to this email directly, view it on GitHub
<#41 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE5Z7QX4PZB5XKVYBP7BDL2ZASUVAVCNFSM6AAAAABRAXAJL2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJXHE3TQNRZGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Let us continue |
|
@coredumperror I've made a test build with the string fix: https://test.pypi.org/project/sphinxcontrib-images/0.9.5rc2/ |
|
Fabulous! With Sphinx 8.2.3 and shinxcontrib-images 0.9.5rc2 installed, I get exactly the output I expect: |
Done, I'll need a little time to follow up on the thread, but I just got back from an extended vacation and more or less caught up on my work responsibilities. |
|
@terencehonles Great! I think you're up to date, nothing relevant to this PR have been posted since I asked you to cherry-pick. This thread has, however, been used to discuss an unreleated issue related to colored log output which causes errors with sphinx>8.2. That discussion is moved to #45. I will try to prepare a release based on your updated PR. A few further changes to the code base (fix to #45 and fixing of pyproject.toml deprecation warnings) should happen outside of this PR. |
sphinx.util.status_iterator alias has been deprecated since sphinx 6.1 and will be removed in sphinx 8.0 See https://www.sphinx-doc.org/en/master/extdev/deprecated.html The preferred access for status_iterator is sphinx.util.display.status_iterator. This commit adopts the new sphinx API and then falls back on the older API access since this plugin supports older versions of sphinx.
terencehonles
force-pushed
the
update-packaging-and-testing-and-drop-eol-pythons
branch
from
f859520 to
8c72db8
Compare
|
I updated the license specifiers as mentioned in the thread #41 (comment) and updated the release instructions/script https://github.com/sphinx-contrib/images/pull/41/files#diff-8896efb82f66e1b2ad453d05d5577b82d8a370f239b79d9468bfe0e164bbb56f |
|
Friendly ping @jonascj
Should we just merge this now then? The release can be managed independently and if there are other docs or scripts that need to be updated they can be done by you outside this PR. |
|
Yes, I've tested it sufficiently in local branches, some pushed here for CI, to just merge it directly into master and close this. Thank you for your persistance @terencehonles in getting this updated and merged, make the package usable again! I'll strive to release a new version this weekend.
|
terencehonles
deleted the
update-packaging-and-testing-and-drop-eol-pythons
branch
This tackles a few points on #40, but will have a failing test without #39
fixes: #36 , fixes: #37