build: fix coverage build #18409

yhwang · 2018-01-27T06:50:47Z

After adding the node_lib target in node.gyp, most of the node source
files are moved to that target. When coverage option is enabled,
corresponding cflags and ldflags are needed in that target as well.
gcovr also needs to check .gcda data for both node and node_lib.

Fixes: #18402

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

Checklist

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

Affected core subsystem(s)

addaleax

Thanks for taking care of this!

addaleax · 2018-01-28T00:21:34Z

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

joyeecheung · 2018-01-28T10:03:45Z

Makefile

@@ -191,6 +191,8 @@ coverage-test: coverage-build
 	$(RM) out/$(BUILDTYPE)/obj.target/node/gen/*.gcda
 	$(RM) out/$(BUILDTYPE)/obj.target/node/src/*.gcda
 	$(RM) out/$(BUILDTYPE)/obj.target/node/src/tracing/*.gcda
+	$(RM) out/$(BUILDTYPE)/obj.target/node_lib/{src,gen}/*.gcda


Can you write this as two commands so it would be accepted by GNU make 4.1? See #18332

done! thanks.

yhwang · 2018-01-29T17:46:57Z

updated the change based on @joyeecheung comment. Please kick off a new CI to verify it.

not sure about the failure of fedora24. Let's see if it fails again in new CI.

BridgeAR · 2018-01-31T00:25:09Z

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

yhwang · 2018-01-31T18:08:07Z

still got weird failures on debian8-x86 and fedora24. However, debian8-64 and other fedora jobs are green. And actually, this change doesn't change anything if you don't specify --coverage. When running CI, there is no --coverage option.....

yhwang · 2018-01-31T21:54:03Z

Seems the debian failure is related to performance. That should not be related to this PR. For those failures in fedora24, I am going to verify that locally. At the same time, I rebased my branch and Please kick off another CI to verify it.

mhdawson · 2018-01-31T22:42:19Z

Another CI run to see if the fedora24 failures re-occur: https://ci.nodejs.org/job/node-test-pull-request/12862/

mhdawson

LGTM provided CI run is green.

yhwang · 2018-02-01T00:24:29Z

The failures in debian8-64 happened in other PR and should be irrelevant.

BridgeAR · 2018-02-01T10:42:16Z

@yhwang would you be so kind and rebase? :-)

After adding the node_lib target in node.gyp, most of the node source files are moved to that target. When coverage option is enabled, corresponding cflags and ldflags are needed in that target as well. gcovr also needs to check .gcda data for both node and node_lib. Fixes: nodejs#18402 Signed-off-by: Yihong Wang <yh.wang@ibm.com>

yhwang · 2018-02-01T18:13:27Z

@BridgeAR rebase is done. Please kick off a CI.

mhdawson · 2018-02-01T19:39:56Z

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

yhwang · 2018-02-01T22:30:47Z

only failed in linux-arm. I guess this is good.

mhdawson · 2018-02-02T15:04:05Z

All ARM failures look like infra issues, and there were no failures on earlier CI runs so I think we are good. Landing.

mhdawson · 2018-02-02T15:08:37Z

Landed as a89d215

After adding the node_lib target in node.gyp, most of the node source files are moved to that target. When coverage option is enabled, corresponding cflags and ldflags are needed in that target as well. gcovr also needs to check .gcda data for both node and node_lib. PR-URL: #18409 Fixes: #18402 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

MylesBorins · 2018-02-21T00:09:08Z

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.

yhwang · 2018-02-21T18:43:33Z

@MylesBorins I guess code coverage is only for master build. right? if that's the case, then there is no need to backport.

After adding the node_lib target in node.gyp, most of the node source files are moved to that target. When coverage option is enabled, corresponding cflags and ldflags are needed in that target as well. gcovr also needs to check .gcda data for both node and node_lib. PR-URL: nodejs#18409 Fixes: nodejs#18402 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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

addaleax approved these changes Jan 28, 2018

View reviewed changes

joyeecheung reviewed Jan 28, 2018

View reviewed changes

jasnell approved these changes Jan 29, 2018

View reviewed changes

yhwang force-pushed the fix-coverage branch from 6ce936d to 6e99211 Compare January 29, 2018 17:23

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

joyeecheung approved these changes Jan 31, 2018

View reviewed changes

yhwang force-pushed the fix-coverage branch from 6e99211 to 4ada3f9 Compare January 31, 2018 21:52

mhdawson approved these changes Jan 31, 2018

View reviewed changes

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

yhwang force-pushed the fix-coverage branch from 4ada3f9 to f4e021c Compare February 1, 2018 17:41

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

mhdawson closed this Feb 2, 2018

yhwang deleted the fix-coverage branch February 2, 2018 17:12

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

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

gibfahn mentioned this pull request Feb 21, 2018

C++ coverage broken #18402

Closed

addaleax mentioned this pull request Feb 27, 2018

v9.7.0 proposal #19040

Closed

MylesBorins added the dont-land-on-v8.x label Aug 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: fix coverage build #18409

build: fix coverage build #18409

yhwang commented Jan 27, 2018

addaleax left a comment

addaleax commented Jan 28, 2018

joyeecheung Jan 28, 2018 •

edited

Loading

yhwang Jan 29, 2018

yhwang commented Jan 29, 2018

BridgeAR commented Jan 31, 2018

yhwang commented Jan 31, 2018

yhwang commented Jan 31, 2018 •

edited

Loading

mhdawson commented Jan 31, 2018 •

edited

Loading

mhdawson left a comment

yhwang commented Feb 1, 2018

BridgeAR commented Feb 1, 2018

yhwang commented Feb 1, 2018

mhdawson commented Feb 1, 2018

yhwang commented Feb 1, 2018

mhdawson commented Feb 2, 2018

mhdawson commented Feb 2, 2018

MylesBorins commented Feb 21, 2018

yhwang commented Feb 21, 2018

build: fix coverage build #18409

build: fix coverage build #18409

Conversation

yhwang commented Jan 27, 2018

Checklist

Affected core subsystem(s)

addaleax left a comment

Choose a reason for hiding this comment

addaleax commented Jan 28, 2018

joyeecheung Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

yhwang Jan 29, 2018

Choose a reason for hiding this comment

yhwang commented Jan 29, 2018

BridgeAR commented Jan 31, 2018

yhwang commented Jan 31, 2018

yhwang commented Jan 31, 2018 • edited Loading

mhdawson commented Jan 31, 2018 • edited Loading

mhdawson left a comment

Choose a reason for hiding this comment

yhwang commented Feb 1, 2018

BridgeAR commented Feb 1, 2018

yhwang commented Feb 1, 2018

mhdawson commented Feb 1, 2018

yhwang commented Feb 1, 2018

mhdawson commented Feb 2, 2018

mhdawson commented Feb 2, 2018

MylesBorins commented Feb 21, 2018

yhwang commented Feb 21, 2018

joyeecheung Jan 28, 2018 •

edited

Loading

yhwang commented Jan 31, 2018 •

edited

Loading

mhdawson commented Jan 31, 2018 •

edited

Loading