build: add node_lib_target_name to cctest deps

[danbev]

Currently the cctest target depend on the node_core_target_name
target. But it is the node_lib_target_name target that compiles the
sources now which means that if a source file in src is updated the
cctest executable will not be re-linked against it, but will remain
unchanged. The code will still be compiled, just not linked which
means that if you are debugging you'll not see the changes and also a
warning will be displayed about this issue.

This commit changes the cctest target to depend on node_lib_target_name.

Checklist

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

Affected core subsystem(s)

build, test

[danbev]

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

[danbev]

Another try: https://ci.nodejs.org/job/node-test-pull-request/12948/

[danbev]

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

[danbev]

Closed the wrong PR 😞

[BridgeAR]

@nodejs/build PTAL

[danbev]

@nodejs/build Don't bother looking into this yet. I don't think this will work as is. Let me come up with something better and I'll ping the group.

[danbev]

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

[danbev]

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

[danbev]

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

[danbev]

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

[danbev]

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

[danbev]

@nodejs/build Would be great to get some eyes on this and see if this is acceptable to merge.

Note sure if I was clear enough in the description but as it is at the moment, if an update is made to a source file and then make -j8 cctest is executed the change will be compiled, but the cctest executable will not be re-linked which means the change will not tested, and if a debugging session is started a warning similar to the following will be displayed:

(lldb) br s -f node.cc -l 4295                                                                                     
error: cctest debug map object file '/node/out/Debug/obj.target/node_lib/src/node.o' has changed (actual time is 0x5a86ccc5, debug map time is 0x5a86cc0c) since this executable was linked, file will be ignored

[Trott]

/cc @nodejs/node-gyp

[gibfahn]

Currently the cctest target depend on the node_core_target_name
target. But it is the node_lib_target_name target that compiles the
sources now

Is this the result of a recent change? Would be good to know which PR it depends on for backporting reasons.

[yhwang]

Hi @gibfahn

Is this the result of a recent change? Would be good to know which PR it depends on for backporting reasons.

I added node_lib_target_name target in this change:
f878f94
And I also tried to modify cctest to depends on node_lib_target_name and failed at that moment. @danbev is right, putting cctest to depend on node_core_target_name would cause the issue. I didn't think about that and that's a good catch. However, I forgot why I failed to put cctest to depend on node_lib_target_name. I will try to figure it out and update it later. (I hope this change fix the issue that I hit before)

[Edit]
I think @danbev fixed the issue I hit before in test_node_postmortem_metadata.cc. Nice! These does remove a lots of duplications. 👍

[yhwang]

LGTM

[danbev]

Are we good to land this, asking because I noticed that there is an awaiting review request from @bnoordhuis? (I don't recall doing that but it's possible that I did by mistake, not that I don't welcome a review)

[danbev]

Landed in 30f89df.

[MylesBorins]

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.