tools: fix c++ code coverage on macOS #24004

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

tools/gyp/pylib/gyp/xcode_emulation.py

@@ -949,6 +949,8 @@ def GetLibtoolflags(self, configname):
    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 = configname libtoolflags = self._Settings().get('OTHER_LDFLAGS', []) # Native macOS libtool does not except any flags with '--' prefix libtoolflags = [f for f in libtoolflags if not f.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

tools/gyp/pylib/gyp/xcode_emulation.py

@@ -949,6 +949,8 @@ def GetLibtoolflags(self, configname):
    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 = configname libtoolflags = self._Settings().get('OTHER_LDFLAGS', []) # Native macOS libtool does not except any flags with '--' prefix libtoolflags = [f for f in libtoolflags if not f.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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: fix c++ code coverage on macOS #24004

tools: fix c++ code coverage on macOS #24004

evanlucas commented Oct 31, 2018

nodejs-github-bot commented Oct 31, 2018

richardlau commented Oct 31, 2018

evanlucas commented Oct 31, 2018

mhdawson left a comment

refack Oct 31, 2018 •

edited

Loading

refack Oct 31, 2018

refack left a comment

refack commented Oct 31, 2018

refack Oct 31, 2018

refack commented Oct 31, 2018

refack commented Oct 31, 2018

antsmartian commented Nov 3, 2018

Trott commented Nov 4, 2018

antsmartian commented Nov 4, 2018 •

edited

Loading

cjihrig commented Nov 4, 2018

richardlau commented Nov 4, 2018

Trott commented Nov 18, 2018

refack commented Nov 20, 2018

Trott commented Dec 1, 2018

tools: fix c++ code coverage on macOS #24004

tools: fix c++ code coverage on macOS #24004

Conversation

evanlucas commented Oct 31, 2018

Checklist

nodejs-github-bot commented Oct 31, 2018

richardlau commented Oct 31, 2018

evanlucas commented Oct 31, 2018

mhdawson left a comment

Choose a reason for hiding this comment

refack Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

refack Oct 31, 2018

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

refack commented Oct 31, 2018

refack Oct 31, 2018

Choose a reason for hiding this comment

refack commented Oct 31, 2018

refack commented Oct 31, 2018

antsmartian commented Nov 3, 2018

Trott commented Nov 4, 2018

antsmartian commented Nov 4, 2018 • edited Loading

cjihrig commented Nov 4, 2018

richardlau commented Nov 4, 2018

Trott commented Nov 18, 2018

refack commented Nov 20, 2018

Trott commented Dec 1, 2018

refack Oct 31, 2018 •

edited

Loading

antsmartian commented Nov 4, 2018 •

edited

Loading