build: link libatomic on mac and linux by devsnek · Pull Request #28232 · nodejs/node · GitHub

cc @nodejs/build

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Sadly, an error occurred when I tried to trigger a build. :(
CI: https://ci.nodejs.org/job/node-test-pull-request/23892/

@nodejs/build macos failed with no library found for -latomic.

@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 make on a wsl debian install. i didn't configure it weirdly or anything.

@devsnek can you provide more platform information, like gcc (I assume its gcc) version, and uname output?

Linux xone 4.19.43-microsoft-standard #1 SMP Mon May 20 19:35:22 UTC 2019 x86_64 GNU/Linux and clang version 9.0.0

also a problem on a native (not wsl) linux i just set up (Linux xone 4.19.49-1-MANJARO #1 SMP PREEMPT Sun Jun 9 20:24:20 UTC 2019 x86_64 GNU/Linux, clang-8.0.0)

I'm going to guess the issue is Clang does not currently automatically link against libatomic when using libgcc_s. You may need to manually add -latomic to support this configuration when using non-native atomic operations (if you see link errors referring to __atomic_* functions). (https://clang.llvm.org/docs/Toolchain.html)

Perhaps we should get a linux clang ci machine?

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 -latomic to all builds).

Another question, for .gyp experts, is there a way to add linker flags conditionally based on the toolchain?

CI: https://ci.nodejs.org/job/node-test-pull-request/23961/

Another question, for .gyp experts, is there a way to add linker flags conditionally based on the toolchain?

Yes. We define llvm_version:

node/configure.py

Line 797 in 779a243

o['variables']['llvm_version'] = get_llvm_version(CC) if is_clang else 0

e.g. used to set cflags (but could be libraries as in this PR):

node/deps/openssl/openssl_common.gypi

Lines 66 to 69 in 128170f

    
           'conditions': [ 
        
             [ 'llvm_version==0', { 
        
               'cflags': ['-Wno-old-style-declaration',], 
        
             }],

There's also a clang variable:

node/common.gypi

Line 28 in d2634be

'clang%': 0,

(set in

node/common.gypi

Lines 112 to 114 in d2634be

    
           ['OS=="mac"', { 
        
             'clang%': 1, 
        
           }],

)

I don't have the background to approve or reject, but seems like its failing to build on at least OS X:

osx/nodes/osx1011/out/Release/libv8_compiler.a /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/libv8_initializers.a -latomic -framework CoreFoundation -lm
09:55:55 ld: library not found for -latomic

https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx1011/27519/console

seems like this should be (OS == "linux" or OS == "mac") and llvm_version != 0?

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.

Fixes #28231

CI: https://ci.nodejs.org/job/node-test-pull-request/23964/

checking llvm_version seems to have worked!

CI: https://ci.nodejs.org/job/node-test-pull-request/23965/

CI didn't pass, probably needs another kick (but I'm on poor wifi ATM, can't do).

CI: https://ci.nodejs.org/job/node-test-pull-request/23978/

it's always parallel/test-worker-debug cc @joyeecheung

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

Fixes #28231 PR-URL: #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>

do we officially support clang?

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.

devsnek added the build Issues and PRs related to build files or the CI. label Jun 14, 2019

devsnek requested a review from addaleax June 14, 2019 21:04

addaleax approved these changes Jun 14, 2019

View reviewed changes

richardlau approved these changes Jun 14, 2019

View reviewed changes

sam-github reviewed Jun 14, 2019

View reviewed changes

Comment thread node.gyp

joyeecheung mentioned this pull request Jun 16, 2019

CI failures: 20190612-20190615 nodejs/reliability#23

Closed

build: link libatomic on mac and linux

5fc6e13

Fixes #28231

devsnek force-pushed the fix-atomic-linkage branch from 8a6960b to 5fc6e13 Compare June 18, 2019 18:41

devsnek requested review from addaleax, richardlau and sam-github June 18, 2019 19:33

sam-github approved these changes Jun 19, 2019

View reviewed changes

sam-github mentioned this pull request Jun 19, 2019

Investigate flake test-worker-debug on ubuntu shared libs #28299

Closed

sam-github approved these changes Jun 21, 2019

View reviewed changes

sam-github added gyp Issues and PRs related to the GYP tool and .gyp build files linux Issues and PRs related to the Linux platform. macos Issues and PRs related to the macOS platform / OSX. labels Jun 21, 2019

jasnell approved these changes Jun 21, 2019

View reviewed changes

devsnek closed this Jun 21, 2019

devsnek deleted the fix-atomic-linkage branch June 21, 2019 20:56

targos mentioned this pull request Jul 2, 2019

Release v12.6.0 proposal #28508

Merged

This was referenced Jul 3, 2019

2019-07-03 Version 12.6.0 (Current) @targos beevelop/docker-nodejs#288

Closed

2019-07-03 Version 12.6.0 (Current) @targos beevelop/docker-nodejs#289

Closed

2019-07-03 Version 12.6.0 (Current) @targos beevelop/docker-nodejs#290

Closed

This was referenced Jul 4, 2019

nodejs-12_x: 12.5.0 -> 12.6.0 NixOS/nixpkgs#64253

Merged

build: link libatomic on gcc or clang on linux #28532

Closed

ryandesign mentioned this pull request Oct 23, 2019

Wrong condition for deciding when to add -latomic #30093

Closed

hsdk123 mentioned this pull request Jul 10, 2024

[bug] atomic doesn't exist on mac, ios but is added kcat/openal-soft#1019

Closed

build: link libatomic on mac and linux by devsnek · Pull Request #28232 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: link libatomic on mac and linux - #28232

build: link libatomic on mac and linux#28232devsnek wants to merge 1 commit intomasternodejs/node:masterfrom fix-atomic-linkagenodejs/node:fix-atomic-linkageCopy head branch name to clipboard

Conversation

devsnek commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

devsnek commented Jun 16, 2019

Uh oh!

rvagg commented Jun 17, 2019

Uh oh!

addaleax commented Jun 17, 2019

Uh oh!

devsnek commented Jun 17, 2019

Uh oh!

sam-github commented Jun 17, 2019

Uh oh!

devsnek commented Jun 17, 2019

Uh oh!

devsnek commented Jun 18, 2019

Uh oh!

devsnek commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sam-github commented Jun 18, 2019

Uh oh!

nodejs-github-bot commented Jun 18, 2019

Uh oh!

richardlau commented Jun 18, 2019

Uh oh!

sam-github commented Jun 18, 2019

Uh oh!

devsnek commented Jun 18, 2019

Uh oh!

richardlau commented Jun 18, 2019

Uh oh!

devsnek commented Jun 18, 2019

Uh oh!

nodejs-github-bot commented Jun 18, 2019

Uh oh!

devsnek commented Jun 18, 2019

Uh oh!

nodejs-github-bot commented Jun 18, 2019

Uh oh!

sam-github commented Jun 19, 2019

Uh oh!

nodejs-github-bot commented Jun 19, 2019

Uh oh!

devsnek commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devsnek commented Jun 21, 2019

Uh oh!

devsnek commented Jun 21, 2019

Uh oh!

ryandesign commented Oct 23, 2019

Uh oh!

sam-github commented Oct 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

build: link libatomic on mac and linux#28232
devsnek wants to merge 1 commit into
masterfrom
fix-atomic-linkage

devsnek commented Jun 14, 2019 •
edited
Loading

nodejs-github-bot commented Jun 14, 2019 •
edited
Loading

devsnek commented Jun 18, 2019 •
edited
Loading

devsnek commented Jun 19, 2019 •
edited
Loading