-
Notifications
You must be signed in to change notification settings - Fork 119
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
Fix the thread code inclusion problem in Emscripten builds #1375
Conversation
Instead, use the predefined macro __EMSCRIPTEN__ in the source code.
It now fails on this: Which version is getting used in CI? |
I'll wait a bit before merging this... |
Strangely, |
Well, the other issue #1361 is like this too: works n my machine, fails in circle-ci. ... and more: when I first spotted it, it seemed there were two different versions of emscripten in circle ci, one passed, one failed... Hm. Maye need to look at the ci config file more carefully? -- This config file: |
I looked at Try adding |
From the CI log:
My emcc.py, as I got from |
link-grammar/bindings/js/install_emsdk.sh Lines 1 to 13 in 43d2392
For some reason it installs emsdk version 1.38.41 , which is one of the firs versions. The latest version, as installed by emsdk install latest , is 3.1.30 . I don't know why a fixed version got used in the first place.
With
And indeed |
I merged this pull req because I actually looked to see what it did. And now that my eyes uncrossed, it was obviously "the right thing to do" |
The js bindings are 10-15 years old :-/ |
I guess you tried this:
|
Yes. |
|
I get this error too. I try to trace how it is created, in order to find out how to create a relocatable result (in a hope it will work). |
https://groups.google.com/g/emscripten-discuss/c/cwg_EdFYOr8/m/xLpVqi9VAQAJ?pli=1 |
|
That seems to build. I have no idea if the result is usable or not. However, look at the debug output here: https://github.com/linas/link-grammar/actions/runs/3952974168/jobs/6768705348 -- this is from trying to uncomment the line
and its .. well, like the threading wasn't suppressed after all. |
Except the issue is not about threading, its about duplicate symbols. So I guess the library should not have been linked. Whatever. I'm going to comment out that line, and just punt. No one is using link-grammar js anyway, and given how things are, it seems unlikely that anyone ever will ... |
It seems that by now Emscripten does support Pthreads: From https://emscripten.org/docs/api_reference/wasm_workers.html:
|
Here is how I ran it using Chrome:
When I open the JS console I see:
I also tried to run it using `node'.
4.B.
So |
These instructions belong in a README |
It turned out the TLS variable got defined to Also, it is possible to replace the I still haven't found how the following call in Unless I miss something, there is not supposed to be Also note that the emsdk's bundled |
In the normal, non-emcc world, the
For the emcc case, I assume
For me, So I assume if you install nodejs, then the emsdk-bundled version will be ignored. |
link-grammar/bindings/js/link-parser/build.sh Lines 1 to 36 in 750315d
Line 7 sets the path the emsdk 's bundled nodejs. This is the output of emsdk_env.sh :
Note the 3rd component in PATH: Regarding Regarding the test in link-grammar/bindings/js/link-parser/build.sh Lines 22 to 34 in 750315d
It invokes link-parser , but which searches it in the PATH , and there is normally no . in it. So it uses an installed one (if any). In addition, line 32 performs popd , so link-parser doesn't run inside the dist directory. So how it is supposed toi find the data directory?In addition, I don't understand the use of npm link . It tries to link to a system directory, which is not supposed to already contain link-grammar (and in any case fails for me because I don't run it as root). I think it should be tested without npm.
|
I said:
I am now reading the npm docs in order to understand how all of that is supposed to work. |
Should I merge #1377? Or should I wait? |
Better to wait since I'm touching the same code. |
See issue #1374 .
Remove the Emscripten check from
configure
and instead check__EMSCRIPTEN__
in the source code.(For some reason this didn't work for me when I previously handled this problem.)
BTW,
AX_TLS
already takes care not to define
TLS`.In addition, git-ignore the file
a.wasm
that is left afteremconfigure ./configure
.Note: This error occurs inside an autoconf macro. I didn't try to find out why.