Add command line to nodereport. #34

hhellyer · 2016-12-16T14:45:31Z

This patch adds the command line used to start the process to nodereport.
It's based off https://github.com/rnchamberlain/nodereport/tree/clang_warning so it could be built and tested on Mac. It should merge cleanly once that has been landed.

One line is added to the NodeReport file. Sample output from a test script that generates a report:

> node --no-warnings my_application.js -a an_argument file.txt

First part of the generated NodeReport file:

================================================================================
==== NodeReport ================================================================

Event: JavaScript API, location: "TriggerReport"
Filename: NodeReport.20161216.145623.6353.001.txt
Dump event time:  2016/12/16 14:56:23
Module load time: 2016/12/16 14:56:23
Process ID: 6353
Command line arguments: node --no-warnings my_application.js -a an_argument file.txt

Node.js version: v6.5.0

Fishrock123 · 2016-12-16T14:47:55Z

@hhellyer could you document how this is supposed to be used? :D

hhellyer · 2016-12-16T15:00:58Z

I've added some sample output.

cjihrig · 2016-12-16T15:03:56Z

@hhellyer it looks like there are conflicts in src/node_report.cc that need to be resolved.

richardlau · 2016-12-16T15:09:15Z

Is it possible to get the equivalent information on AIX and Windows?

richardlau · 2016-12-16T15:45:20Z

Also needs tests.

hhellyer · 2016-12-16T15:52:05Z

@richardlau
Not sure about AIX, it doesn't have /proc/self/cmdline so I'll need to find something else.

It looked like Windows had globals (__argv, __argc) which you could use, though I've not tested them as I don't have a windows box easily accessible. See: https://msdn.microsoft.com/en-us/library/dn727674.aspx for details, there may be a better way - I only had a quick look.

richardlau · 2016-12-16T22:21:58Z

Windows: GetCommandLine()

AIX: getargs(). I actually used this to fix libuv/libuv#464 (although in that case we were only interested in the first argument of the command line).

richardlau

Would like tests for this to be added and coverage extended to Windows and AIX.

rnchamberlain · 2016-12-19T12:00:31Z

Looks good

Suggest just Command line: rather than Command line arguments:
Windows and AIX support would be great - if not too much work
Not sure we'll want tests for every output line in NodeReport, but as this is using some significant OS platform calls then yes maybe a test would be good.
I've landed the Mac build fix, so probably best to rebase this now
It would be handy if the command line could be saved (maybe at start-up as we do for load time and the version strings) because we need to know whether the user specified the --abort-on-uncaught-exception option

…ort on Linux, Mac and AIX. On Linux it reads /proc/self/cmdline. On Mac is uses _NSGetArgv and _NSGetArgc. On AIX it reads /proc/PID/psinfo. The command line is read and stored at startup so files aren't opened when a crash is occuring.

hhellyer · 2017-01-04T16:10:40Z

I've added AIX support. Note that on AIX we get the wrong command but that's because of nodejs/node#10607 and we give the same result as the ps command.

I've squashed the commits down, it looks like github shows the changes on the branch and not a diff against the merge destination which is confusing if you pull in changes from there, which I had done. At least I think that's what was happening... :-)

gibfahn · 2017-01-04T16:16:38Z

@hhellyer AFAIK the Files is just a diff against the merge destination, and if you want to see individual commit diffs you look at Commits. However Github does weird things sometimes.

richardlau · 2017-01-04T17:45:43Z

src/node_report.cc

+      commandline_string += seperator + argv[i];
+      seperator = " ";
+    }
+#elif _AIX


The indentation for this #elif block is different from the rest of this function, please can you make it consistent?

The rest of the function was wrong and didn't match the file. I'd indented when starting #ifdef sections. I've adjusted the rest of the function to be correct.

mhdawson

What about additional tests to validate we get he command line section ?

richardlau · 2017-01-05T18:49:46Z

src/node_report.cc

+  int argc = *_NSGetArgc();
+
+  commandline_string = "";
+  std::string seperator = "";


nit: seperator -> separator

richardlau · 2017-01-05T18:49:54Z

src/node_report.cc

+  fclose(psinfo_fd);
+  if (bytesread == sizeof(psinfo_t)) {
+    commandline_string = "";
+    std::string seperator = "";


nit: seperator -> separator

Add a check for the command line to common.js. Add expected command line to all the test cases that pass an expected pid.

hhellyer · 2017-01-06T14:10:52Z

I've added the testcases or extended the existing ones.
(I've also fixed the spelling mistake.)

This change adds the command line to NodeReport on Windows. It uses the Windows GetCommandLine() API to obtain the command line. Also includes a minor change to print "Command line:" rather than "Command line arguments:" in the report as prefix, plus updates to the testcases.

rnchamberlain · 2017-01-09T14:10:39Z

Added commit dd20187 for support on Windows. I think this PR has all we need now.

richardlau · 2017-01-09T19:42:41Z

test/common.js

@@ -36,18 +36,18 @@ exports.validate = (t, report, options) => {
      const expectedVersions = options ?
                               options.expectedVersions || nodeComponents :
                               nodeComponents;
-      const plan = REPORT_SECTIONS.length + nodeComponents.length + 2;
+      var plan = REPORT_SECTIONS.length + nodeComponents.length + 2;
+      if( options.commandline ) plan++;


Spaces -> if (options.commandline) plan++;

richardlau · 2017-01-09T19:42:51Z

test/common.js

      const nodeReportSection = getSection(reportContents, 'NodeReport');
-      t.match(nodeReportSection, new RegExp('Process ID: ' + pid),
+      t.contains(nodeReportSection, new RegExp('Process ID: ' + pid),


Why was this changed?

@hhellyer said it fixed a test failure, but http://www.node-tap.org/asserts/ indicates that .contains is simply a synonym for .match, so I have reverted it. The tests pass ok for me on Linux, Windows and Mac.

richardlau · 2017-01-09T19:46:23Z

test/common.js

+          // On Windows we need to strip out double quotes from the command line in the
+          // report, and escape the backslashes in the regex comparison string.
+          t.match(nodeReportSection.replace(/"/g,''),
+                  new RegExp('Command line: ' + (options.commandline).replace(/\\/g,'\\\\')),


I think this line is > 80 chars.

mhdawson

LGTM subject to fixing the minor comments from Richard L

Currect spacing on if test, revert use of t.contains to t.match, reduce line lengths to 80 or less.

richardlau · 2017-01-10T19:19:11Z

test/common.js

+      if (options && options.commandline) {
+        if (this.isWindows()) {
+          // On Windows we need to strip double quotes from the command line in
+          // the report, and escape backslashes in the regex comparison string.


n.b. I'm not seeing any double quotes in the reported command line in the reports generated from the tests on Windows.

It might depend on the shell or the executable path. This is what I am seeing on Windows:

==== NodeReport ================================================================ Event: JavaScript API, location: "TriggerReport" Filename: NodeReport.20170110.113318.2184.001.txt Dump event time: 2017/01/10 11:33:18 Module load time: 2017/01/10 11:33:18 Process ID: 2184 Command line: "C:\Program Files\nodejs6\node.exe" C:\tmp\nodereport\test\test-api-bad-processobj.js child

Ah I think it might be spaces in the paths -- I get the same if I use Node.js installed under C:\Program Files

On Linux it reads /proc/self/cmdline. On Mac is uses _NSGetArgv and _NSGetArgc. On AIX it reads /proc/PID/psinfo. On Windows it uses the GetCommandLine() API. The command line is read and stored at startup so files aren't opened when a crash is occuring. PR-URL: #34 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

rnchamberlain · 2017-01-11T11:27:58Z

Landed as c6db073

hhellyer force-pushed the cmdline branch from 0dea11c to b41dfb5 Compare December 16, 2016 15:28

richardlau suggested changes Dec 16, 2016

View reviewed changes

hhellyer force-pushed the cmdline branch from eb551e1 to d5673b8 Compare January 4, 2017 16:06

richardlau reviewed Jan 4, 2017

View reviewed changes

mhdawson reviewed Jan 4, 2017

View reviewed changes

Correct white space. Shouldn't indent when starting a #ifdef.

412efb4

richardlau reviewed Jan 5, 2017

View reviewed changes

src/node_report.cc

int argc = *_NSGetArgc();

commandline_string = "";

std::string seperator = "";

Copy link

Member

richardlau Jan 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: seperator -> separator

richardlau reviewed Jan 5, 2017

View reviewed changes

Fix the spelling of separator

1294c2a

Add a check for the command line to common.js. Add expected command line to all the test cases that pass an expected pid.

richardlau suggested changes Jan 9, 2017

View reviewed changes

mhdawson approved these changes Jan 9, 2017

View reviewed changes

Fix code review and formatting nits

5872102

Currect spacing on if test, revert use of t.contains to t.match, reduce line lengths to 80 or less.

richardlau approved these changes Jan 10, 2017

View reviewed changes

richardlau reviewed Jan 10, 2017

View reviewed changes

rnchamberlain closed this Jan 11, 2017

richardlau mentioned this pull request Jan 20, 2017

aix: skip command line check for test-fatal-error #41

Closed

hhellyer deleted the cmdline branch April 26, 2017 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add command line to nodereport. #34

Add command line to nodereport. #34

hhellyer commented Dec 16, 2016 •

edited by rnchamberlain

Loading

Fishrock123 commented Dec 16, 2016

hhellyer commented Dec 16, 2016

cjihrig commented Dec 16, 2016

richardlau commented Dec 16, 2016

richardlau commented Dec 16, 2016

hhellyer commented Dec 16, 2016

richardlau commented Dec 16, 2016

richardlau left a comment

rnchamberlain commented Dec 19, 2016 •

edited

Loading

hhellyer commented Jan 4, 2017

gibfahn commented Jan 4, 2017

richardlau Jan 4, 2017

hhellyer Jan 5, 2017

mhdawson left a comment

richardlau Jan 5, 2017

richardlau Jan 5, 2017

hhellyer commented Jan 6, 2017

rnchamberlain commented Jan 9, 2017

richardlau Jan 9, 2017

rnchamberlain Jan 10, 2017

richardlau Jan 9, 2017

rnchamberlain Jan 10, 2017 •

edited

Loading

richardlau Jan 9, 2017

rnchamberlain Jan 10, 2017

mhdawson left a comment

richardlau Jan 10, 2017

rnchamberlain Jan 11, 2017

richardlau Jan 11, 2017

rnchamberlain commented Jan 11, 2017

Add command line to nodereport. #34

Add command line to nodereport. #34

Conversation

hhellyer commented Dec 16, 2016 • edited by rnchamberlain Loading

Fishrock123 commented Dec 16, 2016

hhellyer commented Dec 16, 2016

cjihrig commented Dec 16, 2016

richardlau commented Dec 16, 2016

richardlau commented Dec 16, 2016

hhellyer commented Dec 16, 2016

richardlau commented Dec 16, 2016

richardlau left a comment

Choose a reason for hiding this comment

rnchamberlain commented Dec 19, 2016 • edited Loading

hhellyer commented Jan 4, 2017

gibfahn commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhellyer commented Jan 6, 2017

rnchamberlain commented Jan 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnchamberlain Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnchamberlain commented Jan 11, 2017

hhellyer commented Dec 16, 2016 •

edited by rnchamberlain

Loading

rnchamberlain commented Dec 19, 2016 •

edited

Loading

rnchamberlain Jan 10, 2017 •

edited

Loading