Skip to content

Conversation

delfanbaum
Copy link
Contributor

@delfanbaum delfanbaum commented Aug 19, 2025

This PR aims to resolve #404 and #378.

In short:

  • Initialize uv as a package (and library), while also adding [project.scripts] to pyproject.toml such that we no longer need the scripts/ directory; the svg2pdf executable will be installed upon installation of the wheel
  • Add a __init__.py file to the tests/ directory so on uv run pytest it finds it by default
  • Updated the ci.yaml workflow to use uv and the existing matrix; IMO this deprecates any need for tox although I've kept that around in case folks like to use that to test locally; note that I did not update any documentation around that file, as I'm not a tox user myself.
  • I did my best to update the documentation to match what I believe are "best practices" for installation, development, testing, etc.

Any feedback is, of course, welcome.

In particular, I was not hoping to work on the code much, only the organization, but it appears that the DeepSource CI is failing due to pre-existing issues. If that's a requirement for this change I'm happy to take a swing at resolving them.

@delfanbaum delfanbaum marked this pull request as ready for review August 19, 2025 17:04
@deeplook
Copy link
Owner

deeplook commented Aug 20, 2025

@delfanbaum Cool, thanks! This looks much like what I had in mind, but never finished. I see that the CI fails with timeouts on loading the uv-related action. I don't know if this is only a hiccup, but I've seen previous reports about version 5, too. Apparently you have the same errors in your fork:

Error: Unable to resolve action astral-sh/setup-uv@6, unable to find version 6

I agree with all your points above, but want to test it locally myself. The user interface needs to remain unchanged, but I think this is the case.

Oh, and thanks for the py.typed file, which I wasn't aware of, yet!

@delfanbaum
Copy link
Contributor Author

Ah, good shout! I'd missed the setup failure; I was missing a v. And my pleasure! I was also unaware of the py.typed file, which is pretty neat.

@deeplook
Copy link
Owner

Progress, but now we run into an old issue with Cairo that we have ignored for too long:

FAILED tests/test_samples.py::TestW3CSVG::test_convert_pdf_png - reportlab.graphics.utils.RenderPMError: cannot import desired renderPM backend rlPyCairo

@replabrobin, can we finally nail this, and how?

@deeplook
Copy link
Owner

Progress, but now we run into an old issue with Cairo that we have ignored for too long:

FAILED tests/test_samples.py::TestW3CSVG::test_convert_pdf_png - reportlab.graphics.utils.RenderPMError: cannot import desired renderPM backend rlPyCairo

@replabrobin, can we finally nail this, and how?

@delfanbaum I wonder if you also see these errors locally? You can run uv run pytest tests/test_samples.py.

@delfanbaum
Copy link
Contributor Author

delfanbaum commented Aug 21, 2025

I don't remember getting that error on a machine that I'd installed cairo on separately (I'll double check when I get to work); but for that test at home I instead fail at renderPDF.drawToFile(drawing, base, showBoundary=0):

>       sig = self.signature = md5(usedforsecurity=False)
E       TypeError: 'usedforsecurity' is an invalid keyword argument for openssl_md5()

Might be a red herring though, since this appears to be a reportlab thing; I'll try to find some time to do some investigating later on.

Additional context:

uv run python
Python 3.8.20 (default, Oct  2 2024, 16:12:59)
[Clang 18.1.8 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import reportlab
>>> reportlab.__version__
'4.4.3'

@delfanbaum
Copy link
Contributor Author

delfanbaum commented Aug 21, 2025

Re TypeError: 'usedforsecurity' is an invalid keyword argument for openssl_md5() above; it appears to resolve itself if you're using Python 3.9... this implies that either reportlab needs to be pinned to an earlier version or support for 3.8 should be dropped (and, frankly, if that's the choice, 3.9 reaches EOL this October, so).

Once I switch over to 3.9, I do get the

E                   reportlab.graphics.utils.RenderPMError: cannot import desired renderPM backend rlPyCairo
E                   Seek advice at the users list see
E                   https://pairlist2.pair.net/mailman/listinfo/reportlab-users

.venv/lib/python3.9/site-packages/reportlab/graphics/renderPM.py:52: RenderPMError`

error.

@delfanbaum
Copy link
Contributor Author

Adding rlPyCairo resolved the tests locally on 3.9; pushed up the commit to see if that also resolves the errors on CI...

@deeplook
Copy link
Owner

If you remove 3.8 from the github CI, it might pass.

@deeplook
Copy link
Owner

And let's bump to 1.6.0, and add yourself to the Contributors.rst file, please!

@delfanbaum
Copy link
Contributor Author

On the CI on my fork of it, it's still failing 3.9 (See here) due to not being able to find/install cairo during the installation phase:

Run-time dependency cairo found: NO (tried pkgconfig and cmake)

      ../cairo/meson.build:31:12: ERROR: Dependency "cairo" not found, tried
      pkgconfig and cmake

I had gotten it to pass on a machine that already had cairo installed; my guess is that something that used to install cairo now no longer installs it. So that could be an external dependency, or we somehow figure out how to install it via this package.

@delfanbaum
Copy link
Contributor Author

So on my fork, I've got ubuntu and macos working, Windows TBD (I just kicked it off). Moral of the story: cairo is a requisite external dependency now. I'm not sure if this is a reportlab thing, where they used to include an install step but no longer, or what (it appears to be closed source?), or if the pycairo wheels stopped including it (I think this might actually be the case), but regardless: I, at least, don't see a way around this non-Python dependency.

@deeplook
Copy link
Owner

Amazing! I've had some issues myself on macOS playing with cairo, cmake, pkg-config. Will look at your changes tomorrow. Have run CI tests, and repeated on Windows after what seemed like a timeout. It worked for 3.9, but failed for PyPy which amazingly worked on macOS and Linux, but this shouldn't be a blocker. In any case great progress! But a pity that installation is getting so hard.

pyproject.toml Outdated
"Operating System :: POSIX",
"Natural Language :: English",
"Programming Language :: Python",
"Programming Language :: Python :: 3.8",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can drop 3.8 here, too.

@deeplook
Copy link
Owner

I've added a tiny change, unrelated to cairo. We can build now, but one build on Windows takes about 15-17 minutes, and the PyPy one is failing, which I can accept and remove from the CI workflow. But I wonder if there is any faster way to install GTK3 on Windows? Else I'd opt to run only one build on Windows with Python 3.11 or 3.12 or so.

We might need a little more info about installation, but we've made great progress!

@delfanbaum
Copy link
Contributor Author

But I wonder if there is any faster way to install GTK3 on Windows?

I considered just downloading and unzipping the executable from their GitHub release, although that felt brittle; would definitely be faster though!

Else I'd opt to run only one build on Windows with Python 3.11 or 3.12 or so.

Yes; I suppose if we're changing the builds it would make sense to add some newer Pythons, maybe just for ubuntu or some such to keep it quick? I'm not sure what your coverage requirements are/were.

One other option -- and this may actually be a good one -- is that, since we're having uv manage the python installation anyway, we can have uv run the tests for multiple python versions on a single windows image. (which actually... we may always be running 3.9 regardless, now that I think of it, if it's respecting the .python-version; I'll update the tests to ensure this).

@deeplook
Copy link
Owner

I've tried to commit some minor CI changes, but strangely this second commit to your branch was not accepted, unlike the first, being not from a verified, anonymous owner. But that was also the case for the forst one, weird. I'll have to look into this.

But we could just take 3.9 and 3.13 on macOS and Ubuntu, and 3.13 only on Windows with the current setup, and try to accelerate a Windows build later, if you're interested as I'm not of much help there. There is no hard requirements, but it might be nice to take the oldest supported version and the newest, and assume those in between are likely to work, too.

Delegating more of the version switching to uv might get a little efficiency, but would also reduce some of the organizational comfort re monitoring, rerunning etc. So while it might be nice to see it work, I'm not sure the benefit is big enough.

I think it would be nice to clean up this repackaging PR along these lines (toching a bit more on installation complexity), and then there can be a few more polishing touches before making a proper 1.6.0 release within a week or so. Would that work for you?

In any case I'm really happy to finaly see proper grading in many of the tests! 😃

@delfanbaum
Copy link
Contributor Author

Yes, that all sounds good -- I can try to swing at cleaning all this up tomorrow or Friday! Thanks for your patience with all of this.

Ohm, and re: unverified commits, that's an artifact of my working on a personal vs work machine; probably the kick in the pants I need to get the personal machine set up for verified commits...

@@ -0,0 +1,55 @@
name: Run Matrix Tests on Windows
on: [push, pull_request, workflow_dispatch]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just comment this line out, so we can merge this PR and handle the Windows CI acceleration separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, yes -- I'll clean this up today.

This aims to resolve deeplook#404, as
well as deeplook#406, by updating the
project structure and build process leveraging the `uv` tool
(https://docs.astral.sh/uv/).

It also drops support for Python 3.8 for a variety of reasons, including
making the tests pass.

Follow-up commits will fix the CI, improve documentation, etc.

Note that per discussion with deeplook, this includes a "minor" version
bump to 1.6.0.
README updated per new state of affairs; CONTRIBUTORS updated per my
contribution per deeplook.
This commit breaks out the CI runners into different platforms to make
the `cairo` installation simpler, temporarily turning off the Windows
runner, per discussion, until we can figure out how to install `cairo`
on that platform with anything resembling efficiency.

Also, per discussion, the CI steps have been simplified to target the
oldest/newest targeted Python versions, making the (fair) assumption
that things should work in-between.
@delfanbaum
Copy link
Contributor Author

Okay, cleaned up into fewer and nicer commits, took in the suggestions re: CI, and tried to add a little more documentation.

Copy link
Owner

@deeplook deeplook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tests are passing. Will merge tomorrow. Thanks again!

@deeplook deeplook merged commit b63cb60 into deeplook:main Sep 3, 2025
10 of 11 checks passed
@deeplook deeplook mentioned this pull request Sep 4, 2025
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.

installation DEPRECATION warning with pip 25.1.1
2 participants