tools: fix run-valgrind.py script by bnoordhuis · Pull Request #9520 · nodejs/node

bnoordhuis · 2016-11-08T21:11:37Z

addaleax

LGTM

mhdawson

LGTM

thefourtheye · 2016-11-09T02:20:06Z

I think it's better to do this in a separate python file (tools/run_valgrind.py) so that we don't have to float this patch everytime after upgrading V8

bnoordhuis · 2016-11-09T08:58:00Z

Crap, I missed that tools/run-valgrind.py is a symlink to deps/v8/tools/run-valgrind.py. I'll turn it into a copy.

bnoordhuis · 2016-11-09T09:27:37Z

Updated, PTAL.

thefourtheye · 2016-11-09T09:32:05Z

Why don't we keep the changed logic indeps/v8/tools/run-valgrind.py at tools/run_valgrind.py and leave the deps untouched?

bnoordhuis · 2016-11-09T09:36:58Z

Git got confused, fixed now.

thefourtheye · 2016-11-09T10:23:45Z

Something is wrong. When I try to post a comment, it says "Position is invalid"

thefourtheye · 2016-11-09T10:33:11Z

Okay. Let me leave the comments here

Unused imports platform, re
s/V8_ROOT/NODE_ROOT/g
Nit: semi-colons at the end of the lines
Suggestion: Check if valgrind actually exists and fail with an error if it does not.

bnoordhuis · 2016-11-09T10:59:02Z

Suggestion: Check if valgrind actually exists and fail with an error if it does not.

That happens automatically, the Popen() call fails if the binary doesn't exist. I did the style fix-ups in a separate commit.

thefourtheye · 2016-11-09T11:55:10Z

@bnoordhuis Yes, but that check happens for each and every test case. So we will get a screen full of same error messages.

bnoordhuis · 2016-11-09T13:00:14Z

I don't really see the issue.

It was a symbolic link to deps/v8/tools/run-valgrind.py before. We are going to make changes to it and we don't want to carry the patch forward so make a copy. PR-URL: nodejs#9520 Reviewed By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

The script had a dependency on the copy of valgrind that is bundled with V8 but that only gets checked out when doing a full depot_tools checkout. Use the system-provided valgrind. PR-URL: nodejs#9520 Reviewed By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Node.js does not clean up on exit so don't complain about memory leaks but do complain about invalid memory access. In the future we may want to add a cleanup-on-exit flag or put together a suppression list. PR-URL: nodejs#9520 Reviewed By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

bnoordhuis · 2016-11-15T16:18:21Z

Thanks all, landed in f65a48f...a804627. I skipped the CI, it doesn't exercise the script.

It was a symbolic link to deps/v8/tools/run-valgrind.py before. We are going to make changes to it and we don't want to carry the patch forward so make a copy. PR-URL: #9520 Reviewed By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

The script had a dependency on the copy of valgrind that is bundled with V8 but that only gets checked out when doing a full depot_tools checkout. Use the system-provided valgrind. PR-URL: #9520 Reviewed By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Node.js does not clean up on exit so don't complain about memory leaks but do complain about invalid memory access. In the future we may want to add a cleanup-on-exit flag or put together a suppression list. PR-URL: #9520 Reviewed By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>