Skip to content
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

node@6 6.15.1 #34674

Closed
wants to merge 1 commit into from
Closed

node@6 6.15.1 #34674

wants to merge 1 commit into from

Conversation

igas
Copy link
Contributor

@igas igas commented Dec 1, 2018

Created with brew bump-formula-pr.

@fxcoudert
Copy link
Member

Test is failing:

gyp info spawn args [ 'V=1', 'BUILDTYPE=Release', '-C', 'build' ]
  /usr/bin/clang++ '-DNODE_GYP_MODULE_NAME=bignum' '-DUSING_UV_SHARED=1' '-DUSING_V8_SHARED=1' '-DV8_DEPRECATION_WARNINGS=1' '-D_DARWIN_USE_64_BIT_INODE=1' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-DBUILDING_NODE_EXTENSION' -I/private/tmp/node@6-test-20181201-98454-ewnpz8/.node-gyp/6.15.0/include/node -I/private/tmp/node@6-test-20181201-98454-ewnpz8/.node-gyp/6.15.0/src -I/private/tmp/node@6-test-20181201-98454-ewnpz8/.node-gyp/6.15.0/deps/uv/include -I/private/tmp/node@6-test-20181201-98454-ewnpz8/.node-gyp/6.15.0/deps/v8/include -I../../nan -I/private/tmp/node@6-test-20181201-98454-ewnpz8/.node-gyp/6.15.0/deps/openssl/openssl/include -I/private/tmp/node@6-test-20181201-98454-ewnpz8/.node-gyp/6.15.0/deps/openssl/config/k8  -Os -gdwarf-2 -mmacosx-version-min=10.7 -arch x86_64 -Wall -Wendif-labels -W -Wno-unused-parameter -std=gnu++0x -fno-rtti -fno-exceptions -fno-threadsafe-statics -fno-strict-aliasing -MMD -MF ./Release/.deps/Release/obj.target/bignum/bignum.o.d.raw -F/usr/local/Frameworks -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -Os -w -pipe -march=native -mmacosx-version-min=10.14 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -c -o Release/obj.target/bignum/bignum.o ../bignum.cc
  /usr/bin/clang++ -bundle -undefined dynamic_lookup -Wl,-no_pie -Wl,-search_paths_first -mmacosx-version-min=10.7 -arch x86_64 -L./Release -L/usr/local/lib -F/usr/local/Frameworks -Wl,-headerpad_max_install_names -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -o Release/bignum.node Release/obj.target/bignum/bignum.o 
clang: warning: libstdc++ is deprecated; move to libc++ with a minimum deployment target of OS X 10.9 [-Wdeprecated]
ld: library not found for -lstdc++
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [Release/bignum.node] Error 1
gyp ERR! build error 
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/usr/local/Cellar/node@6/6.15.0/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:276:23)
gyp ERR! stack     at emitTwo (events.js:106:13)
gyp ERR! stack     at ChildProcess.emit (events.js:191:7)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:219:12)
gyp ERR! System Darwin 18.2.0
gyp ERR! command "/usr/local/Cellar/node@6/6.15.0/bin/node" "/usr/local/Cellar/node@6/6.15.0/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /private/tmp/node@6-test-20181201-98454-ewnpz8/node_modules/bignum
gyp ERR! node -v v6.15.0
gyp ERR! node-gyp -v v3.4.0
gyp ERR! not ok 

@chrmoritz
Copy link
Contributor

node@6 (unlike all newer node releases) is still configured to use the deprecated and since XCode 10.1 removed libstdc++ instead of lstdc++. I guess superenv takes care that lstdc++ is used during the build of node@6 (preventing a build failure there), but node-gyp still tries to use the now removed libstdc++ when building native addons.

Refs: nodejs/node#24648

We would need to apply the common.gypi patch from there to continue to support compiling native addons with node@6 with XCode 10.1+. This would also fix the long time unnoticed standard library incompatibility between our node@6 formula and native addons compiled with it.

@fxcoudert
Copy link
Member

Or we simply drop node@6. Versioned formulas are only kept if they build an all supported macOS versions.

@chrmoritz
Copy link
Contributor

chrmoritz commented Dec 1, 2018

Yeah, that would be an option too. But strictly speaking the formula is building fine as it is. It's only that we missed to set the correct config to follow superenv switching the build from libstdc++ over to libc++.

I wonder that nobody has reported any incompatibilities between our libc++ node@6 and libstdc++ native addons (prebuild or compiled with our node-gyp) yet. Likely we were lucky and nothing really was broken because of it.

@igas igas changed the title node@6 6.15.0 node@6 6.15.1 Dec 3, 2018
@SMillerDev SMillerDev added legacy Relates to a versioned @ formula build failure CI fails while building the software labels Dec 7, 2018
@chrmoritz
Copy link
Contributor

chrmoritz commented Dec 8, 2018

@igas: Adding this to the beginning of the install method should fix the test issue with native addons: see #34674 (comment)

Also the label should be test failure and not build failure.

All what we are actual doing here is fixing a long outstanding issue with the ABI incompatibility between our node@6 binary and native addons compiled with our node-gyp. Strictly speaking we were never API compatible with upstream node and should have used a different NODE_MODULE_VERSION because of it. But in practice using a different c++ standard library is way less problematic than using a different major version of openssl (which some linux distributions are doing which is actual causing problems with prebuilt native addons in practice).

@igas igas added test failure CI fails while running the test-do block and removed build failure CI fails while building the software labels Dec 8, 2018
@chrmoritz
Copy link
Contributor

chrmoritz commented Dec 10, 2018

Sorry for the typo, there are actual 2 spaces before the comment in the common.gyp inreplace (and not only 1):

def install
    # Switches standard libary for native addons from libstdc++ to libc++ to
    # match the superenv enforced one for the node binary itself. This fixes
    # incompatibilities between native addons built with our node-gyp and our
    # node binary and makes building native addons with XCode 10.1+ possible.
    inreplace "common.gypi", "'CLANG_CXX_LANGUAGE_STANDARD': 'gnu++0x',  # -std=gnu++0x",
      "'CLANG_CXX_LANGUAGE_STANDARD': 'gnu++0x',  # -std=gnu++0x\n'CLANG_CXX_LIBRARY': 'libc++',"

@chrmoritz chrmoritz mentioned this pull request Dec 14, 2018
@igas igas added the help wanted Task(s) needing PRs from the community or maintainers label Dec 17, 2018
@chrmoritz
Copy link
Contributor

Strangely enough I can't reproduce the build failure with the inreplace CLANG_CXX_LIBRARY': 'libc++' locally: https://gist.github.com/chrmoritz/4a7157597a59cb4106afcc94f42442ba

@chrmoritz chrmoritz mentioned this pull request Dec 26, 2018
@igas
Copy link
Contributor Author

igas commented Dec 27, 2018

Hi, @chrmoritz I'm closing it, please feel free to submit updated one. As I'm not very involved and/or experienced with node, it looks like you are more interested in pushing it over the line. Thanks.

@igas igas closed this Dec 27, 2018
@igas igas deleted the node@6-6.15.0 branch December 27, 2018 04:49
@igas igas added the not merged PR was closed without being merged (and may need to be revisited) label Dec 27, 2018
@chrmoritz chrmoritz mentioned this pull request Dec 27, 2018
5 tasks
@lock lock bot added the outdated PR was locked due to age label Jan 26, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Task(s) needing PRs from the community or maintainers legacy Relates to a versioned @ formula not merged PR was closed without being merged (and may need to be revisited) outdated PR was locked due to age test failure CI fails while running the test-do block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants