build: refine static and shared lib build #17604
Conversation
node.gyp
Outdated
There was a problem hiding this comment.
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:
- 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). - Call them
node-win.dllandnode.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?
There was a problem hiding this comment.
or keep using node-win.exe and modify the test.py to pick up the new name in windows platform for this case?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Maybe we should split cctest into internal and external tests, that way we could test the shared library on the external cctest tests.
There was a problem hiding this comment.
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.
|
A summary for the change:
For cctest, since it depends some internal APIs, it can only build from obj files in all of the cases. |
|
I hit two Windows issues mainly when creating this PR, including:
For first one, there are two ways to solve it (maybe more):
For the second one, I found two approaches,
Since our purpose is to used the static lib to build the executable and verify it. I used |
|
@digitalinfinity can you take a look as many of the challenges were around windows. |
|
My 2 cents given the choices for the name of the exe and the dll I think we want to:
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) |
|
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/ |
|
@mhdawson It's good to run CI for verifying. I do see the failures. let me check those failures |
|
If you need access to AIX to test out changes just let me know |
|
Yes, I need to access AIX to verify the change |
|
(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? |
|
@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 And this issue only exists in Windows, in other platforms, the |
|
Updated my change to address AIX failure. May I have another CI to verify the AIX build? |
|
updated the change to fix fips related failures. Please kick off another CI. Thanks |
|
Another CI run: https://ci.nodejs.org/job/node-test-pull-request/12067/console |
|
@yhwang that's very odd- the NAPI methods are marked as |
|
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 |
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. |
|
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. |
|
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 |
|
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. |
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 <yh.wang@ibm.com> Refs: #14158 Backport-PR-URL: #19050 PR-URL: #17604 Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
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 <yh.wang@ibm.com> Refs: #14158 PR-URL: #17604 Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
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 <yh.wang@ibm.com> Refs: #14158 Backport-PR-URL: #19050 PR-URL: #17604 Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
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 <yh.wang@ibm.com> Refs: #14158 PR-URL: #17604 Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
|
P.S. this change broke I opened nodejs/build#1288 so that once we get this working again we can keep it from regressing unintentionaly. |
|
@refack I can help to solve the issue. But I don't know how to use BTW, currently, the master branch has an error when using |
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
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 <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
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 <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
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 <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
#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 <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
#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 <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
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 yh.wang@ibm.com
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build