node-report fails compilation for smartos

[mhdawson]

Running in CI node-report seems to fail compilation on smartos

https://ci.nodejs.org/view/post-mortem/job/nodereport-continuous-integration/39/MACHINE=smartos15-64/console

<features.h>                                                        
warn:                                                      | ^                                                                            
warn:                                                      | compilation terminated.                                                      
warn: e46e12b6b495d53f5a38ce3315099be4755dfef4 npm-install:| In file included from ../src/module.cc:1:0:                                  
warn:                                                      | ../src/node_report.h:9:22: fatal error: features.h: No such file or directory
warn:                                                      | #include <features.h>                                                        
warn:                                                      | ^                                                                            

[mhdawson]

@rnchamberlain moved over to right repo

[jclulow]

I think features.h is a glibc-ism? For what purpose is that header included?

[richardlau]

I can't remember exactly what it is being used for, but features.h was one of the things that had to be excluded when we added AIX support (#17).

[hhellyer]

The features.h include is in a slightly odd block (if not windows, then if not apple or aix - it should probably be #ifdef __linux__) but actually just removing it doesn't break the linux build.

I tried that on citgm and unsurprisingly it still didn't compile, that was due to an issue with one of the ulimit constants in node_report.cc.

The more general problem is that node_report.cc uses a lot of OS specific function to gather it's information. Just correcting the #ifdef's isn't enough, we'd need to add proper support for ulimits, stack walking, reading the command line and so on. For example at the moment the loaded libraries section will be empty on smartos as there's no #ifdef case in there for it.

It's probably not too difficult to add that function but it would require access to a smartos box to do it. Without that work node-report would probably be rather light on content on smartos.

[jclulow]

I took a quick look, and I have some preliminary porting advice:

• It looks like you're just using backtrace(3C) for stack walking, at least.

• It seems that the AIX method of reading a psinfo_t from /proc/$$/psinfo, then dereferencing the pr_argv member, will work on SmartOS/illumos as well.

• According to gettimeofday(3C), the members in our struct timeval are both of type long.

• It seems that, like AIX, we don't have RLIMIT_MEMLOCK; our resource_controls(5) interface would be the place to look -- probably at one of the max-locked-memory controls.

• For printing the loaded libraries, I think you want dlinfo(3C). By way of example:

#include <stdio.h>
#include <stdlib.h>

#include <dlfcn.h>
#include <link.h>
#include <limits.h>
#include <sys/mman.h>

#include <err.h>

int
main(int argc, char *argv[])
{
	Link_map *p;

	if (dlinfo(RTLD_SELF, RTLD_DI_LINKMAP, &p) == -1) {
		errx(1, "dlinfo failure: %s\n", dlerror());
	}

	for (Link_map *l = p; l != NULL; l = l->l_next) {
		fprintf(stderr, "  %s\n", l->l_name);
	}
}

Seems to do roughly what you might expect:

jclulow@jmc3 /var/tmp
 $ gcc -std=gnu99 -o print_libs print_libs.c
jclulow@jmc3 /var/tmp
 $ ./print_libs
  print_libs
  /lib/libc.so.1

[hhellyer]

The AIX similarities makes sense, on the tangled Unix family tree SmartOS is probably on a closer branch to AIX than Linux. (Assuming I've got my origins right, I might not!) Obviously not everything is the same.

If it's not too much of an abuse of the CI hardware the development could probably be done batch mode by submitting branches https://ci.nodejs.org/view/post-mortem/job/nodereport-continuous-integration/ (I think the builds can be restricted down to one machine type). I'm not sure if that's against the CI etiquette though. Otherwise it's a question of getting login access to a SmartOS box.

[gibfahn]

The CI is designed to be runnable on subsets of machines, and on forks. You can use a space-separated list of machines to run like this:

[mhdawson]

We do have a process were we can get you access to one of the smartos boxes https://github.com/nodejs/build/blob/master/doc/process/special_access_to_build_resources.md.

I think it depends on how many iterations you think it might take in the CI. The job runs quite quickly for node-report so running it a 50-100 times (or more) is probably less of an impact than taking the machine off-line while you investigated once you had access. I agree that in general just the CI to develop is not a good way to go, but if we think we'll be able to figure it out in a reasonable number of iterations it may be the right way in this case.

[hhellyer]

We've got a couple of refactoring PR's outstanding, the rename to node-report and switching to use C++ streams to support getReport. Once those are merged I'll take a look at getting node-report (as it will be) compiling on SmartOS machines.

(Edit: I forgot about the linter PR - that'll add a lot of changes too!)

I doubt it'll require too many iterations really but there's no sense doing it right before that and having to make more changes afterwards. (Hopefully that means doing it around the start of next week.)

[hhellyer]

@jclulow I've raised a PR that should get the CI tests clean: #62

[rnchamberlain]

PR #62 landed, CI tests now passing on smartos:
https://ci.nodejs.org/view/post-mortem/job/nodereport-continuous-integration/