Add a minimal pyproject.toml #114
Conversation
|
Windows, Python 3.9 failed during Cython installation. Would pinning the version fix this? |
Possibly - I think there is a more fundamental issue that the command-line args to setup.py may not be passed through to the isolated build. |
|
Assuming that everything passes, this is probably ready for review. |
.github/workflows/test-pr.yml
Outdated
| - name: Install library and dependencies | ||
| run: python -m pip install -e . | ||
| if: matrix.os == 'ubuntu-latest' | ||
| - name: Install library (w/o text rendering support) | ||
| run: python -m pip install -e . --install-option --no-text-rendering | ||
| if: matrix.os != 'ubuntu-latest' |
There was a problem hiding this comment.
Can we keep the explicit no-text-rendering test? Using the CELIAGG_NO_TEXT_RENDERING flag in the environment?
There was a problem hiding this comment.
Yes, but what's the goal? To test the builds with no text rendering or because there is an issue on Windows?
The tests pass on Windows with text rendering (even though there is no Harfbuzz)
There was a problem hiding this comment.
The goal would be to explicitly exercise the logic in setup.py (and also the parts of the library looking out for the absence of text rendering)
There was a problem hiding this comment.
To be more clear: It was decided in the early days of this project that one should be able to build the library with only numpy if they don't care about missing text rendering. So as long as that functionality is in place, we should endeavor to test it.
This decision can of course be revisited, but that's not the intent of this PR.
There was a problem hiding this comment.
My concern was that we had a branch of the text rendering code (the windows non-freetype rendering) that is being missed from tests - ie. if something breaks text rendering on windows, we wouldn't know.
It seems like a richer matrix might make sense, perhaps at the expense of testing less Python versions on Windows/Mac.
What I might do is revert this to testing exactly the same set of cases as the original, and there can be another PR around what cases we actually want to test.
There was a problem hiding this comment.
Sounds good. That's a very good point about explicitly testing Windows text rendering.
.github/workflows/test-pr.yml
Outdated
| - name: Install dependencies for MacOS | ||
| run: | | ||
| brew install pkg-config freetype harfbuzz | ||
| if: matrix.os == 'macos-latest' |
There was a problem hiding this comment.
Can we leave this out? macOS runners are generally pretty oversubscribed, so it would be nice to keep the build fast there.
There was a problem hiding this comment.
It only adds 11 seconds, but it looks like they may already be installed:
Warning: pkg-config 0.29.2_3 is already installed and up-to-date.
To reinstall 0.29.2_3, run:
brew reinstall pkg-config
Warning: freetype 2.13.0_1 is already installed and up-to-date.
To reinstall 2.13.0_1, run:
brew reinstall freetype
Warning: harfbuzz [7](https://github.com/celiagg/celiagg/actions/runs/4691785631/jobs/8316700693?pr=114#step:4:8).1.0 is already installed and up-to-date.
To reinstall 7.1.0, run:
brew reinstall harfbuzz
If we are concerned about minimizing MacOS runner time, do we want to test only on a subset of Python versions (eg. 3.8 and 3.11)?
There was a problem hiding this comment.
Fair enough. Another PR can improve the build matrix.
I fear that this won't work correctly: environment variables may not be passed in.
|
Nope - just too many negations. |
|
Thanks for grinding through this! |
This is enough to permit installation without needing numpy and cython in the environment via PEP-517 style isolated builds. Text rendering in the builds is now controlled by an environment variable. Fixes #105
This is enough to permit installation without needing numpy and cython in the environment via PEP-517 style isolated builds. Text rendering in the builds is now controlled by an environment variable. Fixes #105
This is enough to permit installation without needing
numpyandcythonin the environment via PEP-517 style isolated builds.Fixes #105