build: fix cctest build failure in Windows #21228

yhwang · 2018-06-09T06:32:23Z

cctest needs to access some internal APIs in node core. Putting
node_lib_target_name as dependency would causes linking error when
node_lib is built as shared lib in Windows. The reason being is
those internal APIs don't have __declspec(dllexport). For cctest
we still need to specify individual obj files and link them instead
of node_lib. In Windows, changes of the dependencies in libraries
trigger the cctest to rebuild. However, in Linux platforms, it
doesn't work. Instead, need to put obj dependencies into sources.

Signed-off-by: Yihong Wang yh.wang@ibm.com

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

yhwang · 2018-06-09T06:33:13Z

I've verified the change on Windows 2016 and Ubuntu 16.04.

Trott · 2018-06-09T12:26:17Z

@nodejs/platform-windows @nodejs/build @nodejs/testing

Trott · 2018-06-09T12:26:59Z

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

refack

LGTM, but have some stuff to discuss

refack · 2018-06-10T21:48:36Z

node.gyp

@@ -920,6 +920,143 @@
        'node_dtrace_provider',
      ],

+      'variables': {


Why 2 levels?

Is it because of condition evaluation order.
In that case it's also possible to > or "late variable expansion operator", or just put a comment.

because the only variables we need in cctest target are var_defines and dep_objs. Other variables are just intermediate. I can flatten them if variable scope is minor.

@refack after trying to flatten the variables, I need to use > for late variable expansion and I found it's easy to get confused. I'd prefer 2 level variable and I will put a comment there. how do you think?

👍
Comment is totally reasonable, but IMHO it's needed since it's a non-trivial pattern.

refack · 2018-06-10T21:49:46Z

node.gyp

+        ],
+        'conditions': [
+          [ 'node_use_openssl=="true"', {
+            'var_defines+': [ 'HAVE_OPENSSL=1' ],


So GYP has default list appending semantics so no need for the + suffix
+ just means "prepend"

I ran into some issues when I didn't use + sign in other place. In this case, I can remove it.

refack · 2018-06-10T21:50:21Z

node.gyp

+        'variables': {
+          'obj_path': '<(obj_dir)/<(node_lib_target_name)/src',
+          'obj_gen_path': '<(obj_dir)/<(node_lib_target_name)/gen',
+          'obj_insp_path': '<(obj_dir)/<(node_lib_target_name)/src/inspector',


Let's go with obj_inspetor_path

refack · 2018-06-10T21:57:38Z

node.gyp

+            'dep_objs+': [
+              '<(obj_path)<(obj_separator)node_crypto.<(obj_suffix)',
+              '<(obj_path)<(obj_separator)node_crypto_bio.<(obj_suffix)',
+              '<(obj_path)<(obj_separator)'


IMHO this could be kept on one line (it's not the only >80 line anyway). But others may disagree.

I realized there are other >80 lines. however, let me follow the convention :-).

refack · 2018-06-10T22:00:20Z

node.gyp

-          ],
-          'defines': [
-            'HAVE_INSPECTOR=1',
+            'test/cctest/test_inspector_socket_server.cc',


Why can't we merge these lists?

merge? can you elaborate it more?

are you talking about merging this into variables? I don't know sources is available inside variables. If yes, I will merge them.

@refack I can't access sources inside variables. then what do you mean by merge?

It's just that having the condition in multiple point makes it hard to grok.

So maybe define a conditionally filled variable

'node_inspector_sources' : [], ... ['v8_enable_inspector==1', { 'node_inspector_sources' : [ 'test/cctest/test_inspector_socket.cc', 'test/cctest/test_inspector_socket_server.cc', '<@(node_inspector_generated_sources)', ]

Then here (or anywhere else where sources if appended) just have an unconditional

'sources': [ '>@(node_inspector_sources)']

I see. I can merge them.

@refack I found gyp throws an error when the variable is an empty array (./configure --without-inspector). Therefore, I need to preload the sources in cctest target into that new variable: var_sources. Same as var_defines.

refack · 2018-06-10T22:03:33Z

Looks good in general, left some comments and questions.
(should also CC nodejs/gyp, but it's mostly overlapped anyway)

Resume CI: https://ci.nodejs.org/job/node-test-commit/19141/

yhwang · 2018-06-18T19:26:31Z

rebase the code and new CI: https://ci.nodejs.org/job/node-test-commit/19311/

bnoordhuis

This PR basically reverts 30f89df from a few months ago and (re)introduces a lot of duplication.

cctest needs to access some internal APIs in node core.

I'd say a better way forward is to identify those internal APIs and decide on a case-by-case basis what to do with them. Can you list them?

I could also live with disabling cctest for shared library builds; the static build gives good enough coverage.

bnoordhuis · 2018-06-22T08:29:44Z

I could also live with disabling cctest for shared library builds

Another solution: introduce a NODE_PRIVATE_EXTERN macro that turns on __declspec(dllexport) in shared library builds and __declspec(dllimport) in cctest.

addaleax · 2018-07-18T14:11:46Z

@yhwang What’s the status here?

yhwang · 2018-07-19T08:52:16Z

@addaleax sorry for the lack of activity for this one. I was not able to have time handle this last month.

Based on @bnoordhuis 's comment. I will disable the cctest in Windows when building shared lib. For those internal APIs that are needed by cctest, I think introducing a NODE_PRIVATE_EXTERN macro still can't resolve the issue. In that case, we will need to compile the shared lib 2 times, with and without the macro. If we do believe the static lib already has good coverage, then there is no need to introduce the change to compile the shared lib differently in Windows, especially 2 times with different macros.

addaleax · 2018-07-19T10:00:49Z

@yhwang thanks for the update, makes sense to me!

cctest depends on some internal APIs which don't declare `__declspec(dllexport)` and causes build failure when building node as shared lib on Windows. Since we already have good test coverage in static lib, we decide to skip the cctest in shared lib build on Windows. Signed-off-by: Yihong Wang <yh.wang@ibm.com>

yhwang · 2018-07-24T20:50:20Z

@bnoordhuis @refack I completely removed my original change and just skip cctest target in the new change based on the feedback. Please review the change again. Thanks.

yhwang · 2018-08-01T22:37:57Z

ping @bnoordhuis @refack @danbev . for now, I just disable the cctest on Windows when building shared lib. Let's discuss re-enabling it in another issue if needed.

jasnell · 2018-09-10T18:19:35Z

Ping... any updates on this one?

yhwang · 2018-09-11T00:19:02Z

I updated the change based on the comments and wait for review/comment. after the update, I haven't received any feedback yet.

refack · 2018-09-11T01:48:16Z

I'm fine with the current workaround...

this review is outdated

yhwang · 2018-09-13T16:23:52Z

@bnoordhuis 's review is for old change. let me dismiss his old review

yhwang · 2018-09-13T16:26:34Z

CI: https://ci.nodejs.org/job/node-test-commit/21429/

yhwang · 2018-09-13T18:37:37Z

CI is green

addaleax · 2018-09-17T15:32:54Z

Landed in 29cf335

cctest depends on some internal APIs which don't declare `__declspec(dllexport)` and causes build failure when building node as shared lib on Windows. Since we already have good test coverage in static lib, we decide to skip the cctest in shared lib build on Windows. Signed-off-by: Yihong Wang <yh.wang@ibm.com> PR-URL: #21228 Reviewed-By: Refael Ackermann <refack@gmail.com>

cctest depends on some internal APIs which don't declare `__declspec(dllexport)` and causes build failure when building node as shared lib on Windows. Since we already have good test coverage in static lib, we decide to skip the cctest in shared lib build on Windows. Signed-off-by: Yihong Wang <yh.wang@ibm.com> PR-URL: nodejs#21228 Reviewed-By: Refael Ackermann <refack@gmail.com>

cctest depends on some internal APIs which don't declare `__declspec(dllexport)` and causes build failure when building node as shared lib on Windows. Since we already have good test coverage in static lib, we decide to skip the cctest in shared lib build on Windows. Signed-off-by: Yihong Wang <yh.wang@ibm.com> Backport-PR-URL: #25758 PR-URL: #21228 Reviewed-By: Refael Ackermann <refack@gmail.com>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jun 9, 2018

yhwang requested a review from danbev June 9, 2018 06:32

jasnell requested review from refack and bnoordhuis June 10, 2018 20:46

refack suggested changes Jun 10, 2018

View reviewed changes

yhwang force-pushed the fix-windows-undefined-symbols branch 2 times, most recently from b2a4611 to 13d7c48 Compare June 12, 2018 17:16

yhwang added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 18, 2018

yhwang force-pushed the fix-windows-undefined-symbols branch from 13d7c48 to 748464b Compare June 18, 2018 18:01

bnoordhuis previously requested changes Jun 22, 2018

View reviewed changes

apapirovski removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 25, 2018

yhwang force-pushed the fix-windows-undefined-symbols branch from 748464b to 0ec2e1d Compare July 24, 2018 20:46

jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018

refack approved these changes Sep 11, 2018

View reviewed changes

yhwang added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 13, 2018

addaleax closed this Sep 17, 2018

targos mentioned this pull request Sep 18, 2018

Release proposal: v10.11.0 #22932

Merged

yhwang deleted the fix-windows-undefined-symbols branch November 30, 2018 19:00

BethGriggs mentioned this pull request Jan 28, 2019

[v8.x-backport] build: skip cctest on Windows shared lib build #25758

Closed

2 tasks

MylesBorins mentioned this pull request Mar 26, 2019

v8.16.0 proposal #26933

Merged

build: fix cctest build failure in Windows #21228

build: fix cctest build failure in Windows #21228

Conversation

yhwang commented Jun 9, 2018

Checklist

yhwang commented Jun 9, 2018

Trott commented Jun 9, 2018

Trott commented Jun 9, 2018

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhwang Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhwang Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

refack commented Jun 10, 2018 • edited Loading

yhwang commented Jun 18, 2018

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis commented Jun 22, 2018

addaleax commented Jul 18, 2018

yhwang commented Jul 19, 2018

addaleax commented Jul 19, 2018

yhwang commented Jul 24, 2018

yhwang commented Aug 1, 2018

jasnell commented Sep 10, 2018

yhwang commented Sep 11, 2018

refack commented Sep 11, 2018

yhwang commented Sep 13, 2018

yhwang commented Sep 13, 2018

yhwang commented Sep 13, 2018

addaleax commented Sep 17, 2018

yhwang Jun 11, 2018 •

edited

Loading

yhwang Jun 12, 2018 •

edited

Loading

refack commented Jun 10, 2018 •

edited

Loading