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: refine static and shared lib build #17604

Closed
wants to merge 1 commit into from

Conversation

yhwang
Copy link
Member

@yhwang yhwang commented Dec 11, 2017

Refine the static and shared lib build process in order
to integrate static and shared lib verfication into CI.
When building both static and shared lib, we still build
node executable now and it uses the shared and static lib.

Fixes: #14158

Signed-off-by: Yihong Wang [email protected]

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

build

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Dec 11, 2017
node.gyp Outdated
{
# When using shared lib to build executable in Windows, in order to avoid
# filename collision, the executable is node-win.exe. Need to rename it
# back to node.exe
Copy link
Member

Choose a reason for hiding this comment

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

Was discussing this with Yihong last week, and I think it's worth pointing this out specifically.

In the Unix build, we build libnode.so (which gets linked as -lnode) and node (the binary).

In Windows, we can't build node.dll and node.exe, because the node.pdb and node.lib files clash. So this PR currently builds node.exe as node-win.exe to avoid the clash.

I guess the alternatives would be:

  1. Find some neat way to avoid the conflicts and still call it node.exe (sounds like this isn't possible, but maybe there's a way).
  2. Call them node-win.dll and node.exe (I think this is worse than what the PR does currently).

So assuming 1. isn't possible, I'm leaning towards what this PR does, I guess the only downside is that the dll and pdb files will have the wrong names, maybe they should be renamed too?

Copy link
Member Author

Choose a reason for hiding this comment

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

or keep using node-win.exe and modify the test.py to pick up the new name in windows platform for this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, the problem is the clash in the symbol name? Can you use /Fd with VC++to specify the pdb filename?

node.gyp Outdated
{
'target_name': 'cctest',
'type': 'executable',

'dependencies': [
# This can't depend on node_lib_target_name, the reasoning be
# is the shared lib in Windows only exports NODE_EXTERN functions
# and cctest needs some internal functions which are not NODE_EXTERN.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should split cctest into internal and external tests, that way we could test the shared library on the external cctest tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point. I think it depends on the value of running cctest against the shared lib. If it's good and necessary, then we should do it.

@yhwang
Copy link
Member Author

yhwang commented Dec 11, 2017

A summary for the change:

  • In default build process, the static lib is built first and used to build the executable.
  • For ./configure --enable-static or .\vcbuild.bat static, they do the same thing as default build process
  • For ./configure --shared or .\vcbuild.bat dll, the shared lib is built first and the executable links to the shared lib.

For cctest, since it depends some internal APIs, it can only build from obj files in all of the cases.

@yhwang
Copy link
Member Author

yhwang commented Dec 11, 2017

I hit two Windows issues mainly when creating this PR, including:

  • building shared lib and executable with the same filename causes the filename collision.
  • when using the static lib to build the executable, even using /WHOLEARCHIVE to include
    the static lib into the executable, the functions that are not used by node_main.cc are optimized out

For first one, there are two ways to solve it (maybe more):

  • using different filename for shared lib and executable (I used this in the PR)
  • output to different sub folder

For the second one, I found two approaches,

  • use /INCLUDE option in linker. You put function names in /INCLUDE option and use comma as separator for multiple functions. Linker finds the obj files for each of the function name you specified and include the whole objs into the executable.
  • build the executable from obj files but not static lib.

Since our purpose is to used the static lib to build the executable and verify it. I used /INCLUDE in my PR. And /INCLUDE could also be coded inside the node_main.cc as #pragma. For now, I do it in the node.gyp

@mhdawson
Copy link
Member

@digitalinfinity can you take a look as many of the challenges were around windows.

@mhdawson
Copy link
Member

My 2 cents given the choices for the name of the exe and the dll I think we want to:

  1. keep the naming as node.dll so we don't break existing users
  2. do the rename to node.exe from node-win.exe (as the PR currently does) so that tests that spawn node.js still work

I do wonder if there is any dependency on node.exe when the addons built or if they just use the node.pdb and node.lib files (ie addons would build/run fine without node.exe)

@mhdawson
Copy link
Member

CI run to validate tests pass across platforms in base case (particularly interested in AIX were there was some extra steps previously) https://ci.nodejs.org/job/node-test-pull-request/12040/

@yhwang
Copy link
Member Author

yhwang commented Dec 11, 2017

@mhdawson It's good to run CI for verifying. I do see the failures. let me check those failures

@mhdawson
Copy link
Member

If you need access to AIX to test out changes just let me know

@yhwang
Copy link
Member Author

yhwang commented Dec 11, 2017

Yes, I need to access AIX to verify the change

@digitalinfinity
Copy link
Contributor

(cc @joaocgreis @bzoz since I'm technically on vacation 😄)

@yhwang forgive me, I'm missing a bit of context here so I'll ask a somewhat basic question- in your case where you're building the exe out of the static lib, what is the problem if functions not used by the exe are optimized out? Or is the problem that the exe fails to link?

@yhwang
Copy link
Member Author

yhwang commented Dec 12, 2017

@digitalinfinity I guess you were asking about Windows specifically, right? Yes, unused functions are optimized out, the N-API. Because N-API is used by user addon. In node_main.cc, N-API is not used. That's why I added /INCLUDE napi_module_register and the whole node_api.o is included into the exe. It would be good to run a dumpbin /exports Release\node.exe to see if there is any other function is optimized out. However, I locally run the .\vcbuild.bat test. seems the N-API is the only one. But comparing the output of dumpbin will be better. I am fixing the AIX issue now. I will circle back to this later.

And this issue only exists in Windows, in other platforms, the force_load or archive_whole options include the whole static lib into the executable.

@yhwang
Copy link
Member Author

yhwang commented Dec 12, 2017

Updated my change to address AIX failure. May I have another CI to verify the AIX build?

@gireeshpunathil
Copy link
Member

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

@yhwang
Copy link
Member Author

yhwang commented Dec 12, 2017

updated the change to fix fips related failures. Please kick off another CI. Thanks

@mhdawson
Copy link
Member

@digitalinfinity
Copy link
Contributor

@yhwang that's very odd- the NAPI methods are marked as __declspec(dllexport) so whole-program-optimization shouldn't optimize it away. The other question is, is it common for native module authors to link against the node exe on Windows rather than the node static lib? If not, as long as the lib has the methods that we want, we should be fine, but it wouldn't surprise me if there were modules that linked directly against the exe.

@digitalinfinity
Copy link
Contributor

Actually, I asked around and noticed this: https://blogs.msdn.microsoft.com/oldnewthing/20140321-00/?p=1433

So @yhwang it looks like the behavior you're seeing is by design and if we want those functions to actually get exported, the solution is to probably use a .DEF file

@yhwang
Copy link
Member Author

yhwang commented Dec 13, 2017

is it common for native module authors to link against the node exe on Windows rather than the node static lib?

For link, I guess people may choose shared lib. But I don’t know how many pepole use node.exe with node.dll. and agree with you that people may link to the exe for native modules.

I fixed AIX failure and found we use .exp in AIX. However, the way to generate the exp is not perfect yet. maybe we could refine the script and use it for AIX and Windows. I could create another PR for it later.

@yhwang
Copy link
Member Author

yhwang commented Dec 14, 2017

I am trying to find the failure in windows with VS 2017 now. The weird thing is I am using VS 2017 and I can build the node without any issue. Trying to recreate it first.

For smartos, @cjihrig can I access the smartos env in CI? Then I can try to fix the failure. Thanks.

@yhwang
Copy link
Member Author

yhwang commented Dec 14, 2017

for the windows failures, I can recreated it after I updated my VS2017. and the reason is one static project can't have more then one .res file. maybe merge the node_etw and node_perfctr into one... let me try it. If anyone has suggestion for this, please let me know.

@cjihrig
Copy link
Contributor

cjihrig commented Dec 14, 2017

For smartos, @cjihrig can I access the smartos env in CI? Then I can try to fix the failure. Thanks.

@yhwang, I don't have access to any of the CI machines, but someone from the build group might be able to help. cc: @mhdawson

@yhwang
Copy link
Member Author

yhwang commented Dec 15, 2017

For Windows, I updated change and the .rc files are compiled and included in shared lib and executable. For static lib case, users need to add .rc files to their projects by themselves.

May I have a new CI to verify the Windows build? If everything goes well, then only smartos left.

MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
Refine the static and shared lib build process in order
to integrate static and shared lib verfication into CI.
When building both static and shared lib, we still build
node executable now and it uses the shared and static lib.

Signed-off-by: Yihong Wang <[email protected]>

Refs: #14158
Backport-PR-URL: #19050
PR-URL: #17604
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
Refine the static and shared lib build process in order
to integrate static and shared lib verfication into CI.
When building both static and shared lib, we still build
node executable now and it uses the shared and static lib.

Signed-off-by: Yihong Wang <[email protected]>

Refs: #14158
PR-URL: #17604
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
Refine the static and shared lib build process in order
to integrate static and shared lib verfication into CI.
When building both static and shared lib, we still build
node executable now and it uses the shared and static lib.

Signed-off-by: Yihong Wang <[email protected]>

Refs: #14158
Backport-PR-URL: #19050
PR-URL: #17604
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
Refine the static and shared lib build process in order
to integrate static and shared lib verfication into CI.
When building both static and shared lib, we still build
node executable now and it uses the shared and static lib.

Signed-off-by: Yihong Wang <[email protected]>

Refs: #14158
PR-URL: #17604
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 13, 2018
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
@refack
Copy link
Contributor

refack commented May 25, 2018

P.S. this change broke ninja build on Windows (due to use of presumed location of *.lib files in a <(PRODUCT_DIR)\\lib directory, which ninja does not do).
I'm investigating ways to work around this.

I opened nodejs/build#1288 so that once we get this working again we can keep it from regressing unintentionaly.

@yhwang
Copy link
Member Author

yhwang commented May 25, 2018

@refack I can help to solve the issue. But I don't know how to use ninja to build node in windows. do you have any instruction?

BTW, currently, the master branch has an error when using .\vcbuild.bat dll. The problem is the cctest can't link to the shared lib in Windows, since some symbols that cctest need are not exported. I am trying to fix that.

richardlau added a commit to richardlau/node-1 that referenced this pull request Jan 11, 2019
nodejs#17604 refactored the gyp files
so that `-blibpath:` on AIX was only set if `node_shared=="true"`.
Restore the setting for non-shared builds.

Fixes: nodejs#25444
@richardlau richardlau mentioned this pull request Jan 11, 2019
2 tasks
addaleax pushed a commit that referenced this pull request Jan 14, 2019
#17604 refactored the gyp files
so that `-blibpath:` on AIX was only set if `node_shared=="true"`.
Restore the setting for non-shared builds.

Fixes: #25444

PR-URL: #25447
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
addaleax pushed a commit that referenced this pull request Jan 14, 2019
#17604 refactored the gyp files
so that `-blibpath:` on AIX was only set if `node_shared=="true"`.
Restore the setting for non-shared builds.

Fixes: #25444

PR-URL: #25447
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
nodejs#17604 refactored the gyp files
so that `-blibpath:` on AIX was only set if `node_shared=="true"`.
Restore the setting for non-shared builds.

Fixes: nodejs#25444

PR-URL: nodejs#25447
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 13, 2019
#17604 refactored the gyp files
so that `-blibpath:` on AIX was only set if `node_shared=="true"`.
Restore the setting for non-shared builds.

Fixes: #25444

PR-URL: #25447
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 19, 2019
#17604 refactored the gyp files
so that `-blibpath:` on AIX was only set if `node_shared=="true"`.
Restore the setting for non-shared builds.

Fixes: #25444

PR-URL: #25447
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 20, 2019
#17604 refactored the gyp files
so that `-blibpath:` on AIX was only set if `node_shared=="true"`.
Restore the setting for non-shared builds.

Fixes: #25444

PR-URL: #25447
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
#17604 refactored the gyp files
so that `-blibpath:` on AIX was only set if `node_shared=="true"`.
Restore the setting for non-shared builds.

Fixes: #25444

PR-URL: #25447
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
richardlau added a commit to richardlau/node-1 that referenced this pull request Mar 6, 2019
nodejs#17604 refactored the gyp files
so that `-blibpath:` on AIX was only set if `node_shared=="true"`.
Restore the setting for non-shared builds.

Fixes: nodejs#25444

Backport-PR-URL: nodejs#25521
PR-URL: nodejs#25447
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
richardlau added a commit to richardlau/node-1 that referenced this pull request Mar 12, 2019
nodejs#17604 refactored the gyp files
so that `-blibpath:` on AIX was only set if `node_shared=="true"`.
Restore the setting for non-shared builds.

Fixes: nodejs#25444

Backport-PR-URL: nodejs#26478
PR-URL: nodejs#25447
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Mar 15, 2019
#17604 refactored the gyp files
so that `-blibpath:` on AIX was only set if `node_shared=="true"`.
Restore the setting for non-shared builds.

Fixes: #25444

Backport-PR-URL: #26478
PR-URL: #25447
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Mar 19, 2019
#17604 refactored the gyp files
so that `-blibpath:` on AIX was only set if `node_shared=="true"`.
Restore the setting for non-shared builds.

Fixes: #25444

Backport-PR-URL: #25521
PR-URL: #25447
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
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.

doc: better document --shared build