build: link libatomic on mac and linux #28232
Conversation
|
Sadly, an error occurred when I tried to trigger a build. :( |
|
@nodejs/build macos failed with no library found for |
|
@devsnek do you have any suggestions for actions for Build here? Sounds like a blocker if this won't pass on one the platforms you're intending to impact. |
|
@devsnek Are you building Node.js in a non-standard way (shared library or similar)? If this is already a dep for V8, I’m even more surprised that you’ve run into trouble… |
|
I just pulled and ran |
|
@devsnek can you provide more platform information, like gcc (I assume its gcc) version, and uname output? |
|
|
|
also a problem on a native (not wsl) linux i just set up ( |
|
I'm going to guess the issue is Perhaps we should get a linux clang ci machine? |
Unless it comes with a volunteer to maintain it, I'm not sure that extending our build infrastructure is a good idea. @nodejs/build Perhaps -latomic should be added to build platforms for all non-Windows platforms? I haven't done an investigation, but I believe this would make libatomic a build dep, even for toolchains that don't need it, which is a downside, but it could possibly make builds more consistent as an upside. One question to be answered would be if it was a build dep, would it become a runtime dep for binaries from toolchains that do not currently need libatomic? That would be backwards incompatible, so I assume a non-starter for 12.x, and maybe not a good idea for any version. Also, its not clear to me - do we officially support clang? I'm sure we would make changes to allow people who want to use clang to be able, but I'm not sure its officially supported, to the point that someone from build would go through all our machines and update their ansibles, etc., to have libatomic (if that was required after adding Another question, for .gyp experts, is there a way to add linker flags conditionally based on the toolchain? |
Yes. We define Line 797 in 779a243 e.g. used to set cflags (but could be node/deps/openssl/openssl_common.gypi Lines 66 to 69 in 128170f There's also a Line 28 in d2634be Lines 112 to 114 in d2634be |
|
I don't have the background to approve or reject, but seems like its failing to build on at least OS X: |
|
seems like this should be |
|
I guess the question is if this is needed on mac as the OS X builds have not (AFAIK) been failing and do use clang by default. |
|
@richardlau i've needed it on my mac at least. |
8a6960b to
5fc6e13
Compare
|
checking |
|
CI didn't pass, probably needs another kick (but I'm on poor wifi ATM, can't do). |
|
it's always i am assuming the failure is unrelated to this change though. |
|
this needs approvals for the changed semantics |
Fixes nodejs#28231 PR-URL: nodejs#28232 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in ab3174c |
If you don't support clang, you effectively don't support macOS. Please support clang and macOS. |
|
When you quoted me, you removed context, that was in response to a question about clang on linux. |
Fixes #28231
cc @nodejs/build
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes