Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add nodereport to lookup.json #226

Merged
merged 1 commit into from
Feb 9, 2017
Merged

add nodereport to lookup.json #226

merged 1 commit into from
Feb 9, 2017

Conversation

gdams
Copy link
Member

@gdams gdams commented Nov 17, 2016

nodereport is a foundation module that we should be testing. It seems to be passing on our IBM machines so looks like it should be okay

@richardlau
Copy link
Member

It's mostly passing (see nodejs/node-report#24 and nodejs/node-report#26).

@MylesBorins
Copy link
Contributor

@gdams
Copy link
Member Author

gdams commented Nov 17, 2016

Weird, was passing for me on OSX

@richardlau
Copy link
Member

Looks like the compilation is not finding ares.h or zlib.h -- I'm wondering if this is a node-gyp bug when building with --nodedir as the headers under deps are moved to a different location (include/node) in Node.js' headers tarball.

@richardlau
Copy link
Member

Yes, looks like a node-gyp bug. Have raised nodejs/node-gyp#1055.

@richardlau
Copy link
Member

Build failures should have been addressed in nodereport's master by nodejs/node-report#30.

@MylesBorins
Copy link
Contributor

if this CI run is green then LGTM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/472/

@gdams
Copy link
Member Author

gdams commented Dec 10, 2016

This won't have been fixed because you need to cut a new release of nodereport first.... @richardlau

@richardlau
Copy link
Member

@GeorgeAdams95: nodereport v1.0.7 was released today.

@gdams
Copy link
Member Author

gdams commented Dec 13, 2016

@gibfahn
Copy link
Member

gibfahn commented Dec 13, 2016

(Investigating the flaky issues in the earlier CIs)

CI 3: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/477/

@MylesBorins
Copy link
Contributor

@gibfahn did that last ci run work? Job is no longer archived

@gibfahn
Copy link
Member

gibfahn commented Dec 28, 2016

@thealphanerd As I recall, there was an issue with CI that @richardlau was looking into, I'm not sure where it ended up.

@richardlau
Copy link
Member

@gibfahn it ended up with you 😛. IIRC we were seeing intermittent makefile errors (no rule to make target) on the CI that I was unable to reproduce locally. I had a test fix in my fork of nodereport which you were going to test on the CI machines.

@MylesBorins MylesBorins changed the title add nodereport to lookup.json [wip] add nodereport to lookup.json Jan 17, 2017
@richardlau
Copy link
Member

The various CI runs have expired -- Would it be possible to rerun so we can capture the failure output in this pull request? (I'm still expecting it to fail as we haven't made changes since we've been unable to reproduce locally).

@gdams
Copy link
Member Author

gdams commented Jan 20, 2017

@richardlau
Copy link
Member

https://ci.nodejs.org/job/gibfahn-citgm-smoker-more-platforms/MACHINE=ppcbe-ubuntu1404/79/console
looks unrelated and is probably a problem with Node.js master:

verbose: npm-install:        | make: Entering directory `/tmp/102cb0d9-43da-48cd-9cec-e9c40b469a9f/nodereport/build'
verbose: npm-install:        | CXX(target) Release/obj.target/nodereport/src/node_report.o
warn: npm-install:        | In file included from ../node_modules/nan/nan.h:47:0,                                                                                        
warn:                     | from ../src/node_report.h:4,                                                                                                                 
warn:                     | from ../src/node_report.cc:1:                                                                                                                
warn:                     | /home/iojs/.node-gyp/8.0.0-nightly2017011949902124a9/include/node/node.h:44:33: fatal error: tracing/trace_event.h: No such file or directory
warn:                     | #include "tracing/trace_event.h"                                                                                                             
warn:                     | ^                                                                                                                                            
warn:                     | compilation terminated.                                                                                                                      
warn: npm-install:        | make: *** [Release/obj.target/nodereport/src/node_report.o] Error 1
verbose: npm-install:        | make: Leaving directory `/tmp/102cb0d9-43da-48cd-9cec-e9c40b469a9f/nodereport/build'
warn: npm-install:        | gyp ERR! build error
warn: npm-install:        | gyp ERR! stack Error: `make` failed with exit code: 2

@richardlau
Copy link
Member

@gdams ran https://ci.nodejs.org/view/Node.js-citgm/job/gibfahn-citgm-smoker-more-platforms/80/console with Node 6 which replicates what we were seeing before:

gibfahn-citgm-smoker-more-platforms » ppcbe-ubuntu1404 completed with result SUCCESS

gibfahn-citgm-smoker-more-platforms » aix61-ppc64 completed with result SUCCESS
gibfahn-citgm-smoker-more-platforms » ppcle-ubuntu1404 completed with result SUCCESS

gibfahn-citgm-smoker-more-platforms » fedora22 completed with result FAILURE
gibfahn-citgm-smoker-more-platforms » osx1010 completed with result FAILURE
gibfahn-citgm-smoker-more-platforms » debian8-64 completed with result SUCCESS
gibfahn-citgm-smoker-more-platforms » fedora23 completed with result FAILURE
gibfahn-citgm-smoker-more-platforms » ubuntu1204-64 completed with result FAILURE
gibfahn-citgm-smoker-more-platforms » rhel72-s390x completed with result FAILURE
gibfahn-citgm-smoker-more-platforms » ubuntu1404-64 completed with result SUCCESS
gibfahn-citgm-smoker-more-platforms » ubuntu1604-64 completed with result FAILURE

The failure (e.g. https://ci.nodejs.org/job/gibfahn-citgm-smoker-more-platforms/MACHINE=fedora22/80/console):

verbose: npm-install:        | > nodereport@1.0.7 install /tmp/993291a5-2e40-4728-9b9a-d83e4e0f88c7/nodereport
verbose:                     | > node-gyp rebuild                                                             
verbose: npm-install:        | make: Entering directory '/tmp/993291a5-2e40-4728-9b9a-d83e4e0f88c7/nodereport/build'
warn: npm-install:        | make: *** No rule to make target '/tmp/993291a5-2e40-4728-9b9a-d83e4e0f88c7/nodereport/build/Release/nodereport.node', needed by '/tmp/993291a5-2e40-4728-9b9a-d83e4e0f88c7/nodereport/nodereport.node'.  Stop.
verbose: npm-install:        | CXX(target) Release/obj.target/nodereport/src/node_report.o
verbose:                     | CXX(target) Release/obj.target/nodereport/src/module.o     
warn: npm-install:        | make: *** Waiting for unfinished jobs....
verbose: npm-install:        | make: Leaving directory '/tmp/993291a5-2e40-4728-9b9a-d83e4e0f88c7/nodereport/build'

When we looked at the end of last year, this failure was not consistent across Linux distributions (it would fail on different distributions from run to run).

@gdams gdams force-pushed the nodereporter branch 2 times, most recently from 171c836 to eedccea Compare January 20, 2017 13:18
@gdams gdams changed the title [wip] add nodereport to lookup.json add nodereport to lookup.json Jan 20, 2017
@richardlau
Copy link
Member

@gdams ran my test fork in https://ci.nodejs.org/view/Node.js-citgm/job/gibfahn-citgm-smoker-more-platforms/82/console and everything passed (!) so I'll submit a PR to nodereport.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jan 26, 2017
Move the include from src/node.h to src/node_internals.h.

trace_event.h is not shipped in binary-only and headers-only tarballs,
making it currently impossible to build add-ons against them.

PR-URL: nodejs#10959
Refs: nodejs/citgm#226 (comment)
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matthew Loring <mattloring@google.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@gdams
Copy link
Member Author

gdams commented Feb 9, 2017

okay so node-report is now landed and is fully fixed: https://ci.nodejs.org/view/Node.js-citgm/job/gibfahn-citgm-smoker-more-platforms/

CC @richardlau @gibfahn @MylesBorins

gibfahn

This comment was marked as off-topic.

@gdams gdams merged commit 02d3f7c into nodejs:master Feb 9, 2017
targos pushed a commit to targos/node that referenced this pull request Mar 1, 2017
Move the include from src/node.h to src/node_internals.h.

trace_event.h is not shipped in binary-only and headers-only tarballs,
making it currently impossible to build add-ons against them.

PR-URL: nodejs#10959
Refs: nodejs/citgm#226 (comment)
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matthew Loring <mattloring@google.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 1, 2017
Move the include from src/node.h to src/node_internals.h.

trace_event.h is not shipped in binary-only and headers-only tarballs,
making it currently impossible to build add-ons against them.

PR-URL: nodejs#10959
Refs: nodejs/citgm#226 (comment)
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matthew Loring <mattloring@google.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants