build: fix cctest build failure in Windows

[yhwang]

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]

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

[Trott]

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

[Trott]

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

[refack]

LGTM, but have some stuff to discuss

[refack]

Why 2 levels?

[refack]

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.

[yhwang]

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.

[yhwang]

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

[refack]

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

[refack]

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

[yhwang]

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

[refack]

Let's go with obj_inspetor_path

[yhwang]

sure!

[refack]

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

[yhwang]

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

[refack]

Why can't we merge these lists?

[yhwang]

merge? can you elaborate it more?

[yhwang]

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

[yhwang]

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

[refack]

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)']

[yhwang]

I see. I can merge them.

[yhwang]

@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]

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]

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]

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]

@yhwang What’s the status here?

[yhwang]

@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]

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

[yhwang]

@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]

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]

Ping... any updates on this one?

[yhwang]

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

[refack]

I'm fine with the current workaround...

this review is outdated

[yhwang]

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

[yhwang]

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

[yhwang]

CI is green

[addaleax]

Landed in 29cf335