-
Notifications
You must be signed in to change notification settings - Fork 195
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
base: master
Are you sure you want to change the base?
Changes from 10 commits
b3afc56
8dc1fa4
4d51d18
5a993a9
bb3c356
4555cd0
b3cd0f7
def8aef
ec29f2d
d7282b9
15ebe0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}) | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this line needed? We already declared this as a |
||
target_compile_options(${name} PUBLIC $<$<COMPILE_LANGUAGE:CXX>:-fexceptions>) | ||
endif() | ||
endfunction() | ||
|
||
function (nanobind_strip name) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,32 @@ | ||
include_guard(GLOBAL) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
project(nanobind_tests LANGUAGES CXX) | ||
|
||
if(PROJECT_NAME STREQUAL CMAKE_PROJECT_NAME) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
|
||
|
@@ -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) | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I love |
||
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() | ||
|
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" |
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.
this GLOBAL parameter exists starting in CMake 3.24, while this file declares
What are the implications?
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.
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.