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

Emscripten #130

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions .github/workflows/emscripten.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
name: Emscripten
on:
workflow_dispatch:
pull_request:
push:
branches: [main]
concurrency:
group: ${{ github.workflow }}-${{ github.job }}-${{ github.ref }}
cancel-in-progress: true
defaults:
run:
shell: bash -e -l {0}
jobs:
build:
runs-on: ubuntu-22.04

steps:
- name: Checkout code
uses: actions/checkout@v3

- name: Set conda environment
uses: mamba-org/setup-micromamba@main
with:
environment-name: myenv
environment-file: environment-dev.yml
init-shell: bash
cache-downloads: true

- name: install emscripten
run: |
git clone https://github.com/emscripten-core/emsdk
cd emsdk
./emsdk install latest
./emsdk activate latest
source ./emsdk_env.sh

- name: create wasm prefix
run: |
$MAMBA_EXE create -p $(pwd)/wasm_prefix \
--platform=emscripten-wasm32 \
-c https://repo.mamba.pm/emscripten-forge \
-c https://repo.mamba.pm/conda-forge \
--yes \
howardhinnant_date doctest

- name: build
run: |

source emsdk/emsdk_env.sh
export PREFIX=$(pwd)/wasm_prefix
export CMAKE_PREFIX_PATH=$PREFIX
export CMAKE_SYSTEM_PREFIX_PATH=$PREFIX

mkdir build
cd build

emcmake cmake \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ON \
-DCMAKE_INSTALL_PREFIX=$PREFIX \
-DBUILD_TESTS=ON \
..

emmake make -j9 install
Copy link
Collaborator

Choose a reason for hiding this comment

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

why j9 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

...leftover from copy/pasting


- name: run tests
run: |
# all teswts with timestamps fail
node build/test/test_sparrow_lib.js -tse=typed_array_timestamp

3 changes: 3 additions & 0 deletions environment-dev.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,11 @@ dependencies:
- ninja
# Tests
- doctest
# Wasm Tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to have a condition to not get nodejs if we don't use emscripten ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we could introduce install node at runtime in the ci

- nodejs
# P0355R7 (Extending chrono to Calendars and Time Zones) has not been entirely implemented in libc++ yet.
# See: https://libcxx.llvm.org/Status/Cxx20.html#note-p0355
# For now, we use HowardHinnant/date as a replacement if we are compiling with libc++.
# TODO: remove this once libc++ has full support for P0355R7.
- howardhinnant_date

2 changes: 1 addition & 1 deletion include/sparrow/typed_array.hpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Man Group Operations Limited

Check notice on line 1 in include/sparrow/typed_array.hpp

View workflow job for this annotation

GitHub Actions / build

Run clang-format on include/sparrow/typed_array.hpp

File include/sparrow/typed_array.hpp does not conform to Custom style guidelines. (lines 468, 470, 471, 472, 478, 479)
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -467,7 +467,7 @@
bool operator==(const typed_array<T, Layout>& ta1, const typed_array<T, Layout>& ta2)
{
// see https://github.com/man-group/sparrow/issues/108
#if defined(_LIBCPP_VERSION) && (_LIBCPP_VERSION < 180000)
#if EMSCRIPTEN ||( defined(_LIBCPP_VERSION) && (_LIBCPP_VERSION < 180000))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's possible that you won't need that if <version> is included.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I think it's better to not have this check (also it should be defined(EMSCRIPTEN)?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems this was removed and re-added. Can we confirm that it works without this check if <version> is included?

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 tried with including version 19cb95b but this failed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah it seems that _LIBCPP_VERSION is not defined with Emscripten.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm ok then EMSCRIPTEN is probably the right define do check I suppose.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we know if a clang18-based emscripten would also require this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we know if a clang18-based emscripten would also require this?

we don't know. Atm we use the emscripten version of emscripten-forge (3.1.45).

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's only to know if there should be an upper bound in these checks.

if(ta1.size() != ta2.size())
{
return false;
Expand Down
17 changes: 16 additions & 1 deletion test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ add_executable(${test_target} ${SPARROW_TESTS_SOURCES})
target_link_libraries(${test_target} PRIVATE sparrow doctest::doctest)
add_test(NAME ${test_target} COMMAND ${test_target})

set(link_options)

if (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
set(compiles_options
/permissive-
Expand Down Expand Up @@ -113,9 +115,22 @@ elseif(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" OR CMAKE_CXX_COMPILER_ID STREQUAL "C
$<$<CXX_COMPILER_ID:GNU>:-Wduplicated-cond> # warn if if / else chain has duplicated conditions
$<$<CXX_COMPILER_ID:GNU>:-Wlogical-op> # warn about logical operations being used where bitwise were probably wanted
$<$<CXX_COMPILER_ID:GNU>:-Wuseless-cast>) # warn if you perform a cast to the same type
endif()



if(EMSCRIPTEN)
message(STATUS "Emscripten detected")
set(compiles_options ${compiles_options} -Wno-shorten-64-to-32)

# enable exceptions
set(compiles_options ${compiles_options} -fexceptions)
set(link_options ${link_options} -fexceptions -s WASM_BIGINT)
endif()

endif()

target_compile_options(${test_target} PRIVATE ${compiles_options})
target_link_options(${test_target} PRIVATE ${link_options})

# We do not use non-standard C++
set_target_properties(${test_target} PROPERTIES CMAKE_CXX_EXTENSIONS OFF)
Expand Down
2 changes: 2 additions & 0 deletions test/test_iterator.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Man Group Operations Limited

Check notice on line 1 in test/test_iterator.cpp

View workflow job for this annotation

GitHub Actions / build

Run clang-format on test/test_iterator.cpp

File test/test_iterator.cpp does not conform to Custom style guidelines. (lines 239, 242)
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -237,8 +237,10 @@

TEST_CASE_FIXTURE(iterator_fixture, "contiguous iterator")
{
#ifndef EMSCRIPTEN
Klaim marked this conversation as resolved.
Show resolved Hide resolved
constexpr bool valid = std::contiguous_iterator<test_iterator>;
CHECK(valid);
#endif
}
}

Expand Down
Loading