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

Emscripten #130

wants to merge 12 commits into from

Conversation

DerThorsten
Copy link
Collaborator

  • emscripten build / workflow

@@ -467,7 +467,7 @@ namespace sparrow
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.

@DerThorsten
Copy link
Collaborator Author

atm still all timezone tests fail with:

===============================================================================
/Users/thorstenbeier/src/sparrow/test/test_typed_array_timestamp.cpp:41:
TEST SUITE: typed_array_timestamp
TEST CASE:  constructor with parameter

/Users/thorstenbeier/src/sparrow/test/test_typed_array_timestamp.cpp:41: ERROR: test case THREW exception: discover_tz_dir failed to find zoneinfo


===============================================================================

This is why I currently filter out these tests with:

node build/test/test_sparrow_lib.js -tse=typed_array_timestamp 

-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

@@ -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

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.

None yet

4 participants