FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

build: make `partly_static` opt-out by refack · Pull Request #26726 · nodejs/node · GitHub

/ node Public

build: make partly_static opt-out - #26726

Closed
refack wants to merge 2 commits into
nodejs:masterfrom
refack:change-partialy-static-to-opt-out
ConversationCommits (2)ChecksFiles changed
Closed

build: make partly_static opt-out#26726
refack wants to merge 2 commits into
nodejs:masterfrom
refack:change-partialy-static-to-opt-out

Conversation

refack commented Mar 18, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • 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 commented Mar 18, 2019

Copy link
Copy Markdown
Collaborator

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2954/pipeline

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Mar 18, 2019
refack self-assigned this Mar 18, 2019
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 added this to the 12.0.0 milestone Mar 18, 2019

refack commented Mar 18, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

/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 requested a review from bnoordhuis March 18, 2019 02:20

refack commented Mar 18, 2019

Copy link
Copy Markdown
Contributor Author

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

richardlau commented Mar 18, 2019

Copy link
Copy Markdown
Member

Also nodejs/help#951

rvagg commented Mar 18, 2019

Copy link
Copy Markdown
Member

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

rvagg mentioned this pull request Mar 18, 2019
Closed

refack commented Mar 18, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 commented Mar 18, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

bnoordhuis reviewed Mar 19, 2019
View reviewed changes

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread configure.py Outdated

bnoordhuis Mar 19, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should be default=True,, no quotes.

rvagg commented Mar 19, 2019

Copy link
Copy Markdown
Member

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

refack added 2 commits March 19, 2019 09:30
build: make partly_static opt-out
e777615 
* remove the dependancy of the binary on installed compilers
fixup!
0b6fd15
refack force-pushed the change-partialy-static-to-opt-out branch from 425df0b to 0b6fd15 Compare March 19, 2019 13:43

mhdawson commented Mar 19, 2019

Copy link
Copy Markdown
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 commented Mar 23, 2019

Copy link
Copy Markdown
Contributor Author

Fixes: #26877

refack commented Mar 24, 2019

Copy link
Copy Markdown
Contributor Author

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'

Trott removed this from the 12.0.0 milestone Apr 29, 2019
BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
jasnell added the stalled Issues and PRs that are stalled. label Jun 19, 2020

jasnell commented Jun 19, 2020

Copy link
Copy Markdown
Member

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 closed this Jun 19, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis left review comments

Assignees

refack

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Running node.js 10 on CentOS7 ARM on RaspberryPi

8 participants

Back | FazBrowse Home | New Git URL