[master < T0792-MG] Add wasm compilation target to mgclient by kostasrim · Pull Request #35 · memgraph/mgclient

kostasrim · 2022-01-19T12:43:27Z

No description provided.

gitbuda · 2022-01-19T12:47:36Z

🎉💪

kostasrim · 2022-01-19T12:50:52Z

                      socklen_t addrlen) {
  long status = MG_RETRY_ON_EINTR(connect(sock, addr, addrlen));
-  if (status == -1L) {
+  if (status == -1L && errno != EINPROGRESS) {


Add this only for __EMSCRIPTEN__

I would either introduce a function to check the status and it would be defined differently based on EMSCRIPTEN is used or not, or put a comment about this to the source.

gitbuda · 2022-02-14T13:50:46Z

 endif()

-generate_export_header(mgclient-shared
+generate_export_header(mgclient-static


Why is this changed? We have to be careful with these things because maybe other clients depend on precisely this?

We will have it static only for emcripten builds ;)

gitbuda

Since the compilation of webasm is only possible on Linux for now, I think we should implement a bash script to install the SDK (and all related deps) + do the actual compilation. In addition, the GHA setup has to be extended with that workload/job. Once all that is in place, the same code could be used inside jsmgclient to build the actual package (or any other dependent client).

gitbuda · 2022-03-09T08:14:58Z

Add CI job to run WASM build 😄

The hard-coded paths in the WASM scripts are not great. You can use the following

DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
cd "$DIR"

to make the script independent of where it is run and then create a bin folder (git ignore it) to compile stuff inside it.

jbajic

Just small comments, but otherwise fine from my side.

antaljanosbenjamin

I think the changes contain a lot if improvements, however I think this PR still needs a few changes. In my mind the goal is to reduce the usage of ifdef in files:

mgwasm.h doesn't need to be protected with ifdefs
mg_transport_yield_until_async_xxx functions should be usable in not WASM builds, so there is no need to ifdef them out.

For the second point I think we still miss a level of indirection as I mentioned in my comments.

antaljanosbenjamin

Two small changes!

antaljanosbenjamin

LGTM!

gitbuda

🚀:ship:

kostasrim commented Jan 19, 2022

View reviewed changes

Comment thread src/linux/mgsocket.c Outdated

kostasrim force-pushed the T0792-MG-add-wasm-compilation-target-to-mgclient branch from f2ef513 to c4746ab Compare January 19, 2022 13:28

gitbuda reviewed Jan 25, 2022

View reviewed changes

Comment thread README.md Outdated

gitbuda reviewed Jan 25, 2022

View reviewed changes

Comment thread README.md Outdated

gitbuda reviewed Feb 14, 2022

View reviewed changes

Comment thread CMakeLists.txt

gitbuda reviewed Feb 14, 2022

View reviewed changes

gitbuda requested changes Feb 14, 2022

View reviewed changes

gitbuda reviewed Feb 14, 2022

View reviewed changes

Comment thread README.md Outdated

kostasrim force-pushed the T0792-MG-add-wasm-compilation-target-to-mgclient branch from ef84091 to cf3110d Compare March 8, 2022 20:05

kostasrim marked this pull request as ready for review March 8, 2022 21:18

kostasrim requested a review from gitbuda March 8, 2022 21:18

kostasrim self-assigned this Mar 8, 2022

kostasrim requested review from antaljanosbenjamin and jbajic March 8, 2022 21:19

kostasrim commented Mar 8, 2022

View reviewed changes

Comment thread src/mgclient.c Outdated

antaljanosbenjamin suggested changes Mar 9, 2022

View reviewed changes

jbajic reviewed Mar 9, 2022

View reviewed changes

Comment thread CMakeLists.txt Outdated

Comment thread wasm/install_deps.sh Outdated

Comment thread src/mgclient.c

Comment thread src/mgtransport.c Outdated

Comment thread src/mgwasm.c Outdated

Comment thread src/mgwasm.c Outdated

kostasrim force-pushed the T0792-MG-add-wasm-compilation-target-to-mgclient branch 9 times, most recently from 3a66b15 to ffd0fd2 Compare March 10, 2022 12:38

jbajic reviewed Mar 10, 2022

View reviewed changes

Comment thread .github/workflows/ci.yml Outdated

Comment thread .github/workflows/ci.yml Outdated

Comment thread .github/workflows/ci.yml

jbajic approved these changes Mar 14, 2022

View reviewed changes

Comment thread src/mgwasm.c Outdated

Comment thread src/mgwasm.c

antaljanosbenjamin reviewed Mar 14, 2022

View reviewed changes

Comment thread src/mgwasm.c Outdated

Comment thread src/mgtransport.h

Comment thread src/mgwasm.h Outdated

antaljanosbenjamin suggested changes Mar 14, 2022

View reviewed changes

Comment thread src/mgtransport.h Outdated

Comment thread src/mgclient.c Outdated

Comment thread src/mgwasm.h Outdated

Comment thread src/mgclient.c

kostasrim requested a review from antaljanosbenjamin March 15, 2022 12:43

antaljanosbenjamin suggested changes Mar 15, 2022

View reviewed changes

Comment thread README.md Outdated

Comment thread .github/workflows/ci.yml

Comment thread .gitignore Outdated

Comment thread src/mgsession.c Outdated

Comment thread src/mgtransport.c

kostasrim added 17 commits March 15, 2022 17:18

mgclient wasm poc draft

3251400

Fix missing mg-shared

73a83d9

Fix CMake

56cf72d

Added full integration of emscripten with cmake

7b163f0

Added scripts for wasm deps

bf1cc15

updated Readme

6d81f92

Addressed comments and added workflow on gh action

6c99edf

workflow patch

26c9879

Addressed comments and simplified CI

fa13497

fix CI and improvements

2b8e52b

removed unused activate.sh

b25d731

add consistency to ci.yml

918a2f2

addressed all comments

dfc58d2

Addressed comments and fixed refactoring bug

d30b022

added install deps config step in cmake for wasm build

100e98f

addressed comments

4c41572

addressed comments

885f4ff

kostasrim force-pushed the T0792-MG-add-wasm-compilation-target-to-mgclient branch from 3c8a928 to 885f4ff Compare March 15, 2022 15:18

fix client ub in tests/client.cpp

3ca0917

kostasrim requested a review from antaljanosbenjamin March 15, 2022 16:03

antaljanosbenjamin approved these changes Mar 15, 2022

View reviewed changes

gitbuda approved these changes Mar 15, 2022

View reviewed changes

kostasrim changed the title ~~[E104-MG < T0792-MG] Add wasm compilation target to mgclient~~ [master < T0792-MG] Add wasm compilation target to mgclient Mar 16, 2022

kostasrim merged commit 6bd4891 into master Mar 16, 2022

kostasrim deleted the T0792-MG-add-wasm-compilation-target-to-mgclient branch March 16, 2022 18:17

Conversation

kostasrim commented Jan 19, 2022

Uh oh!

gitbuda commented Jan 19, 2022

Uh oh!

kostasrim Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

antaljanosbenjamin Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gitbuda Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

kostasrim Mar 4, 2022

Choose a reason for hiding this comment

Uh oh!

gitbuda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gitbuda commented Mar 9, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbajic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antaljanosbenjamin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antaljanosbenjamin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antaljanosbenjamin left a comment

Choose a reason for hiding this comment

Uh oh!

gitbuda left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects