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.

Sign up for GitHub

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

Jump to bottom

test: --enable-static linked executable #14986

Closed
wants to merge 7 commits into from
Closed

test: --enable-static linked executable #14986

wants to merge 7 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Aug 23, 2017 •
edited by refack
Loading

Refs: #14158
Refs: #14892

The motivation for this commit is to enable projects embedding Node.js
and building with --enable-static to be able to run the test suite and
linter.

Currently when building with --enable-static no node executable
will be created which means that the tests (apart from the cctest) and
linter cannot be run.

This is currently a work in progress and works on MacOS but I need to
run the CI, and manually on different environments to verify that it
works as expected.

[refack: added refs]

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

test, build

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 23, 2017
@danbev danbev added the wip Issues and PRs that are still a work in progress. label Aug 23, 2017
TimothyGu
TimothyGu reviewed Aug 23, 2017
View reviewed changes
node.gyp Outdated
],
'xcode_settings': {
'OTHER_LDFLAGS': [
'-Wl,-force_load,<(PRODUCT_DIR)/libnode.a',
Copy link
Member

Choose a reason for hiding this comment

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

Weird indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll fix that.

node.gyp Outdated
],
},
'conditions': [
['OS in "linux freebsd"', {
Copy link
Member

Choose a reason for hiding this comment

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

You're right, this does look like it's missing a couple of platforms where --whole-archive can be used. We also need to disable the static node build on platforms that don't support.

@danbev
Copy link
Contributor Author

danbev commented Aug 23, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/9796/

refack
refack reviewed Aug 23, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Love it!

node.gyp Outdated
{
'target_name': 'static_node',
'type': 'executable',
'product_name': 'node',
Copy link
Contributor

@refack refack Aug 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think this should be <(node_core_target_name) as well?
Also AFAICT product_name is only supported for xcode, MSVS, and cmake, so we might need to find a different trick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll update that. Thanks

@refack
Copy link
Contributor

refack commented Aug 23, 2017 •
edited
Loading

@danbev great idea!

  1. We need to find a way to make sure the CI really tests this...
  2. Did you check that the Makefile generator works with product_name. I'm looking in the GYP code and it's not obvious that it does (there is explicit code to support this for cmake, xcode and msvs).
  3. Found code for support in make, ninja, cmake, xcode and msvs, now I suspect eclipse and our own json output (e6eac85)

@refack refack mentioned this pull request Aug 23, 2017
Closed
2 tasks
@hiroppy hiroppy added the test Issues and PRs related to the tests. label Aug 26, 2017
@BridgeAR
Copy link
Member

This needs some LG @nodejs/collaborators

@BridgeAR
Copy link
Member

Oh, I overlooked the work in progress.

@danbev
Copy link
Contributor Author

danbev commented Aug 31, 2017

Oh, I overlooked the work in progress.

No worries, I hope to revisit this in a day or so (some other stuff got in the way).

@mhdawson
Copy link
Member

mhdawson commented Sep 6, 2017

Just to confirm my understanding this only affects when you build a static library, and basically adds generation of an executable which bundles in that static library. If so looks good to me and I'd like the equivalent for the shared library version as well. Once we have that I think we should be building/testing nightly in the CI as well and I'm happy to help set that up.

@mhdawson
Copy link
Member

mhdawson commented Sep 6, 2017

@danbev from the comments I think you still have some changes before people LG, but if thats not the case please let me know.

@danbev
Copy link
Contributor Author

danbev commented Sep 7, 2017

from the comments I think you still have some changes before people LG

Yeah, I still need to figure out if this will work on the various platforms. I'm currently looking (when I have a little extra time) into the windows build which is failing for me. Sounds great to have this tested on the CI after that!

@BridgeAR
Copy link
Member

Ping @danbev

@danbev
Copy link
Contributor Author

danbev commented Sep 14, 2017

@BridgeAR I've verified that this works on Linux but still need to figure out an issue on Windows. I've been busy with other things unfortunately, but I'm going to take another look now.

@danbev
Copy link
Contributor Author

danbev commented Sep 14, 2017 •
edited
Loading

windows compilation error 
>vcbuild.bat test static
...

C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(39): error C2065: 'WORD': undeclared identifier [C:\User
s\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(39): error C2146: syntax error: missing ')' before ident
ifier 'wMajorVersion' [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(40): error C2143: syntax error: missing ';' before '{' [
C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(40): error C2447: '{': missing function header (old-styl
e formal list?) [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(59): error C2065: '_WIN32_WINNT_WINXP': undeclared ident
ifier [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(59): error C3861: 'HIBYTE': identifier not found [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(59): error C3861: 'LOBYTE': identifier not found [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(59): error C3861: 'IsWindowsVersionOrGreater': identifie
r not found [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(65): error C2065: '_WIN32_WINNT_WINXP': undeclared ident
ifier [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(65): error C3861: 'HIBYTE': identifier not found [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(65): error C3861: 'LOBYTE': identifier not found [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(65): error C3861: 'IsWindowsVersionOrGreater': identifie
r not found [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(71): error C2065: '_WIN32_WINNT_WINXP': undeclared ident
ifier [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(71): error C3861: 'HIBYTE': identifier not found [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(71): error C3861: 'LOBYTE': identifier not found [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(71): error C3861: 'IsWindowsVersionOrGreater': identifie
r not found [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(77): error C2065: '_WIN32_WINNT_WINXP': undeclared ident
ifier [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(77): error C3861: 'HIBYTE': identifier not found [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(77): error C3861: 'LOBYTE': identifier not found [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(77): error C3861: 'IsWindowsVersionOrGreater': identifie
r not found [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(83): error C2065: '_WIN32_WINNT_VISTA': undeclared ident
ifier [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(83): error C3861: 'HIBYTE': identifier not found [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(83): error C3861: 'LOBYTE': identifier not found [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(83): error C3861: 'IsWindowsVersionOrGreater': identifie
r not found [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(89): error C2065: '_WIN32_WINNT_VISTA': undeclared ident
ifier [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(89): error C3861: 'HIBYTE': identifier not found [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(89): error C3861: 'LOBYTE': identifier not found [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(89): error C3861: 'IsWindowsVersionOrGreater': identifie
r not found [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(95): error C2065: '_WIN32_WINNT_VISTA': undeclared ident
ifier [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(95): error C3861: 'HIBYTE': identifier not found [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(95): error C3861: 'LOBYTE': identifier not found [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(95): error C3861: 'IsWindowsVersionOrGreater': identifie
r not found [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(101): error C2065: '_WIN32_WINNT_WIN7': undeclared ident
ifier [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(101): error C3861: 'HIBYTE': identifier not found [C:\Us
ers\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(101): error C3861: 'LOBYTE': identifier not found [C:\Us
ers\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(101): error C3861: 'IsWindowsVersionOrGreater': identifi
er not found [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(107): error C2065: '_WIN32_WINNT_WIN7': undeclared ident
ifier [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(107): error C3861: 'HIBYTE': identifier not found [C:\Us
ers\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(107): error C3861: 'LOBYTE': identifier not found [C:\Us
ers\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(107): error C3861: 'IsWindowsVersionOrGreater': identifi
er not found [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(113): error C2065: '_WIN32_WINNT_WIN8': undeclared ident
ifier [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(113): error C3861: 'HIBYTE': identifier not found [C:\Us
ers\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(113): error C3861: 'LOBYTE': identifier not found [C:\Us
ers\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(113): error C3861: 'IsWindowsVersionOrGreater': identifi
er not found [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(119): error C2065: '_WIN32_WINNT_WINBLUE': undeclared id
entifier [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(119): error C3861: 'HIBYTE': identifier not found [C:\Us
ers\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(119): error C3861: 'LOBYTE': identifier not found [C:\Us
ers\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(119): error C3861: 'IsWindowsVersionOrGreater': identifi
er not found [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(125): error C2065: 'OSVERSIONINFOEXW': undeclared identi
fier [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(125): error C2146: syntax error: missing ';' before iden
tifier 'osvi' [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(125): error C2065: 'osvi': undeclared identifier [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(125): error C2065: 'VER_NT_WORKSTATION': undeclared iden
tifier [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(126): error C2065: 'DWORDLONG': undeclared identifier [C
:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(126): error C2143: syntax error: missing ';' before 'con
st' [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(126): error C4430: missing type specifier - int assumed.
 Note: C++ does not support default-int [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(126): error C2065: 'VER_PRODUCT_TYPE': undeclared identi
fier [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(126): error C2065: 'VER_EQUAL': undeclared identifier [C
:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(126): error C3861: 'VerSetConditionMask': identifier not
 found [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(128): error C2065: 'osvi': undeclared identifier [C:\Use
rs\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(128): error C2065: 'VER_PRODUCT_TYPE': undeclared identi
fier [C:\Users\danbev\working\node\static_node.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\VersionHelpers.h(128): error C3861: 'VerifyVersionInfoW': identifier not
found [C:\Users\danbev\working\node\static_node.vcxproj]
src\node_main.cc(39): error C2065: 'DWORD': undeclared identifier [C:\Users\danbev\working\node\static_node.vcxproj]
src\node_main.cc(39): error C2146: syntax error: missing ';' before identifier 'size' [C:\Users\danbev\working\node\static_n
ode.vcxproj]
src\node_main.cc(39): error C2065: 'size': undeclared identifier [C:\Users\danbev\working\node\static_node.vcxproj]
src\node_main.cc(39): error C2065: 'CP_UTF8': undeclared identifier [C:\Users\danbev\working\node\static_node.vcxproj]
src\node_main.cc(39): error C3861: 'WideCharToMultiByte': identifier not found [C:\Users\danbev\working\node\static_node.vcx
proj]
src\node_main.cc(47): error C2065: 'size': undeclared identifier [C:\Users\danbev\working\node\static_node.vcxproj]
src\node_main.cc(53): error C2065: 'size': undeclared identifier [C:\Users\danbev\working\node\static_node.vcxproj]
src\node_main.cc(54): error C2065: 'DWORD': undeclared identifier [C:\Users\danbev\working\node\static_node.vcxproj]
src\node_main.cc(54): error C2146: syntax error: missing ';' before identifier 'result' [C:\Users\danbev\working\node\static
_node.vcxproj]
src\node_main.cc(54): error C2065: 'result': undeclared identifier [C:\Users\danbev\working\node\static_node.vcxproj]
src\node_main.cc(54): error C2065: 'CP_UTF8': undeclared identifier [C:\Users\danbev\working\node\static_node.vcxproj]
src\node_main.cc(59): error C2065: 'size': undeclared identifier [C:\Users\danbev\working\node\static_node.vcxproj]
src\node_main.cc(54): error C3861: 'WideCharToMultiByte': identifier not found [C:\Users\danbev\working\node\static_node.vcx
proj]
src\node_main.cc(62): error C2065: 'result': undeclared identifier [C:\Users\danbev\working\node\static_node.vcxproj]

I was able to work around this by including windows.h in src/node_main.cc.

@BridgeAR
Copy link
Member

PTAL @refack @nodejs/platform-windows

@BridgeAR
Copy link
Member

Ping @nodejs/platform-windows again. PTAL

@danbev
Copy link
Contributor Author

danbev commented Sep 25, 2017

I've run into linking issue and I'm trying to get the /WHOLEARCHIVE flag to work which I think should sort it out.

@bzoz
Copy link
Contributor

bzoz commented Sep 25, 2017

I've tried copying various settings from Node executable target:

    [ 'node_target_type=="static_library"', {
      'targets': [
        {
          'target_name': 'static_node',
          'type': 'executable',
          'product_name': '<(node_core_target_name)',
          'dependencies': [
            '<(node_core_target_name)',
            'deps/uv/uv.gyp:libuv'
          ],
          'sources+': [
            'src/node_main.cc',
          ],
          'include_dirs': [
            'deps/v8/include',
            'src',
            'tools/msvs/genfiles',
            'deps/uv/src/ares',
            '<(SHARED_INTERMEDIATE_DIR)', # for node_natives.h
            'deps/nghttp2/lib/includes'
          ],

          'defines': [
            'NODE_ARCH="<(target_arch)"',
            'NODE_PLATFORM="<(OS)"',
            'NODE_WANT_INTERNALS=1',
            # Warn when using deprecated V8 APIs.
            'V8_DEPRECATION_WARNINGS=1',
            # We're using the nghttp2 static lib
            'NGHTTP2_STATICLIB'
          ],

          'xcode_settings': {
            'OTHER_LDFLAGS': [
              '-Wl,-force_load,<(PRODUCT_DIR)/libnode.a',
            ],
          },

It kinda works - it compiles, but it won't start:

bootstrap_node.js:484
  const ContextifyScript = process.binding('contextify').ContextifyScript;
                                   ^

Error: No such module: contextify
    at bootstrap_node.js:484:36

@bnoordhuis
Copy link
Member

That's because of the __attribute__((constructor)) in node.h. It doesn't work for static libraries, the linker won't pull in the constructors because there is nothing that references them.

@yhwang
Copy link
Member

yhwang commented Sep 27, 2017

Here is one thought based on the change:
could we have a static_lib target to build the static library and change node target to depends on static_lib. in this way, when building the the node executable:

  • the static lib is built
  • the node executable directly uses static lib
  • when running the test, it also verify the static lib that is used by node executable

is this feasible?

@danbev
Copy link
Contributor Author

danbev commented Sep 28, 2017

is this feasible?

Please correct me if I'm wrong, but this is basically what this PR is trying to achieve. When using the --enable-static option to configure, the node executable will link to the static lib and the tests will be run with that statically linked executable.

@yhwang
Copy link
Member

yhwang commented Sep 28, 2017 •
edited
Loading

@danbev my suggestion is to directly remove --enable-static option. In normal build target, we should always build static library first and then the executable. so normal test will indirectly verify the static library. The only thing left is the shared library.

Edit
maybe I shouldn't say remove --enable-static. the idea is when building node executable, the static lib is built first and then the node executable. Just like what you did in the new section you added. But the difference is the section you added is not only executed when using --enable-static but also the normal node executable.

@danbev
Copy link
Contributor Author

danbev commented Sep 28, 2017

@yhwang That could certainly be done but I was under the assumption that the default would be to build just build the node executable as is currently done.

Then the two other cases, building node as a static or shared library, would build a node executable that was either statically linked or dynamically linked and in both cases the tests would be run using the node executable created.

My view of this is that as a user I would either want to build a normal executable for node, build node as a static library, or as a shared library. Having these separate makes sense to me.

@bnoordhuis
Copy link
Member

maybe I shouldn't say remove --enable-static. the idea is when building node executable, the static lib is built first and then the node executable.

That currently won't work because of #14986 (comment) unless you link with --whole-archive but then dead code isn't pruned.

Code in the src directory would need to stop using __attribute__((constructor)), i.e., stop using NODE_MODULE_CONTEXT_AWARE_BUILTIN.

Technically you could make it work by adding a void (*symhack[])() = { ... } to src/node.cc that adds references to all constructor functions but that's finicky.

@yhwang
Copy link
Member

yhwang commented Sep 28, 2017

@danbev I agree with you that we still provide shared lib, static lib and executable. So these things won't change. My suggestion is if we build static lib when building the default node executable target, then I guess no need to add extra CI task to verify static lib separately. The current CI would be sufficient.

@bnoordhuis yes I was talking that we should use @danbev 's change in node executable target.

@bnoordhuis
Copy link
Member

bnoordhuis commented Sep 28, 2017 •
edited
Loading

The salient point is that it currently won't work (can't work) for the aforementioned reasons.

edit: to be clear, I don't think it's a bad idea but we'll need to shuffle some code around or use (the Windows equivalent of) --whole-archive to make it work.

@danbev
Copy link
Contributor Author

danbev commented Sep 29, 2017

I'm using to use /WHOLEARCHIVE like in the following snippet, but running into a link issue (see below for details)

'msvs_settings': {
  'VCLinkerTool': {
    'AdditionalOptions': ['/WHOLEARCHIVE:<(PRODUCT_DIR)/lib/node.lib'],
},

Hopefully this will be sorted out soon and we can discuss any changes to the current build like @yhwang brought up.

link error

console output:

C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\amd64\link.exe /ERRORREPORT:QUEUE /OUT:"C:\Users\danbev\working
  \node\Release\node.exe" /INCREMENTAL:NO /NOLOGO dbghelp.lib shlwapi.lib winmm.lib AdvAPI32.lib User32.lib gdi32.lib advapi
  32.lib iphlpapi.lib psapi.lib shell32.lib user32.lib userenv.lib ws2_32.lib /MANIFEST /MANIFESTUAC:"level='asInvoker' uiAc
  cess='false'" /manifest:embed /DEBUG /PDB:"C:\Users\danbev\working\node\Release\node.pdb" /MAP /MAPINFO:EXPORTS /OPT:REF /
  OPT:ICF /LTCG /TLBID:1 /DYNAMICBASE /NXCOMPAT /IMPLIB:"C:\Users\danbev\working\node\Release\node.lib" /MACHINE:X64 /WHOLEA
  RCHIVE:C:\Users\danbev\working\node\Release\lib/node.lib /SubSystem:Console,"5.02" Release\obj\static_node\node_main.obj
  C:\Users\danbev\working\node\Release\lib\node.lib
  C:\Users\danbev\working\node\Release\lib\nghttp2.lib
  C:\Users\danbev\working\node\Release\lib\v8_base_0.lib
  C:\Users\danbev\working\node\Release\lib\v8_base_1.lib
  C:\Users\danbev\working\node\Release\lib\v8_base_2.lib
  C:\Users\danbev\working\node\Release\lib\v8_base_3.lib
  C:\Users\danbev\working\node\Release\lib\v8_libbase.lib
  C:\Users\danbev\working\node\Release\lib\v8_libsampler.lib
  C:\Users\danbev\working\node\Release\lib\icui18n.lib
  C:\Users\danbev\working\node\Release\lib\icuucx.lib
  C:\Users\danbev\working\node\Release\lib\icudata.lib
  C:\Users\danbev\working\node\Release\lib\icustubdata.lib
  C:\Users\danbev\working\node\Release\lib\v8_snapshot.lib
  C:\Users\danbev\working\node\Release\lib\v8_libplatform.lib
  C:\Users\danbev\working\node\Release\lib\openssl.lib
  C:\Users\danbev\working\node\Release\lib\zlib.lib
  C:\Users\danbev\working\node\Release\lib\http_parser.lib
  C:\Users\danbev\working\node\Release\lib\cares.lib
  C:\Users\danbev\working\node\Release\lib\libuv.lib
  Tracking command:
  C:\Program Files (x86)\MSBuild\14.0\bin\Tracker.exe /a /d "C:\Program Files (x86)\MSBuild\14.0\bin\FileTracker32.dll" /i C
  :\Users\danbev\working\node\Release\obj\static_node\static_node.tlog /r "C:\USERS\DANBEV\WORKING\NODE\RELEASE\LIB\CARES.LI
  B|C:\USERS\DANBEV\WORKING\NODE\RELEASE\LIB\HTTP_PARSER.LIB|C:\USERS\DANBEV\WORKING\NODE\RELEASE\LIB\ICUDATA.LIB|C:\USERS\D
  ANBEV\WORKING\NODE\RELEASE\LIB\ICUI18N.LIB|C:\USERS\DANBEV\WORKING\NODE\RELEASE\LIB\ICUSTUBDATA.LIB|C:\USERS\DANBEV\WORKIN
  G\NODE\RELEASE\LIB\ICUUCX.LIB|C:\USERS\DANBEV\WORKING\NODE\RELEASE\LIB\LIBUV.LIB|C:\USERS\DANBEV\WORKING\NODE\RELEASE\LIB\
  NGHTTP2.LIB|C:\USERS\DANBEV\WORKING\NODE\RELEASE\LIB\NODE.LIB|C:\USERS\DANBEV\WORKING\NODE\RELEASE\LIB\OPENSSL.LIB|C:\USER
  S\DANBEV\WORKING\NODE\RELEASE\LIB\V8_BASE_0.LIB|C:\USERS\DANBEV\WORKING\NODE\RELEASE\LIB\V8_BASE_1.LIB|C:\USERS\DANBEV\WOR
  KING\NODE\RELEASE\LIB\V8_BASE_2.LIB|C:\USERS\DANBEV\WORKING\NODE\RELEASE\LIB\V8_BASE_3.LIB|C:\USERS\DANBEV\WORKING\NODE\RE
  LEASE\LIB\V8_LIBBASE.LIB|C:\USERS\DANBEV\WORKING\NODE\RELEASE\LIB\V8_LIBPLATFORM.LIB|C:\USERS\DANBEV\WORKING\NODE\RELEASE\
  LIB\V8_LIBSAMPLER.LIB|C:\USERS\DANBEV\WORKING\NODE\RELEASE\LIB\V8_SNAPSHOT.LIB|C:\USERS\DANBEV\WORKING\NODE\RELEASE\LIB\ZL
  IB.LIB|C:\USERS\DANBEV\WORKING\NODE\RELEASE\OBJ\STATIC_NODE\NODE_MAIN.OBJ" /b MSBuildConsole_CancelEventcb658c3e654c4b1e8d
  911ba93f10811e  /c "C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\amd64\link.exe"  /ERRORREPORT:QUEUE /OUT:"C
  :\Users\danbev\working\node\Release\node.exe" /INCREMENTAL:NO /NOLOGO dbghelp.lib shlwapi.lib winmm.lib AdvAPI32.lib User3
  2.lib gdi32.lib advapi32.lib iphlpapi.lib psapi.lib shell32.lib user32.lib userenv.lib ws2_32.lib /MANIFEST /MANIFESTUAC:"
  level='asInvoker' uiAccess='false'" /manifest:embed /DEBUG /PDB:"C:\Users\danbev\working\node\Release\node.pdb" /MAP /MAPI
  NFO:EXPORTS /OPT:REF /OPT:ICF /LTCG /TLBID:1 /DYNAMICBASE /NXCOMPAT /IMPLIB:"C:\Users\danbev\working\node\Release\node.lib
  " /MACHINE:X64 /WHOLEARCHIVE:C:\Users\danbev\working\node\Release\lib/node.lib /SubSystem:Console,"5.02" Release\obj\stati
  c_node\node_main.obj
  C:\Users\danbev\working\node\Release\lib\node.lib
  C:\Users\danbev\working\node\Release\lib\nghttp2.lib
  C:\Users\danbev\working\node\Release\lib\v8_base_0.lib
  C:\Users\danbev\working\node\Release\lib\v8_base_1.lib
  C:\Users\danbev\working\node\Release\lib\v8_base_2.lib
  C:\Users\danbev\working\node\Release\lib\v8_base_3.lib
  C:\Users\danbev\working\node\Release\lib\v8_libbase.lib
  C:\Users\danbev\working\node\Release\lib\v8_libsampler.lib
  C:\Users\danbev\working\node\Release\lib\icui18n.lib
  C:\Users\danbev\working\node\Release\lib\icuucx.lib
  C:\Users\danbev\working\node\Release\lib\icudata.lib
  C:\Users\danbev\working\node\Release\lib\icustubdata.lib
  C:\Users\danbev\working\node\Release\lib\v8_snapshot.lib
  C:\Users\danbev\working\node\Release\lib\v8_libplatform.lib
  C:\Users\danbev\working\node\Release\lib\openssl.lib
  C:\Users\danbev\working\node\Release\lib\zlib.lib
  C:\Users\danbev\working\node\Release\lib\http_parser.lib
  C:\Users\danbev\working\node\Release\lib\cares.lib
  C:\Users\danbev\working\node\Release\lib\libuv.lib
node.lib(node_perfctr_provider.res) : fatal error LNK1241: resource file node.lib(node.res) already specified [C:\Users\danb
ev\working\node\static_node.vcxproj]
  The command exited with code 1241.
Done executing task "Link" -- FAILED.
Done building target "Link" in project "static_node.vcxproj" -- FAILED.
Done Building Project "C:\Users\danbev\working\node\static_node.vcxproj" (default targets) -- FAILED.

Done executing task "MSBuild" -- FAILED.
Done building target "Build" in project "static_node.vcxproj.metaproj" -- FAILED.
Done Building Project "C:\Users\danbev\working\node\static_node.vcxproj.metaproj" (default targets) -- FAILED.

Done executing task "MSBuild" -- FAILED.
Done building target "Build" in project "node.sln" -- FAILED.
Done Building Project "C:\Users\danbev\working\node\node.sln" (Build target(s)) -- FAILED.


Build FAILED.

Time Elapsed 00:11:23.87

@danbev
Copy link
Contributor Author

danbev commented Nov 15, 2017

Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/11443/

danbev added a commit to danbev/node that referenced this pull request Nov 16, 2017
@danbev
test: --enable-static linked executable
b021403 
The motivation for this commit is to enable projects embedding Node.js
and building with --enable-static to be able to run the test suite and
linter.

Currently when building with --enable-static no node executable
will be created which means that the tests (apart from the cctest) and
linter cannot be run.

This is currently a work in progress and works on MacOS but I need to
run the CI, and manually on different environments to verify that it
works as expected.

PR-URL: nodejs#14986
Refs: nodejs#14158
Refs: nodejs#14892
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
danbev added a commit to danbev/node that referenced this pull request Nov 16, 2017
@danbev
Revert "build: for --enable-static, run only cctest"
9ae81b9 
This reverts commit a36b540.

PR-URL: nodejs#14986
Refs: nodejs#14158
Refs: nodejs#14892
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@danbev
Copy link
Contributor Author

danbev commented Nov 16, 2017

Landed in b021403

Thanks for everyones help/feedback, and patience on this long running issue!

@danbev danbev closed this Nov 16, 2017
@danbev danbev deleted the enable-static-node-test branch November 16, 2017 08:34
@gireeshpunathil
Copy link
Member

thanks to you too @danbev , for your perseverance!

MylesBorins pushed a commit that referenced this pull request Dec 11, 2017
@danbev @MylesBorins
test: --enable-static linked executable
70060ee 
The motivation for this commit is to enable projects embedding Node.js
and building with --enable-static to be able to run the test suite and
linter.

Currently when building with --enable-static no node executable
will be created which means that the tests (apart from the cctest) and
linter cannot be run.

This is currently a work in progress and works on MacOS but I need to
run the CI, and manually on different environments to verify that it
works as expected.

PR-URL: #14986
Refs: #14158
Refs: #14892
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 11, 2017
@danbev @MylesBorins
Revert "build: for --enable-static, run only cctest"
2eec944 
This reverts commit a36b540.

PR-URL: #14986
Refs: #14158
Refs: #14892
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
yhwang added a commit to yhwang/node that referenced this pull request Dec 12, 2017
@yhwang
src: explicitly register built-in modules
d979e2e 
Previously, built-in modules are registered before main() via
__attribute__((constructor)) mechanism in GCC and similiar
mechanism in MSVC. This causes some issues when node is built as
static library. Calling module registration function for built-in
modules in node::Init() helps to avoid the issues.

Signed-off-by: Yihong Wang <[email protected]>
Original PR-URL: nodejs#16565
Refs: nodejs#14986 (comment)
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@yhwang yhwang mentioned this pull request Dec 12, 2017
Closed
2 tasks
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@yhwang @MylesBorins
src: explicitly register built-in modules
a37dce4 
Previously, built-in modules are registered before main() via
__attribute__((constructor)) mechanism in GCC and similiar
mechanism in MSVC. This causes some issues when node is built as
static library. Calling module registration function for built-in
modules in node::Init() helps to avoid the issues.

Signed-off-by: Yihong Wang <[email protected]>
Refs: #14986 (comment)
Backport-PR-URL: #17625
PR-URL: #16565
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@yhwang @MylesBorins
src: explicitly register built-in modules
9c7a42a 
Previously, built-in modules are registered before main() via
__attribute__((constructor)) mechanism in GCC and similiar
mechanism in MSVC. This causes some issues when node is built as
static library. Calling module registration function for built-in
modules in node::Init() helps to avoid the issues.

Signed-off-by: Yihong Wang <[email protected]>
Refs: #14986 (comment)
Backport-PR-URL: #17625
PR-URL: #16565
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@danbev @gibfahn
test: --enable-static linked executable
c69590d 
The motivation for this commit is to enable projects embedding Node.js
and building with --enable-static to be able to run the test suite and
linter.

Currently when building with --enable-static no node executable
will be created which means that the tests (apart from the cctest) and
linter cannot be run.

This is currently a work in progress and works on MacOS but I need to
run the CI, and manually on different environments to verify that it
works as expected.

PR-URL: #14986
Refs: #14158
Refs: #14892
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@MylesBorins
Copy link
Contributor

This does not land cleanly in v6.x LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@danbev @gibfahn
test: --enable-static linked executable
b1bcd4f 
The motivation for this commit is to enable projects embedding Node.js
and building with --enable-static to be able to run the test suite and
linter.

Currently when building with --enable-static no node executable
will be created which means that the tests (apart from the cctest) and
linter cannot be run.

This is currently a work in progress and works on MacOS but I need to
run the CI, and manually on different environments to verify that it
works as expected.

PR-URL: #14986
Refs: #14158
Refs: #14892
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jan 17, 2018
@yhwang @gibfahn
src: explicitly register built-in modules
5d03e0c 
Previously, built-in modules are registered before main() via
__attribute__((constructor)) mechanism in GCC and similiar
mechanism in MSVC. This causes some issues when node is built as
static library. Calling module registration function for built-in
modules in node::Init() helps to avoid the issues.

Signed-off-by: Yihong Wang <[email protected]>
PR-URL: nodejs#16565
Backport-PR-URL: nodejs#18179
Refs: nodejs#14986 (comment)
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2018
@yhwang @MylesBorins
src: explicitly register built-in modules
e71beba 
Previously, built-in modules are registered before main() via
__attribute__((constructor)) mechanism in GCC and similiar
mechanism in MSVC. This causes some issues when node is built as
static library. Calling module registration function for built-in
modules in node::Init() helps to avoid the issues.

Signed-off-by: Yihong Wang <[email protected]>
Backport-PR-URL: #18179
PR-URL: #16565
Refs: #14986 (comment)
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@aschaeffer aschaeffer mentioned this pull request Oct 6, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu left review comments

@yhwang yhwang yhwang left review comments

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. embedding Issues and PRs related to embedding Node.js in another project. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.