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

build: add tracing/trace_event.h to tarballs #10929

Closed
wants to merge 1 commit into from

Conversation

richardlau
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

Discovered during nodejs/citgm#226 (comment), which was attempting to build a native module (nodereport) against a recent nightly (node-v8.0.0-nightly2017011949902124a9).

Failing CITGM run: https://ci.nodejs.org/job/gibfahn-citgm-smoker-more-platforms/79/
e.g. https://ci.nodejs.org/job/gibfahn-citgm-smoker-more-platforms/MACHINE=ppcbe-ubuntu1404/79/console

verbose: using-node          | /home/iojs/build/workspace/gibfahn-citgm-smoker-more-platforms/MACHINE/ppcbe-ubuntu1404/node-v8.0.0-nightly2017011949902124a9-linux-ppc64/bin/node
verbose: using-npm           | /home/iojs/build/workspace/gibfahn-citgm-smoker-more-platforms/MACHINE/ppcbe-ubuntu1404/node-v8.0.0-nightly2017011949902124a9-linux-ppc64/bin/npm
warn: update-available    | v1.10.1 (current: v1.6.1)
warn:                     | npm install -g citgm     
info: npm:                | Downloading project: https://github.com/nodejs/nodereport/archive/v1.0.7.tar.gz
info: npm:                | Project downloaded nodereport-1.0.7.tgz
info: npm:                | install started     
verbose: npm-install:        | > nodereport@1.0.7 install /tmp/102cb0d9-43da-48cd-9cec-e9c40b469a9f/nodereport
verbose:                     | > node-gyp rebuild                                                             
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

It looks like node.h was modifed by #9304 to include tracing/trace_event.h but tracing/trace_event.h is missing from the tarballs (both the binary and header tarballs).

node.h was modifed by nodejs#9304 to include tracing/trace_event.h but
tracing/trace_event.h was not added to the headers installed by
tools/install.py.
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. lts-watch-v6.x labels Jan 21, 2017
@richardlau
Copy link
Member Author

cc @nodejs/build

@gibfahn
Copy link
Member

gibfahn commented Jan 21, 2017

cc/ @matthewloring @bnoordhuis

@bnoordhuis
Copy link
Member

Why is trace_event.h included in node.h in the first place? I'd think node_internals.h is a better, more logical place for it.

@matthewloring
Copy link

It was included so the trace event macros could be used throughout core. If node_internals.h is a better place we can move it there. The contents of trace_event.h should be currently unused so it can be moved to a different place without any other refactoring.

@bnoordhuis
Copy link
Member

#10959

@richardlau
Copy link
Member Author

Superseded by #10959.

@richardlau richardlau closed this Jan 23, 2017
@richardlau richardlau deleted the node-headers branch January 23, 2017 14:31
@richardlau richardlau restored the node-headers branch January 23, 2017 18:29
@mscdex mscdex added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Jan 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants