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,deps: enable building V8 as shared library #23202

Closed
wants to merge 2 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 1, 2018

Motivation: Makes linking the node binary faster, which is especially significant when doing a debug build.

The incantation is ./configure ... -- -Dv8_dynamic=true
Files will be in out/[Release\Debug]/lib.target/libv8*.so

/CC @nodejs/build-files

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

the incantation is `./configure ... -- -Dv8_dynamic=true`
@refack refack self-assigned this Oct 1, 2018
@refack refack requested review from addaleax and targos October 1, 2018 17:58
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Oct 1, 2018
@refack refack added the blocked PRs that are blocked by other issues or PRs. label Oct 1, 2018
@refack
Copy link
Contributor Author

refack commented Oct 1, 2018

Blocked by GYP bug/undefined behavior WRT defaults.

@refack refack removed the blocked PRs that are blocked by other issues or PRs. label Oct 1, 2018
@refack
Copy link
Contributor Author

refack commented Oct 1, 2018

Fixed, PTAL

@bnoordhuis
Copy link
Member

Can you explain (also in the commit log) why this is useful or necessary?

@joyeecheung
Copy link
Member

@bnoordhuis To reduce the build time of debug builds

@joyeecheung
Copy link
Member

joyeecheung commented Oct 2, 2018

@bnoordhuis (Just realized that you might mean "why not just use ./configure ... -- -Dcomponent= shared_library"...then I guess there really isn't a good reason other than it's more convenient? Does simply setting that flag work though?)

@refack refack added the blocked PRs that are blocked by other issues or PRs. label Oct 2, 2018
@refack
Copy link
Contributor Author

refack commented Oct 2, 2018

Can you explain (also in the commit log) why this is useful or necessary?

As mentioned by @joyeecheung, it makes linking the node binary faster, which is especially significant when doing a debug build. (I'll document this as requested).

"why not just use ./configure ... -- -Dcomponent=shared_library"...

We need to conditionally turn on -fPIC (conditionally because it has a compile time and runtime penalty). Also we have:

node/node.gypi

Lines 261 to 266 in 80225d4

[ '(OS=="freebsd" or OS=="linux") and node_shared=="false"'
' and force_load=="true" and v8_dynamic!="true"', {
'ldflags': [ '-Wl,-z,noexecstack',
'-Wl,--whole-archive <(v8_base)',
'-Wl,--no-whole-archive' ]
}],

I am considering adding it as a flag to configure.py, that will come with some docu.

BTW: There's still is a bug, since now we need to also compile ICU with -fPIC, since now it's a direct dependency of V8.

@bnoordhuis
Copy link
Member

We need to conditionally turn on -fPIC

You can do that by guarding on ['component=="shared_library"', ...], it doesn't need a new variable.

@Trott
Copy link
Member

Trott commented Nov 20, 2018

What is this blocked on?

@refack
Copy link
Contributor Author

refack commented Nov 20, 2018

Making it simpler (remove the new flag, and just guard for special cases using the existing V8 flag component=="shared_library")

@refack refack closed this Dec 21, 2018
@refack refack deleted the enable-dynamic-v8 branch December 21, 2018 01:32
@refack refack restored the enable-dynamic-v8 branch January 22, 2019 21:42
@refack refack removed their assignment Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants