tools: fix c++ code coverage on macOS by evanlucas · Pull Request #24004 · nodejs/node

evanlucas · 2018-10-31T19:11:55Z

Recent libtool on macOS does not support the --coverage flag that was
being passed through in the final linking stage.
This change patches gyp-mac-tool to not pass the --coverage flag through
to libtool. It is now possible to generate code coverage for src/ on macOS.

Fixes: #19057

Checklist

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

Recent libtool on macOS does not support the --coverage flag that was being passed through in the final linking stage. This change patches gyp-mac-tool to not pass the --coverage flag through to libtool. It is now possible to generate code coverage for src/ on macOS. Fixes: nodejs#19057

nodejs-github-bot · 2018-10-31T19:11:57Z

@evanlucas build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1444/pipeline

richardlau · 2018-10-31T19:36:46Z

Can we not remove --coverage from ldflags/OTHER_LDFLAGS on macOS instead, or would that also break coverage?

node/node.gypi

Lines 272 to 291 in c515e5c

 [ 'OS in "mac freebsd linux" and node_shared=="false"'
' and coverage=="true"', { 
'ldflags': [ '--coverage', 
'-g', 
'-O0' ], 
'cflags': [ '--coverage', 
'-g', 
'-O0' ], 
'cflags!': [ '-O3' ], 
'xcode_settings': { 
'OTHER_LDFLAGS': [ 
'--coverage', 
 ], 
'OTHER_CFLAGS+': [ 
'--coverage', 
'-g', 
'-O0'
 ], 
 } 
 }], 

evanlucas · 2018-10-31T19:47:17Z

@richardlau that broke coverage when I tried it

mhdawson

LGTM

refack · 2018-10-31T20:59:05Z

 libtoolflags = []

 for libtoolflag in self._Settings().get('OTHER_LDFLAGS', []):
+ if libtoolflag == "--coverage":


Ok after some digging is seem like the native libtool does not accept any flag that starts with --
Can I suggest this instead of the whole for expression (From L948 to L955)
self.configname=confignamelibtoolflags=self._Settings().get('OTHER_LDFLAGS', []) # Native macOS libtool does not except any flags with '--' prefixlibtoolflags= [fforfinlibtoolflagsifnotf.startswith('--')] # TODO(thakis): ARCHS?

refack

IMHO there should be a more direct way.

refack · 2018-10-31T21:01:18Z

Hello @evanlucas I'm going to look into solving this by a more direct means. If I can't I'll dismiss my review.

/CC @nodejs/python @nodejs/gyp

refack · 2018-10-31T21:50:07Z

 libtoolflags = []

 for libtoolflag in self._Settings().get('OTHER_LDFLAGS', []):
+ if libtoolflag == "--coverage":


Ok after some digging is seem like the native libtool does not accept any flag that starts with --
Can I suggest this instead of the whole for expression (From L948 to L955)
self.configname=confignamelibtoolflags=self._Settings().get('OTHER_LDFLAGS', []) # Native macOS libtool does not except any flags with '--' prefixlibtoolflags= [fforfinlibtoolflagsifnotf.startswith('--')] # TODO(thakis): ARCHS?

refack · 2018-10-31T21:51:49Z

I've tested my suggestion and it generate the same make scaffolding as the current PR. And allows building make coverage.

refack · 2018-10-31T23:43:37Z

there should be a more direct way.

I've been thinking about this some more, and I think the settings in

node/node.gypi

Lines 272 to 291 in c515e5c

 [ 'OS in "mac freebsd linux" and node_shared=="false"'
' and coverage=="true"', { 
'ldflags': [ '--coverage', 
'-g', 
'-O0' ], 
'cflags': [ '--coverage', 
'-g', 
'-O0' ], 
'cflags!': [ '-O3' ], 
'xcode_settings': { 
'OTHER_LDFLAGS': [ 
'--coverage', 
 ], 
'OTHER_CFLAGS+': [ 
'--coverage', 
'-g', 
'-O0'
 ], 
 } 
 }], 

are wrong. They should only be applied to the binary target, not to the libnode.lib indermidiary. I'll work on that tomorrow...

antsmartian · 2018-11-03T06:33:05Z

Thanks. I added your change and tried running make coverage, I end up in seeing the below error:

mkdir -p coverage .cov_tmp
./node ./node_modules/.bin/nyc merge 'out/Release/.coverage' \
.cov_tmp/libcov.json
coverage files in out/Release/.coverage merged into .cov_tmp/libcov.json
(cd lib && ../node ../node_modules/.bin/nyc report \
--temp-directory "/Users/anto/programs/node/node/.cov_tmp" \
--report-dir "/Users/anto/programs/node/node/coverage")
/Users/anto/programs/node/node/node_modules/nyc/node_modules/yargs/yargs.js:1133
else throw err
^
Error: ENOENT: no such file or directory, scandir '/Users/anto/programs/node/node/lib/.nyc_output'
at Object.readdirSync (fs.js:22200:3)
at NYC.eachReport (/Users/anto/programs/node/node/node_modules/nyc/index.js:525:31)
at NYC.getCoverageMapFromAllCoverageFiles (/Users/anto/programs/node/node/node_modules/nyc/index.js:424:8)
at NYC.report (/Users/anto/programs/node/node/node_modules/nyc/index.js:442:18)
at Object.exports.handler (/Users/anto/programs/node/node/node_modules/nyc/lib/commands/report.js:48:7)
at Object.runCommand (/Users/anto/programs/node/node/node_modules/nyc/node_modules/yargs/lib/command.js:235:44)
at Object.parseArgs [as _parseArgs] (/Users/anto/programs/node/node/node_modules/nyc/node_modules/yargs/yargs.js:1046:30)
at Object.parse (/Users/anto/programs/node/node/node_modules/nyc/node_modules/yargs/yargs.js:551:25)
at Object.<anonymous> (/Users/anto/programs/node/node/node_modules/nyc/bin/nyc.js:24:35)
at Module._compile (internal/modules/cjs/loader.js:10564:30)

I'm on Mojave 0.14. I even tried cleaning up coverage and run several times, but ended up in the same issue above. Anything I'm missing?

Trott · 2018-11-04T02:31:29Z

Thanks. I added your change and tried running make coverage, I end up in seeing the below error:

@antsmartian Do you mean @refack's change or @cjihrig's changes in this PR or something else?

antsmartian · 2018-11-04T02:52:38Z

@Trott I have added the changes from this PR:

 if libtoolflag == "--coverage":
continue

cjihrig · 2018-11-04T14:36:29Z

or @cjihrig's changes in this PR

I assume this was supposed to be @evanlucas, but please let me know if I missed something.

richardlau · 2018-11-04T19:26:00Z

@antsmartian Maybe check what version of nyc is being used? The error looks similar to #23690 and #23769 pinned the npm install to nyc@13 (but the npm install is only done if node_modules/nyc doesn't exist).

Trott · 2018-11-18T04:54:53Z

@nodejs/platform-macos @evanlucas @refack Any chance someone can get this unstuck? Seems like it's close....

refack · 2018-11-20T16:29:16Z

Alternative PR: #24520
(figuring out how to test)

Trott · 2018-12-01T15:27:11Z

Since #24520 landed, I assume this can be closed. But if I'm wrong about that, please re-open!

nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Oct 31, 2018

addaleax approved these changes Oct 31, 2018
View reviewed changes

mhdawson approved these changes Oct 31, 2018
View reviewed changes

refack reviewed Oct 31, 2018
View reviewed changes

refack suggested changes Oct 31, 2018
View reviewed changes

ryzokuken approved these changes Oct 31, 2018
View reviewed changes

refack suggested changes Oct 31, 2018
View reviewed changes

refack added the python PRs and issues that require attention from people who are familiar with Python. label Oct 31, 2018

Trott added wip Issues and PRs that are still a work in progress. and removed wip Issues and PRs that are still a work in progress. labels Nov 4, 2018

refack mentioned this pull request Nov 20, 2018
build: fix c++ code coverage on macOS #24520
Merged
2 tasks

Trott closed this Dec 1, 2018

Uh oh!

Uh oh!

Conversation

evanlucas commented Oct 31, 2018

Checklist

Uh oh!

nodejs-github-bot commented Oct 31, 2018

Uh oh!

richardlau commented Oct 31, 2018

Uh oh!

evanlucas commented Oct 31, 2018

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

refackOct 31, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refackOct 31, 2018

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Oct 31, 2018

Uh oh!

refackOct 31, 2018

Choose a reason for hiding this comment

Uh oh!

refack commented Oct 31, 2018

Uh oh!

refack commented Oct 31, 2018

Uh oh!

antsmartian commented Nov 3, 2018

Uh oh!

Trott commented Nov 4, 2018

Uh oh!

antsmartian commented Nov 4, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Nov 4, 2018

Uh oh!

richardlau commented Nov 4, 2018

Uh oh!

Trott commented Nov 18, 2018

Uh oh!

refack commented Nov 20, 2018

Uh oh!

Trott commented Dec 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

refackOct 31, 2018•
edited
Loading

antsmartian commented Nov 4, 2018•
edited
Loading