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

Create Unit tests for Pyodide #697

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
28 changes: 28 additions & 0 deletions .github/workflows/emscripten.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
name: WASM

on:
workflow_dispatch:
pull_request:
branches:
- master
- stable
- v*

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true

jobs:
build-wasm-emscripten:
name: Pyodide wheel
runs-on: ubuntu-22.04
steps:
- uses: actions/checkout@v4
with:
submodules: true
fetch-depth: 0

- uses: pypa/cibuildwheel@v2.20
with:
package-dir: tests
only: cp312-pyodide_wasm32
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ inter_module.dll
/src/nanobind/ext
/src/nanobind/src
/dist
/tests/dist
/bench
compile_commands.json

Expand All @@ -30,6 +31,7 @@ cmake_install.cmake
\.DS_Store
\.cmake
__pycache__
.pyodide-xbuildenv-*/
nanobind.egg-info
test_*_ext*.so
test_*_ext*.pyd
Expand Down
3 changes: 2 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,8 @@ if (NOT TARGET Python::Module OR NOT TARGET Python::Interpreter)

find_package(Python 3.8
REQUIRED COMPONENTS Interpreter ${NB_PYTHON_DEV_MODULE}
OPTIONAL_COMPONENTS Development.SABIModule)
OPTIONAL_COMPONENTS Development.SABIModule
GLOBAL)
Copy link
Owner

Choose a reason for hiding this comment

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

this GLOBAL parameter exists starting in CMake 3.24, while this file declares

cmake_minimum_required(VERSION 3.15...3.27)

What are the implications?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to try to experiment a bit here in a day or two and see if I can either make the test subdirectory build nicer or build from the parent directory.

endif()

# ---------------------------------------------------------------------------
Expand Down
13 changes: 11 additions & 2 deletions cmake/nanobind-config.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,11 @@ function (nanobind_build_library TARGET_NAME)
target_link_options(${TARGET_NAME} PUBLIC $<${NB_OPT_SIZE}:-Wl,--gc-sections>)
endif()

if (CMAKE_SYSTEM_NAME MATCHES Emscripten)
target_compile_options(${TARGET_NAME} PUBLIC -fexceptions)
target_link_options(${TARGET_NAME} PUBLIC -fexceptions)
endif()

set_target_properties(${TARGET_NAME} PROPERTIES
POSITION_INDEPENDENT_CODE ON)

Expand Down Expand Up @@ -228,9 +233,10 @@ function (nanobind_build_library TARGET_NAME)
${NB_DIR}/ext/robin_map/include)
endif()

get_property(nanobind_python_headers TARGET Python::Module PROPERTY INTERFACE_INCLUDE_DIRECTORIES)
Copy link
Owner

Choose a reason for hiding this comment

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

Following the style of the rest of the code, how about renaming this to NB_PYTHON_HEADERS?

Copy link
Owner

Choose a reason for hiding this comment

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

I would also like to know: under what conditions does the new code lead to behavior that is different from what was there before?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the headers and targets are not defined, then this produces an error, while the other one silently adds an empty string and proceeds to fail much later with a Python.h not found error.

target_include_directories(${TARGET_NAME} PUBLIC
${Python_INCLUDE_DIRS}
${NB_DIR}/include)
"${nanobind_python_headers}"
"${NB_DIR}/include")

target_compile_features(${TARGET_NAME} PUBLIC cxx_std_17)
nanobind_set_visibility(${TARGET_NAME})
Expand Down Expand Up @@ -275,6 +281,9 @@ function (nanobind_compile_options name)
if (MSVC)
target_compile_options(${name} PRIVATE $<$<COMPILE_LANGUAGE:CXX>:/bigobj /MP>)
endif()
if (CMAKE_SYSTEM_NAME MATCHES Emscripten)
Copy link
Owner

Choose a reason for hiding this comment

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

Is this line needed? We already declared this as a PUBLIC exported flag of the nanobind library target, which is also inherited by anything else consuming the target.

target_compile_options(${name} PUBLIC $<$<COMPILE_LANGUAGE:CXX>:-fexceptions>)
endif()
endfunction()

function (nanobind_strip name)
Expand Down
97 changes: 82 additions & 15 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,11 +1,32 @@
include_guard(GLOBAL)
Copy link
Owner

Choose a reason for hiding this comment

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

This line seems superfluous, nobody will double-include this file.


cmake_minimum_required(VERSION 3.15...3.27)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm confused about these lines. It looks to me like this sets up an independent top-level project, but I don't understand why. The tests/CMakeLists.txt file is part of the parent CMakeLists.txt build system.

project(nanobind_tests LANGUAGES CXX)

if(PROJECT_NAME STREQUAL CMAKE_PROJECT_NAME)
Copy link
Owner

Choose a reason for hiding this comment

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

This looks complicated to me, and I don't understand why it is needed.

set(NB_TESTS ON)
if(APPLE)
set(BASEPOINT @loader_path)
else()
set(BASEPOINT $ORIGIN)
endif()
set(CMAKE_INSTALL_RPATH ${BASEPOINT} ${BASEPOINT}/${CMAKE_INSTALL_LIBDIR})

add_subdirectory(.. ./build)
endif()

# If leaks are found, abort() during interpreter shutdown to catch this in the CI
add_definitions(-DNB_ABORT_ON_LEAK)

if (EMSCRIPTEN)
set(NB_EXTRA_ARGS ${NB_EXTRA_ARGS} NB_STATIC)
endif()

if (NB_TEST_STABLE_ABI)
set(NB_EXTRA_ARGS ${NB_EXTRA_ARGS} STABLE_ABI)
endif()

if (NB_TEST_SHARED_BUILD)
if (NB_TEST_SHARED_BUILD AND NOT EMSCRIPTEN)
set(NB_EXTRA_ARGS ${NB_EXTRA_ARGS} NB_SHARED)
endif()

Expand Down Expand Up @@ -58,22 +79,26 @@ foreach (NAME functions classes ndarray stl enum typing make_iterator)
set(PYI_PREFIX $<CONFIG>/)
endif()

nanobind_add_stub(
${NAME}_ext_stub
MODULE test_${NAME}_ext
OUTPUT ${PYI_PREFIX}test_${NAME}_ext.pyi
PYTHON_PATH $<TARGET_FILE_DIR:test_${NAME}_ext>
DEPENDS test_${NAME}_ext
${EXTRA})
if(NOT CMAKE_CROSSCOMPILING)
nanobind_add_stub(
${NAME}_ext_stub
MODULE test_${NAME}_ext
OUTPUT ${PYI_PREFIX}test_${NAME}_ext.pyi
PYTHON_PATH $<TARGET_FILE_DIR:test_${NAME}_ext>
DEPENDS test_${NAME}_ext
${EXTRA})
endif()
endforeach()

nanobind_add_stub(
py_stub
MODULE py_stub_test
OUTPUT ${PYI_PREFIX}py_stub_test.pyi
PYTHON_PATH $<TARGET_FILE_DIR:test_stl_ext>
DEPENDS py_stub_test.py
)
if(NOT CMAKE_CROSSCOMPILING)
nanobind_add_stub(
py_stub
MODULE py_stub_test
OUTPUT ${PYI_PREFIX}py_stub_test.pyi
PYTHON_PATH $<TARGET_FILE_DIR:test_stl_ext>
DEPENDS py_stub_test.py
)
endif()

find_package (Eigen3 3.3.1 NO_MODULE)
if (TARGET Eigen3::Eigen)
Expand Down Expand Up @@ -147,3 +172,45 @@ if (NOT (CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_CURRENT_BINARY_DIR) OR MSVC)

add_custom_target(copy-tests ALL DEPENDS ${TEST_FILES_OUT})
endif()

if(DEFINED SKBUILD)
if(NOT CMAKE_CROSSCOMPILING)
install(
FILES
"${CMAKE_CURRENT_BINARY_DIR}/test_functions_ext.pyi"
"${CMAKE_CURRENT_BINARY_DIR}/test_stl_ext.pyi"
"${CMAKE_CURRENT_BINARY_DIR}/test_typing_ext.pyi"
"${CMAKE_CURRENT_BINARY_DIR}/test_enum_ext.pyi"
"${CMAKE_CURRENT_BINARY_DIR}/test_ndarray_ext.pyi"
"${CMAKE_CURRENT_BINARY_DIR}/test_make_iterator_ext.pyi"
"${CMAKE_CURRENT_BINARY_DIR}/test_classes_ext.pyi"
"${CMAKE_CURRENT_BINARY_DIR}/py_stub_test.pyi"
DESTINATION
.
)
endif()
install(
TARGETS
inter_module
Copy link
Owner

Choose a reason for hiding this comment

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

There is a bit too much repetition of test names in this project. How about defining them as a list and then creating the build and install targets by pasting the list or iterating over elements.

Copy link
Contributor

Choose a reason for hiding this comment

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

I love list(TRANSFORM (CMake 3.12+) :)

test_inter_module_1_ext
test_inter_module_2_ext
test_functions_ext
test_classes_ext
test_holders_ext
test_stl_ext
test_bind_map_ext
test_bind_vector_ext
test_chrono_ext
test_enum_ext
test_eval_ext
test_ndarray_ext
test_intrusive_ext
test_exception_ext
test_make_iterator_ext
test_typing_ext
test_issue_ext
DESTINATION
.
)
endif()

24 changes: 24 additions & 0 deletions tests/pyproject.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Warning: this is currently used for pyodide, and is not a general out-of-tree
# builder for the tests (yet). Specifically, wheels can't be built from SDists.

[build-system]
requires = ["scikit-build-core>=0.10"]
build-backend = "scikit_build_core.build"

[project]
name = "nanobind_tests"
version = "0.0.1"
dependencies = ["pytest", "pytest-timeout", "numpy", "scipy"]
classifiers = [
"Private :: Do Not Upload",
]

[tool.scikit-build]
minimum-version = "build-system.requires"

[tool.cibuildwheel]
test-command = "pytest -o timeout=0 -p no:cacheprovider {project}/tests/test_*.py"

[[tool.cibuildwheel.overrides]]
select = ["*-pyodide_wasm32"]
environment.PYODIDE_BUILD_EXPORTS = "whole_archive"
7 changes: 5 additions & 2 deletions tests/test_stubs.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@
import platform
import pytest

is_unsupported = platform.python_implementation() == 'PyPy' or sys.version_info < (3, 10)
import test_typing_ext
LIB_DIR = pathlib.Path(test_typing_ext.__file__).parent.resolve()

is_unsupported = platform.python_implementation() == 'PyPy' or sys.version_info < (3, 10) or sys.platform.startswith("emscripten")
skip_on_unsupported = pytest.mark.skipif(
is_unsupported, reason="Stub generation is only tested on CPython >= 3.10.0")

Expand Down Expand Up @@ -33,7 +36,7 @@ def test01_check_stub_refs(p_ref):
"""
Check that generated stub files match reference input
"""
p_in = p_ref.with_suffix('')
p_in = LIB_DIR / p_ref.with_suffix('').name
with open(p_ref, 'r') as f:
s_ref = f.read().split('\n')
with open(p_in, 'r') as f:
Expand Down