build: link libatomic on mac and linux #28232

@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.

addaleax · Jun 17, 2019

@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…

devsnek · Jun 17, 2019

I just pulled and ran make on a wsl debian install. i didn't configure it weirdly or anything.

sam-github · Jun 17, 2019

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

devsnek · Jun 17, 2019

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

devsnek · Jun 18, 2019

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)

devsnek · Jun 18, 2019

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?

sam-github · Jun 18, 2019

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?

nodejs-github-bot · Jun 18, 2019

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

richardlau · Jun 18, 2019

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, 
        
           }],

)

sam-github · Jun 18, 2019

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

devsnek · Jun 18, 2019

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

richardlau · Jun 18, 2019

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.

devsnek · Jun 18, 2019

@richardlau i've needed it on my mac at least.


      build: link libatomic on mac and linux

Fixes #28231

nodejs-github-bot · Jun 18, 2019

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

devsnek · Jun 18, 2019

checking llvm_version seems to have worked!

nodejs-github-bot · Jun 18, 2019

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

sam-github · 2019-06-19T17:50:16Z

sam-github approved these changes

Jun 19, 2019

View changes

sam-github · Jun 19, 2019

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

nodejs-github-bot · Jun 19, 2019

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

devsnek · Jun 19, 2019

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

i am assuming the failure is unrelated to this change though.

devsnek · Jun 21, 2019

this needs approvals for the changed semantics

sam-github · 2019-06-21T20:30:52Z

sam-github approved these changes

Jun 21, 2019

View changes

jasnell · 2019-06-21T20:38:57Z

jasnell approved these changes

Jun 21, 2019

View changes


      build: link libatomic on mac and linux

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>

devsnek · Jun 21, 2019

Landed in ab3174c


      build: link libatomic on mac and linux

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>

devsnek added the build label Jun 14, 2019

devsnek requested a review from addaleax Jun 14, 2019

joyeecheung referenced this pull request Jun 16, 2019

CI failures: 20190612-20190615 #23

Open

build: link libatomic on mac and linux
Fixes #28231

Verified

This commit was signed with a verified signature.

devsnek Gus Caplan

GPG key ID: F00BD11880E82F0E Learn about signing commits

Loading status checks…

5fc6e13

devsnek force-pushed the fix-atomic-linkage branch from 8a6960b to 5fc6e13 Jun 18, 2019

devsnek requested review from addaleax, richardlau and sam-github Jun 18, 2019

sam-github referenced this pull request Jun 19, 2019

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

Closed

sam-github added gyp linux macos labels Jun 21, 2019

devsnek added a commit to devsnek/node that referenced this pull request Jun 21, 2019

devsnek closed this Jun 21, 2019

devsnek deleted the fix-atomic-linkage branch Jun 21, 2019

targos added a commit that referenced this pull request Jul 2, 2019

targos referenced 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

Open

nodejs/node

Join GitHub today

build: link libatomic on mac and linux #28232

Conversation

devsnek commented Jun 14, 2019 • edited

Checklist

devsnek added the build label Jun 14, 2019

devsnek requested a review from addaleax Jun 14, 2019

This comment has been minimized.

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

This comment has been minimized.

devsnek commented Jun 16, 2019

This comment has been minimized.

rvagg commented Jun 17, 2019

This comment has been minimized.

addaleax commented Jun 17, 2019

This comment has been minimized.

devsnek commented Jun 17, 2019

This comment has been minimized.

sam-github commented Jun 17, 2019

This comment has been minimized.

devsnek commented Jun 17, 2019

This comment has been minimized.

devsnek commented Jun 18, 2019

This comment has been minimized.

devsnek commented Jun 18, 2019 • edited

This comment has been minimized.

sam-github commented Jun 18, 2019

This comment has been minimized.

nodejs-github-bot commented Jun 18, 2019

This comment has been minimized.

richardlau commented Jun 18, 2019

This comment has been minimized.

sam-github commented Jun 18, 2019

This comment has been minimized.

devsnek commented Jun 18, 2019

This comment has been minimized.

richardlau commented Jun 18, 2019

This comment has been minimized.

devsnek commented Jun 18, 2019

devsnek force-pushed the fix-atomic-linkage branch from 8a6960b to 5fc6e13 Jun 18, 2019

This comment has been minimized.

nodejs-github-bot commented Jun 18, 2019

This comment has been minimized.

devsnek commented Jun 18, 2019

devsnek requested review from addaleax, richardlau and sam-github Jun 18, 2019

This comment has been minimized.

nodejs-github-bot commented Jun 18, 2019

This comment has been minimized.

sam-github commented Jun 19, 2019

This comment has been minimized.

nodejs-github-bot commented Jun 19, 2019

This comment has been minimized.

devsnek commented Jun 19, 2019 • edited

This comment has been minimized.

devsnek commented Jun 21, 2019

sam-github added gyp linux macos labels Jun 21, 2019

This comment has been minimized.

devsnek commented Jun 21, 2019

devsnek closed this Jun 21, 2019

devsnek deleted the fix-atomic-linkage branch Jun 21, 2019

devsnek commented Jun 14, 2019 •

edited

nodejs-github-bot commented Jun 14, 2019 •

edited

devsnek commented Jun 18, 2019 •

edited

devsnek force-pushed the fix-atomic-linkage branch from `8a6960b` to `5fc6e13` Jun 18, 2019

devsnek commented Jun 19, 2019 •

edited