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 static libnode_internal to avoid cyclic dependency on mkcodecache #28891

Closed
wants to merge 2 commits into from
Closed

Build static libnode_internal to avoid cyclic dependency on mkcodecache #28891

wants to merge 2 commits into from

Conversation

jeroen
Copy link
Contributor

@jeroen jeroen commented Jul 29, 2019

Here is a first draft to fix #27431 by implementing exactly this:

node/node.gyp

Lines 1161 to 1166 in 62a809f

# TODO(joyeecheung): do not depend on node_lib,
# instead create a smaller static library node_lib_base that does
# just enough for node_native_module.cc and the cache builder to
# compile without compiling the generated code cache C++ file.
# So generate_code_cache -> mkcodecache -> node_lib_base,
# node_lib -> node_lib_base & generate_code_cache

The problem here is that the libnode shared library is missing the code cache symbols because of the cyclical dependency between libnode and mkcodecache in node.gyp.

We need to build mkcodecache to generate node_code_cache.cc. However currently mkcodecache itself depends on libnode so we cannot include the generated symbols to libnode. In the current setup node_code_cache.cc is linked when building node.exe but when building node as a shared library the symbols are missing libnode, which causes the issues in #27431.

The solution is described comment above: modify the mkcodecache build such that it does not depend on the full libnode, but instead on a smaller libnode_internal which suffices to generate the cache symbols. And then we can include node_code_cache.cc in libnode.so.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • 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 Jul 29, 2019
@Trott
Copy link
Member

Trott commented Jul 31, 2019

@nodejs/node-gyp @joyeecheung

'NODE_OPENSSL_SYSTEM_CERT_PATH=""',
],

'sources': [
Copy link
Member

Choose a reason for hiding this comment

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

is this list duplicated now?

@Trott
Copy link
Member

Trott commented Aug 1, 2019

Pinging some other folks that I suspect might have opinions: @indutny @addaleax @jasnell @danbev

@jeroen
Copy link
Contributor Author

jeroen commented Aug 1, 2019

The bug #27431 has already been fixed via a more simple solution in #28897.

Ideally a rewrite of the build as above would probably be better so that we get the same cached symbols when calling libnode from a 3rd party application as in node itself. However this is just a small performance gain and for my purposes the workaround from #28897 suffices.

@jeroen jeroen closed this Aug 1, 2019
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Link issue with static library of nodejs
4 participants