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

build: make partly_static opt-out #26726

Closed

Conversation

refack
Copy link
Contributor

@refack refack commented Mar 18, 2019

  • remove the dependency of the binary on installed compilers

Fixes: nodejs/build#1724
Refs: nodejs/build#1543

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Mar 18, 2019
@refack refack self-assigned this Mar 18, 2019
@refack refack added c++ Issues and PRs that require attention from people who are familiar with C++. semver-major PRs that contain breaking changes and should be released in the next major version. meta Issues and PRs related to the general management of the project. node-api Issues and PRs related to the Node-API. embedding Issues and PRs related to embedding Node.js in another project. shared_lib Issues and PRs related to use of Node.js as a shared library. investigating labels Mar 18, 2019
@refack refack added this to the 12.0.0 milestone Mar 18, 2019
@refack
Copy link
Contributor Author

refack commented Mar 18, 2019

/CC @nodejs/build @nodejs/build-files

Posted to reevaluate our approach to static linking to the compiler libraries:

  • CLang already does something similar (with libc++)
  • This should alleviate our woes regarding binary cross-compatibility with systems that do not include the exact version of our compiler.

But:

  • It does make the binary bigger
  • Native module interop needs to be validated.

@refack refack requested a review from bnoordhuis March 18, 2019 02:20
@refack
Copy link
Contributor Author

refack commented Mar 18, 2019

Last discussion I found that's around this - #4152
Some refs: https://stackoverflow.com/questions/13636513/linking-libstdc-statically-any-gotchas

@richardlau
Copy link
Member

Also nodejs/help#951

@rvagg
Copy link
Member

rvagg commented Mar 18, 2019

can you show us a binary file size difference with this on for master?

@refack
Copy link
Contributor Author

refack commented Mar 18, 2019

can you show us a binary file size difference with this on for master?

I run this on linuxONE (where BTW our vanilla binary throws /lib64/libstdc++.so.6: version 'GLIBCXX_3.4.20' not found (required by ./node))
pre (45'841'792):

[iojs@test-linuxonecc-rhel72-s390x-1 rhel72-s390x]$ ldd out/Release/node
out/Release/node: /lib64/libstdc++.so.6: version `GLIBCXX_3.4.20' not found (required by out/Release/node)
        libdl.so.2 => /lib64/libdl.so.2 (0x000003ffb9d80000)
        librt.so.1 => /lib64/librt.so.1 (0x000003ffb9d00000)
        libstdc++.so.6 => /lib64/libstdc++.so.6 (0x000003ffb9b80000)
        libm.so.6 => /lib64/libm.so.6 (0x000003ffb9a80000)
        libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x000003ffb9a00000)
        libpthread.so.0 => /lib64/libpthread.so.0 (0x000003ffb9980000)
        libc.so.6 => /lib64/libc.so.6 (0x000003ffb9780000)
        /lib/ld64.so.1 (0x000003ffb9e80000)
[iojs@test-linuxonecc-rhel72-s390x-1 rhel72-s390x]$ ls -la out/Release/node
-rwxr-xr-x 1 iojs iojs 45841792 Mar 18 08:33 out/Release/node

post (52'055'208)

[iojs@test-linuxonecc-rhel72-s390x-1 rhel72-s390x]$ ldd out/Release/node
        libdl.so.2 => /lib64/libdl.so.2 (0x000003ff85a00000)
        librt.so.1 => /lib64/librt.so.1 (0x000003ff85980000)
        libm.so.6 => /lib64/libm.so.6 (0x000003ff85880000)
        libpthread.so.0 => /lib64/libpthread.so.0 (0x000003ff85800000)
        libc.so.6 => /lib64/libc.so.6 (0x000003ff85600000)
        /lib/ld64.so.1 (0x000003ff85b00000)
[iojs@test-linuxonecc-rhel72-s390x-1 rhel72-s390x]$ ls -la out/Release/node
-rwxrwxr-x 1 iojs iojs 52055208 Mar 18 09:18 out/Release/node

@refack
Copy link
Contributor Author

refack commented Mar 18, 2019

Another datapoint, Ubuntu Xenial (only 1MB or ~2.5%):
pre (42'883'024)

[iojs@ubuntu1604]$ ldd out/Release/node
        linux-vdso.so.1 =>  (0x00007ffcd99b7000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007ff3eb8d5000)
        librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007ff3eb6cd000)
        libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007ff3eb34b000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007ff3eb042000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007ff3eae2c000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007ff3eac0f000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007ff3ea845000)
        /lib64/ld-linux-x86-64.so.2 (0x00007ff3ebad9000)
[iojs@ubuntu1604]$ ls -la out/Release/node
-rwxrwxr-x 1 iojs iojs 42883024 Mar 18 13:41 out/Release/node

post (43'889'160)

[iojs@ubuntu1604-x64]$ ldd out/Release/node
        linux-vdso.so.1 =>  (0x00007ffe503fd000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f7ebade4000)
        librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f7ebabdc000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f7eba8d3000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f7eba6b6000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f7eba2ec000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f7ebafe8000)
[iojs@ubuntu1604]$ ls -la out/Release/node
-rwxrwxr-x 1 iojs iojs 43889160 Mar 18 13:48 out/Release/node

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal is to enable partially static builds by default? -static-libstdc++ is unlikely to be what we want for release builds. It's also not a good default for distro packagers. It might wreak havoc with add-ons that are linked to a different stdlib.

--not-partly-static is kind of an awkward name, other opt-out options start with --no-<name>.

configure.py Outdated
dest="partly_static",
default='True',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be default=True,, no quotes.

@rvagg
Copy link
Member

rvagg commented Mar 19, 2019

@bnoordhuis "-static-libstdc++ is unlikely to be what we want for release builds" - we're trying to find a way upward on GCC version for Node 12 while supporting currently supported distros as much as possible (Node 12 supported platforms planning - #26714 - your input would be great). On x64 it's easy because devtoolset gives us their backward compat magic, but on anything else (ARM, IBM, maybe SmartOS) we're stuck with lowest-common-denominator and knocking out some OS versions (Ubuntu 16.04 on ARM would be out if we go to GCC-6, for example).

* remove the dependancy of the binary on installed compilers
@refack refack force-pushed the change-partialy-static-to-opt-out branch from 425df0b to 0b6fd15 Compare March 19, 2019 13:43
@mhdawson
Copy link
Member

I am worried about the impact to addon binaries as well, particularly if it means that it breaks the N-API stability promise.

I think we need to very carefully understand all of the implications of this kind of change. Ideally, we'd make it just after cutting a release so we'd have more time in master to see what/if issues arise.

@refack
Copy link
Contributor Author

refack commented Mar 23, 2019

Fixes: #26877

@refack
Copy link
Contributor Author

refack commented Mar 24, 2019

Some info about how RH's dev-toolset solves this issue. They create a delta library libstdc++_nonshared.a which is statically linked to binaries if they require any of it's symbols.
This library has ~30% of the symbols that libstdc++.so.6.26 holds:

$ objdump -x /opt/rh/devtoolset-8/root/usr/lib/gcc/x86_64-redhat-linux/8/libstdc++.a | wc -l
99322
$ objdump -x /opt/rh/devtoolset-8/root/usr/lib/gcc/x86_64-redhat-linux/8/libstdc++_nonshared.a | wc -l
36897

There is no consensus on the "internet" about the optimal solution to this problem. From my survey it seems like these are the common solutions, ranked in order of empirical "safety":

  1. requiring users to update libstdc++
  2. using libstdc++_nonshared.a
  3. static linking, as in this PR
  4. packing libstdc++.so.6.21 with our binary and linking with -Wl,-rpath,'$ORIGIN'

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jun 19, 2020
@jasnell
Copy link
Member

jasnell commented Jun 19, 2020

There's been no activity on this in over a year. It's not clear if it should move forward or not. Closing it as stalled but we can reopen if necessary. @nodejs/build

@jasnell jasnell closed this Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. embedding Issues and PRs related to embedding Node.js in another project. meta Issues and PRs related to the general management of the project. node-api Issues and PRs related to the Node-API. semver-major PRs that contain breaking changes and should be released in the next major version. shared_lib Issues and PRs related to use of Node.js as a shared library. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running node.js 10 on CentOS7 ARM on RaspberryPi
8 participants