Build / install issue

[benbenz]

Hello,

Thanks for making this repository!

I'm having trouble installing/running the repository. When following the install directions (v1), it seems git is not finding a valid git repository. When trying to install after downloading the repository (v2), the submodules are properly installed but the build is failing because build.ninja is missing ... ? I know I do not have perl6 installed yet but it doesn't seem to be related to this quite yet and I wanted to try with the default perl for now...

OS: Mac OSX 10.13.6 (High Sierra) Node: Node v16.14.0 NPM: v8.3.1 protoc: libprotoc 3.19.4 ninja: 1.10.2 Go: go1.17.8 darwin/amd64 cmake-js: 6.3.0 cmake: 3.23.0-rc2 clang: Apple LLVM version 10.0.0 (clang-1000.11.45.2) perl: v5.18.2

STEPS v1:

mkdir quic-test
cd quic-test
echo "@fails-components:registry=https://npm.pkg.github.com" > .npmrc
npm init
git init
npm install --save https://github.com/fails-components/webtransport

ERROR v1:

npm ERR! code 128
npm ERR! git dep preparation failed
npm ERR! command /usr/local/bin/node /usr/local/lib/node_modules/npm/bin/npm-cli.js install --force --cache=/Users/benoit/.npm --prefer-offline=false --prefer-online=false --offline=false --no-progress --no-save --no-audit --include=dev --include=peer --include=optional --no-package-lock-only --no-dry-run
npm ERR! > @fails-components/webtransport@0.0.0 preinstall
npm ERR! > git submodule update -i -r
npm ERR! npm WARN using --force Recommended protections disabled.
npm ERR! fatal: not a git repository (or any of the parent directories): .git
npm ERR! npm ERR! code 128
npm ERR! npm ERR! path /Users/benoit/.npm/_cacache/tmp/git-cloneVgtOkV
npm ERR! npm ERR! command failed
npm ERR! npm ERR! command sh -c git submodule update -i -r
npm ERR! 
npm ERR! npm ERR! A complete log of this run can be found in:
npm ERR! npm ERR!     /Users/benoit/.npm/_logs/2022-03-08T11_55_48_125Z-debug-0.log

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/benoit/.npm/_logs/2022-03-08T11_52_37_208Z-debug-0.log

debug build file  ...

394 verbose stack Error: command failed
394 verbose stack     at ChildProcess.<anonymous> (/usr/local/lib/node_modules/npm/node_modules/@npmcli/promise-spawn/index.js:64:27)
394 verbose stack     at ChildProcess.emit (node:events:520:28)
394 verbose stack     at maybeClose (node:internal/child_process:1092:16)
394 verbose stack     at Process.ChildProcess._handle.onexit (node:internal/child_process:302:5)
395 verbose pkgid @fails-components/webtransport@0.0.0
396 verbose cwd /Users/benoit/.npm/_cacache/tmp/git-cloneVgtOkV
397 verbose Darwin 17.7.0
398 verbose argv "/usr/local/bin/node" "/usr/local/lib/node_modules/npm/bin/npm-cli.js" "install" "--force" "--cache=/Users/benoit/.npm" "--prefer-offline=false" "--prefer-online=false" "--offline=false" "--no-progress" "--no-save" "--no-audit" "--include=dev" "--include=peer" "--include=optional" "--no-package-lock-only" "--no-dry-run"
399 verbose node v16.14.0
400 verbose npm  v8.3.1
401 error code 128
402 error path /Users/benoit/.npm/_cacache/tmp/git-cloneVgtOkV
403 error command failed
404 error command sh -c git submodule update -i -r

STEPS v2:

git clone https://github.com/fails-components/webtransport.git
cd webtransport
npm run preinstall
npm run install

ERROR v2:

> @fails-components/webtransport@0.0.0 preinstall
> git submodule update -i -r

> @fails-components/webtransport@0.0.0 install
> cmake-js build

[ '/usr/local/bin/node', '/usr/local/bin/cmake-js', 'build' ]
info TOOL Using Ninja generator, because ninja is available.
info CMD BUILD
info RUN [
info RUN   'cmake',
info RUN   '--build',
info RUN   '/Users/benoit/dev/quic-test/webtransport/build',
info RUN   '--config',
info RUN   'Release'
info RUN ]
ninja: error: loading 'build.ninja': No such file or directory
ERR! OMG Process terminated: 1

[benbenz]

Test seems to be ALL GOOD :D Awesome ! Ill make further test to see if I get locks / seg faults etc.

[benbenz]

I get this error/warning at runtime:

          const {value,done} = await reader.read().catch(this.onError) ;    
           ^
TypeError: Cannot destructure property 'value' of '(intermediate value)' as it is undefined.

As there been a WebTransport syntax change?

[martenrichter]

Thanks! For testing, it is good to hear that the tests passed.

Regarding the error, that can not be caused by the current change (I think). The reader is also coming from the readable object, which should be supplied from node itself. Anyway to identify the cause, please try it on the development branch and may be on the release tags of the last versions. Indeed I changed the behaviour of the webtransport object, since it was not behaving properly with stream closes, and now I am closer to the spec. This may have introduced some new problems. (What reader is it? The readable from a stream or the readable to get the streams?)

[benbenz]

Yes it is great :) Thank you ! This error is when I retrieve the read the actual stream content (readable from a stream). Do you think it is possible to incorporate the CMakeLists.txt changes above. Do you think there is a way to automate with CMake the generation of event-config.h as well ?

For the windows branch, I get the following error:

[  0%] Building CXX object CMakeFiles/icu.dir/third_party/icu/icu4c/source/common/appendable.cpp.o
In file included from /Users/benoit/dev/webtransport/third_party/icu/icu4c/source/common/appendable.cpp:17:
/Users/benoit/dev/webtransport/third_party/icu/icu4c/source/common/unicode/utypes.h:38:10: fatal error: 'unicode/umachine.h' file not found
#include "unicode/umachine.h"
         ^~~~~~~~~~~~~~~~~~~~
1 error generated.
make[2]: *** [CMakeFiles/icu.dir/third_party/icu/icu4c/source/common/appendable.cpp.o] Error 1

I will test against v0.0.2 and development and see if there is the same error.

[martenrichter]

Yes it is great :) Thank you ! This error is when I retrieve the read the actual stream content (readable from a stream). Ok, really strange, since I use the same code in the tests. All that is changed, that the stream now closes....

Do you think it is possible to incorporate the CMakeLists.txt changes above. I think all important ones are still incorporated. The protobuf are now directly included in the third_party directory and in the build. So that the protobuf related changes, do not apply any more.... Do you think there is a way to automate with CMake the generation of event-config.h as well ? I do not understand, on windows and linux, the libevent tested many properties and I think generated it as well. Did you do a rebuild?

For the windows branch, I get the following error:

[  0%] Building CXX object CMakeFiles/icu.dir/third_party/icu/icu4c/source/common/appendable.cpp.o
In file included from /Users/benoit/dev/webtransport/third_party/icu/icu4c/source/common/appendable.cpp:17:
/Users/benoit/dev/webtransport/third_party/icu/icu4c/source/common/unicode/utypes.h:38:10: fatal error: 'unicode/umachine.h' file not found
#include "unicode/umachine.h"
         ^~~~~~~~~~~~~~~~~~~~
1 error generated.
make[2]: *** [CMakeFiles/icu.dir/third_party/icu/icu4c/source/common/appendable.cpp.o] Error 1

I do not understand, the icu build does not find its own included. Hmm may be you should add for all platforms:

PUBLIC third_party/icu/icu4c/source/common)

right after the add library for the icu. If it helps, I will add it to cmake list.

I will test against v0.0.2 and development and see if there is the same error. And it it is than use the v0.0.1 I believe it was introduced between the two releases.

[benbenz]

The protobuf are now directly included in the third_party directory and in the build. So that the protobuf related changes, do not apply any more....

I meant for the libevent header files, as well as quiche headers - line 807 of CmakeLists.txt I've added:

PUBLIC third_party/libevent/include
PUBLIC third_party

The first line is so that event2/*.hare found The second line is because quiche was not finding quiche/quic/core/proto/cached_network_parameters.pb.h (error details in my comment above). This one is more of a hack because we shouldn't add this directory as includes root.

I do not understand, on windows and linux, the libevent tested many properties and I think generated it as well. Did you do a rebuild?

I ran a full rebuild. Also running git submodule update --init --recursive every time I switch The problem is I that I had to "bootstrap" libevent manually so that event-config.h gets created. CMake didn't seem to trigger this.

I do not understand, the icu build does not find its own included. Hmm may be you should add for all platforms:

PUBLIC third_party/icu/icu4c/source/common)

Adding target_include_directories(icu PUBLIC third_party/icu/icu4c/source/common) line 134 (after the add_library) did fix the issue. The project compiles fine. But I get a runtime missing symbol error:

Error: dlopen(/Users/benoit/dev/webtransport/build/Release/webtransport.node, 1): Symbol not found: _icudt72_dat
  Referenced from: /Users/benoit/dev/webtransport/build/Release/webtransport.node
  Expected in: flat namespace
 in /Users/benoit/dev/webtransport/build/Release/webtransport.node
    at Object.Module._extensions..node (node:internal/modules/cjs/loader:1189:18)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at file:///Users/benoit/dev/webtransport/src/webtransport.js:21:18
    at ModuleJob.run (node:internal/modules/esm/module_job:198:25)
    at async Promise.all (index 0)
    at ESMLoader.import (node:internal/modules/esm/loader:385:24)
    at importModuleDynamicallyWrapper (node:internal/vm/module:437:15)

And it it is than use the v0.0.1 I believe it was introduced between the two releases.

On development, I don't get the error. On v0.0.1, I don't get the error.

[martenrichter]

The protobuf are now directly included in the third_party directory and in the build. So that the protobuf related changes, do not apply any more....

I meant for the libevent header files, as well as quiche headers - line 807 of CmakeLists.txt I've added:

PUBLIC third_party/libevent/include
PUBLIC third_party

Can you please recheck the line number? It is in the middle of the add library of libquiche... Also the line above is already present around line 950.

The first line is so that event2/*.hare found The second line is because quiche was not finding quiche/quic/core/proto/cached_network_parameters.pb.h (error details in my comment above). This one is more of a hack because we shouldn't add this directory as includes root. Actually includes root was always included see the . folder, but I have currently no clue.

I do not understand, on windows and linux, the libevent tested many properties and I think generated it as well. Did you do a rebuild?

I ran a full rebuild. Also running git submodule update --init --recursive every time I switch The problem is I that I had to "bootstrap" libevent manually so that event-config.h gets created. CMake didn't seem to trigger this.

On linux and windows it did. Did you see the long annoying tests for the various defines and presence test of certain functions? The questions is what the mac needs...

Adding target_include_directories(icu PUBLIC third_party/icu/icu4c/source/common) line 134 (after the add_library) did fix the issue. The project compiles fine. But I get a runtime missing symbol error:

Error: dlopen(/Users/benoit/dev/webtransport/build/Release/webtransport.node, 1): Symbol not found: _icudt72_dat
  Referenced from: /Users/benoit/dev/webtransport/build/Release/webtransport.node
  Expected in: flat namespace
 in /Users/benoit/dev/webtransport/build/Release/webtransport.node
    at Object.Module._extensions..node (node:internal/modules/cjs/loader:1189:18)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at file:///Users/benoit/dev/webtransport/src/webtransport.js:21:18
    at ModuleJob.run (node:internal/modules/esm/module_job:198:25)
    at async Promise.all (index 0)
    at ESMLoader.import (node:internal/modules/esm/loader:385:24)
    at importModuleDynamicallyWrapper (node:internal/vm/module:437:15)

Hmm, this is the starting point of another icu lib. May be we should build icu only manually on windows. Can you use some ifs and use find_package for icu lib on mac (and linux)...

On development, I don't get the error. On v0.0.1, I don't get the error.

Development should be after v0.0.2..., so it is introduced in the libevent branch? I can't believe this, I do not think I have touched the javascript. May be we should compare the files?

[martenrichter]

Regarding event-config.h, did you have a recent python version installed, libevent uses python for these tests? And I remember cmake complained about it.

[martenrichter]

For diagnosis of the problem, maybe put a console.log(args) to onStreamRead in webtransport.js. If this gives no insight, put a printout to every function in webtransport.js, that uses readable or readableController. These are the only paths, that can cause trouble. Or the changes in building result in trouble for node.js itself, by replacing some libs with our versions...., put the linker should detect this.

[benbenz]

Can you please recheck the line number? It is in the middle of the add library of libquiche... Also the line above is already present around line 950.

eventloop branch This is weird. I have this around line 807 (after modification):

target_include_directories(gquiche
PUBLIC third_party/quiche
PUBLIC third_party/boringssl/src/include
PUBLIC third_party/abseil-cpp
PUBLIC third_party/googleurl
PUBLIC third_party/libevent/include     <---- line 807
PUBLIC third_party
PUBLIC platform
PUBLIC .
PUBLIC ${Protobuf_INCLUDE_DIRS}
PUBLIC ${CMAKE_CURRENT_BINARY_DIR}
PUBLIC ${CMAKE_JS_INC}
)

This seems to match this

On linux and windows it did. Did you see the long annoying tests for the various defines and presence test of certain functions? The questions is what the mac needs...

windows branch Seeing these tests now. I will play around with it and try to come up with the right command for Mac.

Hmm, this is the starting point of another icu lib. May be we should build icu only manually on windows. Can you use some ifs and use find_package for icu lib on mac (and linux)...

windows branch Question: do we need this ICU for Mac as well? Or completely bypass it (no compilation , no find_package) ? I don't think I ever installed ICU on this Mac but it was - obviously - compiling fine on other branches. Is this a new internal feature on the windows branch that you're adding to all platforms?

Development should be after v0.0.2..., so it is introduced in the libevent branch? I can't believe this, I do not think I have touched the javascript. May be we should compare the files?

eventloop branch This is weird then, if you think this side of the server code hasn't changed ... I'll re-check but I'm pretty sure I rebuilt everything from scratch after switching.

Regarding event-config.h, did you have a recent python version installed, libevent uses python for these tests? And I remember cmake complained about it.

eventloop branch I'll pay more attention to CMake warnings etc. But I did not install a new python on this machine. It's the same setup since the very first reporting of the install/build issue.

For diagnosis of the problem, maybe put a console.log(args) to onStreamRead in webtransport.js. If this gives no insight, put a printout to every function in webtransport.js, that uses readable or readableController. These are the only paths, that can cause trouble. Or the changes in building result in trouble for node.js itself, by replacing some libs with our versions...., put the linker should detect this.

eventloop branch I can try that

[benbenz]

Maybe there has been a confusion between the eventloop and windows branch? I see and understand now that the windows branch also uses libevent. Do you want me to test mostly on this branch then? So we focus on the same CMakeList... ?

[benbenz]

Okay so: on the windows branch, compilation is working and the compilation of libevent is working as well (with event-config.h created). And you've already included the change for the ICU headers. So basically the windows branch compiles fine from the get-go now, with libevent included.

So we only have to resolve the ICU library runtime issue and this solves it all? Or is there anything specific to the eventloop branch that you want me to look at?

Then - hopefully - the server will run fine and I can look into the Reader runtime error...

[benbenz]

find_package/FindICU is not finding ICU on my machine, which is normal. I can either install ICU or resolve why this symbol is used in the first place. I've added all source files in icu4c/common directory with no luck :/

[martenrichter]

Ok, yes the windows branch is completely based on the libevent branch, so testing the libevent branch makes only sense, if something is broken on the windows branch and not on the libevent branch. If we get going the windows branch, it is everything we need.

The icu library is used by googleurl. However the stuff needed by googleurl is never called by the stuff we need. Therefore lazy linking saved us, from including it. But the windows clang linker insisted on resolving all symbols. If you know a switch to get the clang linker also to lazy link on windows, we can get of several files from googleurl and the icu. The unresolved symbol is actually the loading point of another icu lib, it would be also ok to define it somewhere to make the linker happy, since it is never called. And thanks for testing...

[benbenz]

I think the variable that's searched is U_DEF_ICUDATA_ENTRYPOINT whose name is pre-compiled line 172 of utypes.h And I think this is declared line 646 of udata.cpp (through a sequence of macros U UDEF2 into U_DEF_ICUDATA_ENTRY_POINT) Trying to comment this file ....

[martenrichter]

Yes I think that is the variable. On windows I use a special compilation flag, that excludes it, but it did not work on linux and caused other problems. But we may also just declare the variable somewhere else.... and fake it.

[martenrichter]

And udata.cc is needed by other files... at least on the windows build

[benbenz]

The server started when commenting out udata.cpp ! :) ... and QUIC is working :)) ... and we're back with the syntax reader issue. So I think this is good !

[martenrichter]

Perfect, but may be we can also just leave icu on mac out....

[benbenz]

Right. This was working just fine before :) I can try that - I remove it from quiche all together ? I don't see why this wouldn't be working

[martenrichter]

Yes you remove it, and if it works, it will only be conditional included in windows. The syntax reader issue, may be the catch is the problem, that it returns undefined if an error occurs which is handled by the catch? try using try catch instead, or do not use await.

[benbenz]

A quick incremental build seems to have done the job: the library is working without ICU. Running a full build now to confirm.

Will try the syntax change. If I don't use await, you mean switching to a promise then syntax ?

[martenrichter]

Yes, I think the problem is the mixing of the two types so either catch and then or await with a try catch block. But I could be wrong.

[martenrichter]

Or your catch can return {done: true}

[benbenz]

Oh right. That could totally be the reason. I did change the syntax few months ago but maybe didnt have errors there for some time. Will report once I printout info

[martenrichter]

Well actually 0.0.1 did not raise some error, when it should have risen the errors....

[benbenz]

I have some "[warn] kevent: Invalid argument" at startup as well. But QUIC is running fine.

Testing now the "syntax" error... Got caught up.

[martenrichter]

No idea, what the "kevent Invalid argument" means, anyway this must be a problem in quiche itself. Since it is fresh, we should wait if it goes away with other commits, or report it upstream....

[benbenz]

The connection behaves normally after that so no worries for now indeed. Im starting to think my test was inconsistent for the syntax error. It actually occurs when I switch between QUIC back to WS (aka disconnect the QUIC connections). So it could have very well occurred in the other version. Im about to test but I think you are right with the bad await / promise mix ... My bad. Sorry about that

I think the ping performance is better though, if it even makes sense?

[martenrichter]

I have no clue, I do not think another event loop should affect the ping performance. But it is a newer version of the quic library and may have some improvements in the mean time.

[benbenz]

Error is definitively at the app level and caused by the mix of syntax. You can disregard this one. And thanks for helping :) Basically the windows branch is compiling and working fine now :) Just this kevent warning but this doesn't impact functionality at the moment.

[martenrichter]

Ok, thanks, I have just commit something to include icu only on windows. That was all we needed....? I will then check later if it still compiles on windows and linux. Then fix the windows build..... (not working yet)

[benbenz]

I think so: on the windows branch, the ICU was the only issue. Good luck with windows !!!!

[martenrichter]

So Windows is almost done and now on the development branch, However I had to change some szuff in the client and server, it works well on windows and linux, but on MacOS Well I have setup a MacOS CI bot to check it, but it stalls... and uses a release branch. Can you please tell me if it is broken for npm test on your debug build as well? Any clues were it breaks? I am sorry but it is quite difficult to make all platforms happy....

[martenrichter]

I found the issue leading to the failure of the mac builds...

[benbenz]

Hello Marten. Sorry I was not on my computer. I'm available if you need me to check stuff on OSX.

[martenrichter]

No, thanks, finally the unit test on macOS passed. I think it should work now.....

[benbenz]

okay. great to hear. I ran a rebuild-debug without issue :) I have never used those CI builds. This is interesting. Is GitHub providing this service? You just have to write the build.js file ?

[martenrichter]

Well github provides the github actions, you have just to write a yml, which the stuff you want to do. In this case I check on all three platforms, if it builds and if the test are working.