Skip to content
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

tests: run on pyodide #4745

Merged
merged 3 commits into from
Jul 18, 2024
Merged

Conversation

henryiii
Copy link
Collaborator

Adding WebAssembly testing on Pyodide.

Description

Suggested changelog entry:

* Adding WASM testing.

@henryiii henryiii force-pushed the henryiii/tests/pyodide branch from 1932d36 to 0e4d3bd Compare July 14, 2023 20:08
@rwgk
Copy link
Collaborator

rwgk commented Jul 14, 2023

Do you already understand the Python 3.12 failures? When I saw them previously I decided to trigger a CI run with master as-is (#3939) and I don't see any errors there.

@henryiii henryiii force-pushed the henryiii/tests/pyodide branch from 0e4d3bd to cc090fc Compare July 16, 2023 03:10
include/pybind11/pybind11.h Outdated Show resolved Hide resolved
@henryiii
Copy link
Collaborator Author

I don't understand how this could be adding errors on 3.12. No code changes that I see. 🤷

@rwgk
Copy link
Collaborator

rwgk commented Jul 20, 2023

I don't understand how this could be adding errors on 3.12. No code changes that I see. 🤷

My best guess: some change in the environment triggers a latent bug in 3.12 (or how we use 3.12).

Could tests/pyproject.toml have something to do with it?

@henryiii henryiii force-pushed the henryiii/tests/pyodide branch from f327847 to c5b493c Compare August 7, 2023 19:58
@henryiii henryiii force-pushed the henryiii/tests/pyodide branch from c5b493c to 80618dd Compare November 15, 2023 21:22
@henryiii henryiii force-pushed the henryiii/tests/pyodide branch from 80618dd to b658159 Compare December 1, 2023 22:28
@henryiii henryiii force-pushed the henryiii/tests/pyodide branch from b658159 to f2536eb Compare March 26, 2024 07:43
@henryiii henryiii force-pushed the henryiii/tests/pyodide branch 2 times, most recently from 58117d8 to 3fc4ea6 Compare June 26, 2024 20:00
@henryiii
Copy link
Collaborator Author

henryiii commented Jun 26, 2024

Importing pytest or sys (probably pytest) inside test_embed causes Python 3.12 to segfault. b9b391c fixed the issues - we don't really need to do it, since we are avoiding these anyway on Pyodide. But why? How?

@henryiii
Copy link
Collaborator Author

And the interesting question: Should we add -fexceptions when building on emscripten in our CMake helpers? Is there a reason someone wouldn't want them (using them, which we do a lot, would cause crashes). @hoodmane?

@henryiii henryiii marked this pull request as ready for review June 26, 2024 21:33
henryiii added 3 commits July 18, 2024 14:15
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii henryiii force-pushed the henryiii/tests/pyodide branch from b9b391c to c9a83e5 Compare July 18, 2024 18:15
@henryiii henryiii merged commit a582ca8 into pybind:master Jul 18, 2024
87 checks passed
@henryiii henryiii deleted the henryiii/tests/pyodide branch July 18, 2024 18:50
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 18, 2024
import pytest

asyncio = pytest.importorskip("asyncio")
m = pytest.importorskip("pybind11_tests.async_module")

if sys.platform.startswith("emscripten"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome to have test coverage for WASM!

Sorry I missed that this PR was marked as ready for review (last time I looked there were still test failures).

Just a minor question:

Did you consider adding something like this to tests/env.py?

 import pytest

+EMSCRIPTEN = sys.platform.startswith("emscripten")
 LINUX = sys.platform.startswith("linux")
 MACOS = sys.platform.startswith("darwin")
 WIN = sys.platform.startswith("win32") or sys.platform.startswith("cygwin")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like these very much. It makes a bit of sense for WIN (since it's a bit lengthy and combining two conditions - which would be better written sys.platform.startswith(("win32", "cygwin")), and I'm not sure it's right - often Cygwin acts a bit more like Linux), but the others are not really much clearer than the standard expression.

henryiii added a commit that referenced this pull request Aug 12, 2024
* tests: run on pyodide

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

* ci: use cibuildwheel for pyodide test

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

* tests: revert changes to test_embed

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

---------

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Aug 13, 2024
henryiii added a commit that referenced this pull request Aug 13, 2024
* tests: run on pyodide

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

* ci: use cibuildwheel for pyodide test

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

* tests: revert changes to test_embed

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

---------

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
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.

2 participants