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: fix static dll compilation on Windows #30695

Closed
wants to merge 3 commits into from
Closed

build: fix static dll compilation on Windows #30695

wants to merge 3 commits into from

Conversation

davidhouweling
Copy link

@davidhouweling davidhouweling commented Nov 28, 2019

build: fix static dll compilation on Windows

This PR fixes the static dll compilation steps failing.
I'm uncertain about adding tests for this as I spotted in
the gyp files that shared mode isn't supported so to speak.

I tested this by running the following commands successfully:

  • .\vcbuild.bat dll
  • .\vcbuild.bat dll x86

Note: I encountered an issue when compiling a different
architecture after compiling the first for node 12 (though it
wasn't a problem against master). But if you delete the out
folder after the first build, the second build will pass.

Fixes: #28845

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 build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Nov 28, 2019
@addaleax addaleax added the windows Issues and PRs related to the Windows platform. label Nov 30, 2019
@@ -720,6 +723,7 @@
'libraries': [
'Dbghelp',
'Psapi',
'Winmm',
Copy link
Member

Choose a reason for hiding this comment

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

If this is necessary to build v8_base, shouldn't this be placed in the v8 gypfiles? cc @targos @ryzokuken

Copy link
Member

Choose a reason for hiding this comment

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

Possibly. What exactly in V8 uses Winmm APIs?

Copy link
Author

Choose a reason for hiding this comment

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

#28845 (comment) was the initial explanation as to why I had to add it at this level, so potentially there is something wrong in the gyp configuration of v8?

Regarding what is using it exactly, it's the use of __imp_timeGetTime. From the errors (as shown in #28845 (comment)) I Googled it and the solution was to add Winmm.

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 blocking?

@Trott
Copy link
Member

Trott commented Dec 31, 2019

@nodejs/platform-windows

when compiling windows dll (shared mode), v8_libbase.vcxproj requires
winmm.lib which are not being picked up by libnode. In the normal mode
it is picked up via another dependency route. However, in the shared
mode, the indirect dependency is not added.

Fixes: #28845
When compiling node as a dll, libnode must be compiled as a lib
instead of a dll. Having it compiled as a dll triggers node.vcxproj
to compile libnode as a lib, meaning duplicate definitions appear
causing the compiler to throw an error when compiling node

Fixes: #28845
When compiling node in shared mode, the output should be a
shared library rather than an executable.

Fixes: #28845
@FullyArticulate
Copy link

FullyArticulate commented Mar 18, 2020

This patch fixes Windows, but breaks Linux, AFAIK.
Under Linux, configured static with ninja.
Ninja complains:

ninja: warning: multiple rules generate libnode.a builds involving this target will not be correct; continuing anyway

Simultaneously, out/Release/node is not generated under Linux.

If I roll back the patch, everything is created just fine on Linux, but fails under Windows for the reasons you identified.

@GoddessLuBoYan
Copy link

excuse me, now I see the error again.
node version: release 13.11.0
os: windows7

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@davidhouweling
Copy link
Author

I just tried this against master and was able to successfully build both x64 and x86. Given this i'll close this ticket.

@davidhouweling davidhouweling deleted the fix/dll branch June 18, 2020 06:02
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. doc Issues and PRs related to the documentations. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot build static dll on windows v12.7.0
9 participants