-
Notifications
You must be signed in to change notification settings - Fork 268
Support GraalPy #1538
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
Support GraalPy #1538
Conversation
ad26df1
to
ab43cbb
Compare
Would it make sense to include it?
Maybe it's time to think about adding this. It comes up occasionally. I'd like to also remove pypy37 and pypy38 from the default, as they are no longer supported by upstream. |
It would certainly be neater if it was included. Also, I've never heard of GraalPy, so I can't assess wether it's inclusion and maintenance here would be worth it for users. If Manylinux were to adopt it, that would make the decision much easier!
Hm. Yes, agreed. It would also allow us to soft-drop EoL CPythons, while keeping the functionality. The simplest way to do this would be to set a default for CIBW_BUILD. E.g. the default could be |
I was under the assumption that manylinux (like e.g. conda) wants to build everything into their images from source, but now I see that they also just download PyPy binaries and put them in there, so I can open a PR there to do the same for GraalPy. This will simplify this PR.
That's what I also thought. I did this, which is just skipping any Python specifier starting with |
Converting to draft until manylinux support lands. |
manylinux support has landed. It will be available in cibuildwheel once the update workflow runs & gets merge. |
I've updated the PR to use the manylinux support, but in the meantime |
uv now supports GraalPy on |
5dfb310
to
69760d9
Compare
60b55f6
to
c012a0f
Compare
@timfel, I don't know if you're still actively working on this, but we have a v3.0 release coming up, which would be a perfect time to land a feature like this! |
What is the timeframe for 3.0? We are still working on this, but needed a few fixes in tox, uv, and virtualenv to go in first. The virtualenv fix is still not merged because we can only pass it with the new GraalPy release that is due out in March |
Ah, I didn't realise there were blockers.
No definitive timeline, (when it's ready ;) ) but probably some time over the next few weeks. |
March is probably too far out for us to hold the 3.0 release for specifically. Not a problem, it can arrive in a point update, I just thought it would be neat :) |
Something we need to consider: how this impacts CI times. It's considerable-
Perhaps there's a way we can flag what's enabled for an integration test run - e.g. don't run the test suite on all the EnableGroups by default? The test duration is less important on |
Perhaps this is a good model to copy - pyodide-build has this code in their CI config to add flags to their tests based on PR labels pyodide/pyodide-build@794ed7a |
I've drawn up that approach in #2357 |
This looks very reasonable to me :-) Will you let me know if/when I should rebase this PR on top of that one? |
|
We don't release Windows arm binaries of GraalPy so far, so as long as it is not enabled for that platform it's fine. |
7bba507
to
7446d63
Compare
Doesn't seem to be causing a problem after rebasing, so I think it's fine. |
test/test_0_basic.py
Outdated
assert "WARNING: Running pip as the 'root' user can result" not in stream | ||
# manylinux-interpreters ensure will run pip on GraalPy as the root | ||
# user once, so we will get the warning below. We make sure it is only | ||
# because of manylinux and nothing else. We do this by checking that | ||
# the error message only occurs directly after a call to graalpy's | ||
# ensurepip in the manylinux `/opt/` directory | ||
start = 0 | ||
bad_msg = "WARNING: Running pip as the 'root' user can result" | ||
assert_msg = f"{bad_msg} is only tolerated from manylinux's ensure for graalpy" | ||
for m in re.finditer(bad_msg, stream): | ||
idx = stream.rfind("/opt/", start, m.end()) | ||
start = m.end() | ||
m2 = re.match( | ||
"/opt/.*/graalpy.*/bin/python -m ensurepip --default-pip", stream[idx : m.start()] | ||
) | ||
assert m2 is not None, assert_msg | ||
assert idx + m2.end() + 1 == m.start(), assert_msg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In light of the much longer CI times, I'm checking how not excluding graalpy from image warm-up impacts CI time on Linux, which would most likely make this workaround no-op:
diff --git a/test/conftest.py b/test/conftest.py
index 8c0c9504..f2b575df 100644
--- a/test/conftest.py
+++ b/test/conftest.py
@@ -56,9 +56,8 @@ def docker_warmup(request: pytest.FixtureRequest) -> None:
images = [build_options.manylinux_images[arch] for arch in archs] + [
build_options.musllinux_images[arch] for arch in archs
]
- # exclude GraalPy as it's not a target for cibuildwheel
command = (
- "manylinux-interpreters ensure $(manylinux-interpreters list 2>/dev/null | grep -v graalpy) &&"
+ "manylinux-interpreters ensure-all &&"
"cpython3.13 -m pip download -d /tmp setuptools wheel pytest"
)
for image in images:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied in a47e354 and I removed this workaround in the same commit.
Updating warm-up on Linux images allows to reduce CI times on Linux.
|
Sorry, I think I misunderstood your message, you meant me to apply @mayeut's patch, right @joerick? That's what I did now plus rebase. |
for more information, see https://pre-commit.ci
Signed-off-by: Henry Schreiner <[email protected]>
Thanks! |
This PR adds support for GraalPy.
A couple of questions:
CIBW_BUILD
variable explicitly.