build: add node_lib_target_name to cctest deps #18576

danbev · 2018-02-05T14:22:54Z

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 · 2018-02-05T14:23:21Z

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

danbev · 2018-02-05T16:48:53Z

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

danbev · 2018-02-06T06:42:42Z

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

danbev · 2018-02-06T07:17:45Z

Closed the wrong PR 😞

BridgeAR · 2018-02-06T21:17:53Z

@nodejs/build PTAL

danbev · 2018-02-07T04:54:57Z

@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 · 2018-02-13T12:08:05Z

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

danbev · 2018-02-14T04:54:21Z

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

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.

danbev · 2018-02-14T08:12:45Z

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

danbev · 2018-02-14T10:25:05Z

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

It seems that these are not linked properly on Windows with out this.

danbev · 2018-02-16T11:29:52Z

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

danbev · 2018-02-16T12:32:20Z

@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 · 2018-02-16T14:44:49Z

/cc @nodejs/node-gyp

gibfahn · 2018-02-16T21:20:47Z

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 · 2018-02-16T21:53:49Z

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 · 2018-02-19T06:56:07Z

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 · 2018-02-19T11:34:18Z

Landed in 30f89df.

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. PR-URL: #18576 Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins · 2018-02-21T15:13:20Z

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.

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. PR-URL: nodejs#18576 Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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. Backport-PR-URL: #18550 PR-URL: #18576 Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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. PR-URL: nodejs#18576 Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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. PR-URL: nodejs#20797 Original PR-URL: nodejs#18576 Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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. Backport-PR-URL: #20797 PR-URL: #18576 Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added the build label Feb 5, 2018

danbev force-pushed the fix_cctest_target_dependency branch from 3908cb7 to be95268 Compare Feb 6, 2018

danbev closed this Feb 6, 2018

danbev reopened this Feb 6, 2018

danbev added the wip label Feb 7, 2018

danbev force-pushed the fix_cctest_target_dependency branch from be95268 to b6ce878 Compare Feb 13, 2018

danbev force-pushed the fix_cctest_target_dependency branch from 3a26ecd to a4565e2 Compare Feb 14, 2018

squash: add extern declarations for constants

9fc08af

danbev force-pushed the fix_cctest_target_dependency branch from a4565e2 to 9fc08af Compare Feb 14, 2018

squash: re-add rename_node_bin_win to cctest deps

15051d3

danbev force-pushed the fix_cctest_target_dependency branch from f750f20 to 15051d3 Compare Feb 15, 2018

squash: add extern "C" to global vars

d76100c

It seems that these are not linked properly on Windows with out this.

danbev removed the wip label Feb 16, 2018

Trott requested review from mmarchini and removed request for mmarchini Feb 16, 2018

mmarchini approved these changes Feb 16, 2018

View changes

gibfahn requested a review from yhwang Feb 16, 2018

yhwang approved these changes Feb 16, 2018

View changes

gibfahn approved these changes Feb 18, 2018

View changes

BridgeAR added the author ready label Feb 19, 2018

bnoordhuis approved these changes Feb 19, 2018

View changes

danbev closed this Feb 19, 2018

danbev deleted the fix_cctest_target_dependency branch Feb 19, 2018

MylesBorins added the backport-requested-v9.x label Feb 21, 2018

danbev mentioned this pull request Feb 23, 2018

[v9.x backport] build: add node_lib_target_name to cctest deps #18954

Closed

2 tasks

addaleax mentioned this pull request Feb 27, 2018

v9.7.0 proposal #19040

Closed

yhwang mentioned this pull request May 17, 2018

won't link on OS X with --enable-static #20763

Closed

yhwang mentioned this pull request May 17, 2018

[v8.x backport] 16238 and 18576 #20797

Closed

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

nodejs / node Public

build: add node_lib_target_name to cctest deps #18576

build: add node_lib_target_name to cctest deps #18576

danbev commented Feb 5, 2018

danbev commented Feb 5, 2018

danbev commented Feb 5, 2018

danbev commented Feb 6, 2018

danbev commented Feb 6, 2018

BridgeAR commented Feb 6, 2018

danbev commented Feb 7, 2018

danbev commented Feb 13, 2018

danbev commented Feb 14, 2018

danbev commented Feb 14, 2018

danbev commented Feb 14, 2018

danbev commented Feb 16, 2018

danbev commented Feb 16, 2018

Trott commented Feb 16, 2018

gibfahn commented Feb 16, 2018

yhwang commented Feb 16, 2018 •

edited

yhwang left a comment

danbev commented Feb 19, 2018

danbev commented Feb 19, 2018

MylesBorins commented Feb 21, 2018

nodejs / node Public

build: add node_lib_target_name to cctest deps #18576

build: add node_lib_target_name to cctest deps #18576

Conversation

danbev commented Feb 5, 2018

Checklist

Affected core subsystem(s)

danbev commented Feb 5, 2018

danbev commented Feb 5, 2018

danbev commented Feb 6, 2018

danbev commented Feb 6, 2018

BridgeAR commented Feb 6, 2018

danbev commented Feb 7, 2018

danbev commented Feb 13, 2018

danbev commented Feb 14, 2018

danbev commented Feb 14, 2018

danbev commented Feb 14, 2018

danbev commented Feb 16, 2018

danbev commented Feb 16, 2018

Trott commented Feb 16, 2018

gibfahn commented Feb 16, 2018

yhwang commented Feb 16, 2018 • edited

yhwang left a comment

danbev commented Feb 19, 2018

danbev commented Feb 19, 2018

MylesBorins commented Feb 21, 2018

yhwang commented Feb 16, 2018 •

edited