build: add c++ coverage support on macOS #16163

evanlucas · 2017-10-12T14:17:14Z

macOS requires passing the --coverage flag in OTHER_LDFLAGS and
OTHER_CFLAGS in xcode_settings.

Checklist

commit message follows commit guidelines

Affected core subsystem(s)

build

jasnell

rubber stampy LGTM. Looked it over, didn't see anything that was problematic, but cannot test on macos at the moment.

bnoordhuis · 2017-10-13T10:38:10Z

Makefile

@@ -34,6 +35,10 @@ ifdef DISABLE_V8_I18N
  V8_BUILD_OPTIONS += i18nsupport=off
 endif

+ifeq ($(OSTYPE), darwin)
+	GCOV = xcrun llvm-cov gcov


Tiniest of nits: two space indent like the lines above.

joyeecheung · 2017-10-13T12:07:20Z

I am still seeing 0% C++ Coverage with this patch on MacOS 10.12.4...

evanlucas · 2017-10-13T12:10:58Z

@joyeecheung That was happening to me before too. I had to completely remove the out/ directory after running make clean. I also had to remove the use of ccache for it to work properly. This is working for me on 10.12.6. Thanks for checking!

cjihrig

Nice work

gireeshpunathil

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

mhdawson

LGTM, good to see this get added.

evanlucas · 2017-10-18T13:02:57Z

ping @joyeecheung, did you get a chance to try this again?

joyeecheung · 2017-10-18T16:09:54Z

@evanlucas I have tried before with make clean and rm -rf out, but it's still 0%. I have upgraded my system after, let me try again...

evanlucas · 2017-10-18T16:10:38Z

@joyeecheung are you using ccache by chance?

joyeecheung · 2017-10-18T16:40:03Z

@evanlucas No, I don't have ccache set up. I tried again but still no luck :( Maybe this has something to do with the version of llvm toolchain that I am using?

> llvm-cov --version
LLVM (http://llvm.org/):
  LLVM version 5.0.0
  Optimized build.
  Default target: x86_64-apple-darwin16.7.0
  Host CPU: haswell

evanlucas · 2017-10-18T16:43:04Z

@joyeecheung it very well could:

$ xcrun llvm-cov --version                                                                                                                                                                                               
Apple LLVM version 9.0.0 (clang-900.0.35)
  Optimized build.
  Default target: x86_64-apple-darwin16.7.0
  Host CPU: broadwell

Ah, are you using Xcode's cli tools or did you manually install everything?

joyeecheung · 2017-10-18T17:13:26Z

@evanlucas Ah, I seemed to forget to rerun ./configure --coverage. It is working now! Thanks!

-n C++ coverage %:
89.6%

evanlucas · 2017-10-18T17:14:38Z

@joyeecheung awesome thanks!

I'll land today then! Thanks!

macOS requires passing the --coverage flag in OTHER_LDFLAGS and OTHER_CFLAGS in xcode_settings. PR-URL: nodejs#16163 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

evanlucas · 2017-10-18T19:06:16Z

Landed in a078584. Thanks!

macOS requires passing the --coverage flag in OTHER_LDFLAGS and OTHER_CFLAGS in xcode_settings. PR-URL: #16163 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

macOS requires passing the --coverage flag in OTHER_LDFLAGS and OTHER_CFLAGS in xcode_settings. PR-URL: nodejs/node#16163 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

MylesBorins · 2017-11-16T20:28:09Z

Should this be backported to v6.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.

evanlucas · 2017-11-17T21:50:00Z

Not really necessary IMO, so I'll add the label

macOS requires passing the --coverage flag in OTHER_LDFLAGS and OTHER_CFLAGS in xcode_settings. PR-URL: nodejs/node#16163 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 12, 2017

jasnell approved these changes Oct 13, 2017

View reviewed changes

bnoordhuis approved these changes Oct 13, 2017

View reviewed changes

gibfahn approved these changes Oct 13, 2017

View reviewed changes

evanlucas force-pushed the macos-src-coverage branch from acb5cb3 to d261d72 Compare October 13, 2017 17:07

cjihrig approved these changes Oct 15, 2017

View reviewed changes

gireeshpunathil approved these changes Oct 16, 2017

View reviewed changes

mhdawson approved these changes Oct 16, 2017

View reviewed changes

XadillaX approved these changes Oct 18, 2017

View reviewed changes

joyeecheung approved these changes Oct 18, 2017

View reviewed changes

evanlucas force-pushed the macos-src-coverage branch from d261d72 to a078584 Compare October 18, 2017 18:58

evanlucas closed this Oct 18, 2017

evanlucas deleted the macos-src-coverage branch October 18, 2017 18:58

evanlucas merged commit a078584 into nodejs:master Oct 18, 2017

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

evanlucas added dont-land-on-v6.x and removed backport-requested-v6.x labels Nov 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add c++ coverage support on macOS #16163

build: add c++ coverage support on macOS #16163

evanlucas commented Oct 12, 2017

jasnell left a comment

bnoordhuis Oct 13, 2017

joyeecheung commented Oct 13, 2017

evanlucas commented Oct 13, 2017

cjihrig left a comment

gireeshpunathil left a comment

mhdawson left a comment

evanlucas commented Oct 18, 2017

joyeecheung commented Oct 18, 2017 •

edited

Loading

evanlucas commented Oct 18, 2017

joyeecheung commented Oct 18, 2017

evanlucas commented Oct 18, 2017

joyeecheung commented Oct 18, 2017

evanlucas commented Oct 18, 2017

evanlucas commented Oct 18, 2017

MylesBorins commented Nov 16, 2017

evanlucas commented Nov 17, 2017

build: add c++ coverage support on macOS #16163

build: add c++ coverage support on macOS #16163

Conversation

evanlucas commented Oct 12, 2017

Checklist

Affected core subsystem(s)

jasnell left a comment

Choose a reason for hiding this comment

bnoordhuis Oct 13, 2017

Choose a reason for hiding this comment

joyeecheung commented Oct 13, 2017

evanlucas commented Oct 13, 2017

cjihrig left a comment

Choose a reason for hiding this comment

gireeshpunathil left a comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

evanlucas commented Oct 18, 2017

joyeecheung commented Oct 18, 2017 • edited Loading

evanlucas commented Oct 18, 2017

joyeecheung commented Oct 18, 2017

evanlucas commented Oct 18, 2017

joyeecheung commented Oct 18, 2017

evanlucas commented Oct 18, 2017

evanlucas commented Oct 18, 2017

MylesBorins commented Nov 16, 2017

evanlucas commented Nov 17, 2017

joyeecheung commented Oct 18, 2017 •

edited

Loading