Fix version reporting in NodeReport section #30

richardlau · 2016-11-24T20:47:49Z

Fixes: #29

Replaces compile time constants with the contents of process.versions. As a bonus ares.h and zlib.h are no longer required, which should unblock nodejs/citgm#226.

Report content before changes:

Node.js version: v6.9.1
(v8: 5.1.281.84, libuv: 1.9.1, zlib: 1.2.8, ares: 1.10.1-DEV)

after changes:

process.versions = {
  http_parser: 2.7.0
  node: 6.9.1
  v8: 5.1.281.84
  uv: 1.9.1
  zlib: 1.2.8
  ares: 1.10.1-DEV
  icu: 57.1
  modules: 48
  openssl: 1.0.2j
}

Tests have been extended to validate the versions contained in the report.

bnoordhuis · 2016-11-24T20:50:31Z

src/node_report.cc

+      fprintf(fp, "  %s: %s\n", *name, *version);
+    }
+  }
+  fprintf(fp, "}\n");


How sure are you that it's safe to call into the VM at this point?

Also, spaces around operators a few lines up?

Spaces fixed, thanks.

Other bits of the report later on (e.g. GC statistics, JavaScript stack) are generated via VM calls, so I guess as safe as they are? For this PR an alternative to avoid VM calls here is perhaps capturing process.versions into a static buffer on module initialization and then outputting the captured buffer when the report is written.

I would capture it at the earliest opportunity, that will let you deal with errors more gracefully. For example, if user JS code (however unwise) did the below, your code would assert and abort.

Object.defineProperty(process, 'versions', {get() { throw 'boom' }});

This particular example is a little farfetched but buggy code clobbering process is not unheard of.

rnchamberlain · 2016-11-25T10:06:09Z

I was trying to avoid going into JavaScript-land if at all possible. As Ben says, things may be damaged, especially JS objects on the heap. For v8 there is a nice C++ API - V8::GetVersion() - but for Node and the other components just the compile-time constants. I guess building the npm on one Node patch level and running on another is quite likely though, and we do want the actual runtime versions in the report.

Yes we should capture the versions at module load time and stash them rather than access the process object at dump time.

Incidently, using SetAbortOnUncaughtExceptionCallback() to intercept exceptions is not working very well, so I may need to switch that to use process.on('uncaughtException'). Longer term I think we'll need better APIs in v8 and Node, but that's a long haul.

One other design thing was to make NodeReport easily readable by non-programmers, admins etc. So I prefer something that headlines the Node version like
Node.js version: v6.9.1

rather than 'programmer-style' output
process.versions = {
http_parser: 2.7.0
node: 6.9.1 ....

richardlau · 2016-11-25T14:41:35Z

Updated to capture versions at module load time.

Changed the output to be slightly more human readable, currently:

http_parser version: 2.7.0
node version: 6.9.1
v8 version: 5.1.281.84
uv version: 1.9.1
zlib version: 1.2.8
ares version: 1.10.1-DEV
icu version: 57.1
modules version: 48
openssl version: 1.0.2j

Comments on formatting? I could pull out node so that it's first in the listed output.

I think I also need to change SetVersionString() to check the MaybeLocal's instead of calling ToLocalChecked() so that the code can fail gracefully (instead of aborting).

rnchamberlain · 2016-11-25T16:09:30Z

Yes, suggest have node and v8 top of the list as they are of most interest to people I think. I had the less interesting components on a single line, to keep the intro section short - you don't have to scroll down far to see the stack traces, but it's not a big deal.

Anything to make the code resilient - so we carry on - is good, with some 'version information not available' indication maybe. Thanks!

sam-github · 2016-11-25T16:30:24Z

Ah, scratch that last comment, I'm sorry. I liked the original format, but all the process.versions keys should
be mentioned, with the same key! libuv -> uv. And I'd sort.

Node.js version: v6.9.1
(ares: 1.10.1-DEV, http_parser: 2.7.0, icu: 57.1, modules: 48, openssl: 1.0.2j, uv: 1.9.1, v8: 5.1.281.84, zlib: 1.2.8)

As an aside, modules is an absolutely terrible name for the ABI version, and its meaning is undocumented, I just had to read the source to figure out what it was (since it clearly isn't a component in deps/, like everything else except node). :-(. /cc @mhdawson

richardlau · 2016-11-25T16:34:43Z

My issue with including all the components on one line is that the line is quite long.

Should we filter out modules from the list?

sam-github · 2016-11-25T16:38:32Z

No, patching over oddities in node just makes things odder, we'd have two special snowflakes, not one.

The line is not longer than lots of other lines, so I don't think its a big issue, but I see your point. I think repeating the word "version" in every line is ugly and repetitive. Perhaps:

Node.js version: v6.9.1
  ares: 1.10.1-DEV
  http_parser: 2.7.0
  icu: 57.1
  modules: 48
  openssl: 1.0.2j
  uv: 1.9.1
  v8: 5.1.281.84
  zlib: 1.2.8

mhdawson · 2016-11-25T18:37:53Z

I think people understand what

process.versions = {
  http_parser: 2.7.0
  node: 6.9.1
  v8: 5.1.281.84
  uv: 1.9.1
  zlib: 1.2.8
  ares: 1.10.1-DEV
  icu: 57.1
  modules: 48
  openssl: 1.0.2j
}

is.

Would it make sense to stick to that format even if we don't go back to javascript to generate it ?

mhdawson · 2016-11-25T18:39:22Z

@sam-github I agree that modules: is not a great name, but thinking we want to be consistent with process.versions.

richardlau · 2016-11-25T19:52:22Z

Updated so the output is now similar to what it was before. I've enclosed the components (i.e. everything in process.versions except for node) in () and wrapped at 80 characters. I aligned subsequent lines (i.e. the second line in the sample output is indented by one character) but that can easily be changed.

Node.js version: v6.9.1
(ares: 1.10.1-DEV, http_parser: 2.7.0, icu: 57.1, modules: 48, openssl: 1.0.2j, 
 uv: 1.9.1, v8: 5.1.281.84, zlib: 1.2.8)

I left the tests so that they only check the information is there (and at the moment ignore formatting).

I'll squash later on.

While I'm editing in this area, as per #29 I'll add a line to this section of the report that reports the version of nodereport used and which version of Node.js it was compiled against, e.g. nodereport v1.0.6 built against Node.js v6.9.1.

richardlau · 2016-11-25T19:56:01Z

Oh I also attempted to make the code resilient, with a default Unable to determine Node.js version if something goes wrong. I'll try to add a test to verify this (probably by clobbering process.version/process.versions.

richardlau · 2016-11-25T20:00:18Z

Possibly stupid question, should it be

nodereport v1.0.6 built against Node.js v6.9.1

or

NodeReport v1.0.6 built against Node.js v6.9.1

?

sam-github · 2016-11-25T20:54:10Z

NodeReport v1.0.6 built against Node.js v6.9.1 or nodereport v1.0.6 built against node v6.9.1, IMO.

mhdawson · 2016-11-28T16:26:08Z

It should be Node.js, not so sure about nodereport versus NodeReport but I'm leaning to NodeReport.

bnoordhuis · 2016-11-28T21:09:03Z

src/node_report.cc

@@ -70,6 +68,7 @@ const char* v8_states[] = {"JS", "GC", "COMPILER", "OTHER", "EXTERNAL", "IDLE"};
 static bool report_active = false; // recursion protection
 static char report_filename[NR_MAXNAME + 1] = "";
 static char report_directory[NR_MAXPATH + 1] = ""; // defaults to current working directory
+static std::string versionString = "Unable to determine Node.js version\n";


Can you use snake_case for consistency? (Here and elsewhere, e.g., also please update processProp, versionProp, and so on.)

bnoordhuis · 2016-11-28T21:11:50Z

src/node_report.cc

+    isolate->GetCurrentContext()->Global(),
+    processProp.ToLocalChecked());
+  if (process.IsEmpty() || process.ToLocalChecked()->IsUndefined()
+      || trycatch.HasCaught()) {


I don't know how much core style is supposed to be followed but assuming it's the same style:

the operator should go on the previous line

line continuations in the Nan::Get() call should be indented with four spaces

The call to IsUndefined() looks like it should be !process.ToLocalChecked()->IsObject()?

bnoordhuis · 2016-11-28T21:16:04Z

src/node_report.cc

+  Nan::TryCatch trycatch;
+  Nan::MaybeLocal<String> processProp = Nan::New<String>("process");
+  if (processProp.IsEmpty() || trycatch.HasCaught()) {
+    trycatch.Reset();


.Reset() is the default behaviour unless .ReThrow() is called, you don't need to call it explicitly. You also don't really need to check for .HasCaught() because .IsEmpty() is always true in case of exception.

A more succinct way of writing it:

Local<String> process_prop; if (!Nan::New<String>("process").ToLocal(&process_prop)) return;

Thanks, I've redone SetVersionString() in the suggested more succinct style and it's much easier to read now.

bnoordhuis · 2016-11-28T21:24:03Z

src/node_report.cc

+  size_t lineLength = 1;
+  versionString.append("(");
+  std::sort(compVersions.begin(), compVersions.end());
+  for (std::vector<std::string>::iterator it = compVersions.begin();


richardlau · 2016-11-29T06:17:06Z

Fixed suggestions from @bnoordhuis.

Added reporting of NodeReport version and test.

Node.js version: v6.9.1
(ares: 1.10.1-DEV, http_parser: 2.7.0, icu: 57.1, modules: 48, openssl: 1.0.2j, 
 uv: 1.9.1, v8: 5.1.281.84, zlib: 1.2.8)

NodeReport version: 1.0.6 (built against Node.js v6.9.1)

richardlau · 2016-11-29T19:13:55Z

Made the code even more resilient -- It will now get the Node.js version from process.versions if process.version is unavailable. If neither process.version nor process.versions is available, the report will contain:

Unable to determine Node.js version

NodeReport version: 1.0.6 (built against Node.js v6.9.1)

Squashed commits.

rnchamberlain · 2016-12-05T15:02:01Z

LGTM, new tests all ran ok (on Linux)

@bnoordhuis - are your review comments all resolved now? thanks

gdams · 2016-12-08T15:17:33Z

@bnoordhuis is this good to merge because this is blocking us from adding nodereport to citgm?
nodejs/citgm#226

bnoordhuis

LGTM with two final comments and a suggestion, otherwise looks nice!

bnoordhuis · 2016-12-08T18:01:52Z

src/node_report.cc

+  v8::Local<v8::Object> global_obj = isolate->GetCurrentContext()->Global();
+  v8::Local<v8::Value> process_value;
+  if (!Nan::Get(global_obj, process_prop).ToLocal(&process_value)) return;
+  v8::Local<v8::Object> process_obj = process_value.As<v8::Object>();


For the sake of robustness, you should if (!process_value->IsObject()) return; first.

bnoordhuis · 2016-12-08T18:05:13Z

src/node_report.cc

+    if (!Nan::Get(versions_obj, name_value).ToLocal(&version_value)) continue;
+
+    Nan::Utf8String component_name(name_value);
+    Nan::Utf8String component_version(version_value);


Robustness: if (*component_name == nullptr || *component_version == nullptr) continue;

bnoordhuis · 2016-12-08T18:09:04Z

test/common.js

+      }
+      nodeComponents.forEach((c) => {
+        if (c !== 'node') {
+          if (expectedVersions.indexOf(c) === -1) {


Might as well use .includes() here.

NodeReport claims to be Node.js v4.x compatible ("node": ">=4.0.0") and the version of V8 in v4.x doesn't have Array.prototype.includes().

Report the versions of Node.js and its components based on the runtime and not compile time constants. Report NodeReport version and the version of Node.js it was built against. Extend the tests to validate the versions reported in the report. Fixes: nodejs#29

richardlau · 2016-12-08T20:18:11Z

Updated to address Ben's robustness comments. Haven't implemented the suggested use of .includes() as that isn't supported in Node.js v4.x (which NodeReport supports).

gdams

LGTM

Report the versions of Node.js and its components based on the runtime and not compile time constants. Report NodeReport version and the version of Node.js it was built against. Extend the tests to validate the versions reported in the report. Fixes: #29 PR-URL: #30 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>

rnchamberlain · 2016-12-09T12:49:11Z

Landed as 368f253

bnoordhuis reviewed Nov 24, 2016

View reviewed changes

richardlau force-pushed the versions branch from 132bc9e to 4788811 Compare November 24, 2016 20:50

richardlau force-pushed the versions branch from 4788811 to 976ae46 Compare November 25, 2016 14:25

richardlau force-pushed the versions branch from 976ae46 to 0645644 Compare November 25, 2016 18:15

bnoordhuis suggested changes Nov 28, 2016

View reviewed changes

richardlau force-pushed the versions branch from a30d76d to f130d42 Compare November 29, 2016 19:09

richardlau force-pushed the versions branch from f130d42 to 262dc0a Compare November 29, 2016 20:59

richardlau mentioned this pull request Dec 5, 2016

Fix include path when pointing to Node.js source nodejs/node-gyp#1055

Closed

bnoordhuis approved these changes Dec 8, 2016

View reviewed changes

richardlau force-pushed the versions branch from 262dc0a to 10fa1f5 Compare December 8, 2016 20:15

gdams approved these changes Dec 9, 2016

View reviewed changes

rnchamberlain closed this Dec 9, 2016

richardlau mentioned this pull request Dec 9, 2016

add nodereport to lookup.json nodejs/citgm#226

Merged

richardlau mentioned this pull request Feb 9, 2017

mac: Fix compilation errors against Node > v5 #56

Closed

Fix version reporting in NodeReport section #30

Fix version reporting in NodeReport section #30

Conversation

richardlau commented Nov 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnchamberlain commented Nov 25, 2016

richardlau commented Nov 25, 2016

rnchamberlain commented Nov 25, 2016

sam-github commented Nov 25, 2016 • edited Loading

richardlau commented Nov 25, 2016

sam-github commented Nov 25, 2016

mhdawson commented Nov 25, 2016 • edited Loading

mhdawson commented Nov 25, 2016

richardlau commented Nov 25, 2016

richardlau commented Nov 25, 2016

richardlau commented Nov 25, 2016

sam-github commented Nov 25, 2016

mhdawson commented Nov 28, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardlau commented Nov 29, 2016

richardlau commented Nov 29, 2016

rnchamberlain commented Dec 5, 2016

gdams commented Dec 8, 2016 • edited Loading

bnoordhuis 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardlau commented Dec 8, 2016

gdams left a comment

Choose a reason for hiding this comment

rnchamberlain commented Dec 9, 2016

richardlau commented Nov 24, 2016 •

edited

Loading

sam-github commented Nov 25, 2016 •

edited

Loading

mhdawson commented Nov 25, 2016 •

edited

Loading

gdams commented Dec 8, 2016 •

edited

Loading