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.

Sign up for GitHub

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

Jump to bottom

v7.x: cherry-pick tracing related fixes #11634

Closed
wants to merge 7 commits into from
Closed

v7.x: cherry-pick tracing related fixes #11634

wants to merge 7 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Mar 1, 2017

Refs: #11628

santigimeno and others added 7 commits March 1, 2017 15:37
@santigimeno @targos
tracing: fix -Wreorder warning
cc7f302 
Initialize `InternalTraceBuffer::id_` the last.

PR-URL: nodejs#10416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@santigimeno @targos
tracing: fix -Wunused-private-field warning
4730c42 
Remove `external_buffer_` from `InternalTraceBuffer` as it seems not to
be used anywhere.

PR-URL: nodejs#10416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targos
src: always initialize tracing controller in agent
7fdeeb1 
PR-URL: nodejs#10507
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasongin @targos
src: fix TracingController cleanup
43f55af 
This fixes an incorrect deletion of the `TracingController` instance,
which in some environments could cause an error about an invalid
pointer passed to `free()`. The `TracingController` instance is
actually owned by a `unique_ptr` member of the platform, so calling
`platform::SetTracingController(nullptr)` is the correct way to
delete it. But before that, the `TraceBuffer` must be deleted in
order for the tracing loop to exit; that is accomplished by calling
`TracingController::Initialize(nullptr)`.

PR-URL: nodejs#10623
Reviewed-By: Matthew Loring <mattloring@google.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@danbev @targos
build: add missing src/tracing header files
0eaad57 
I noticed that only one header from src/tracing is included in the
sources list in node.gyp. Not sure if this is intentional or not so I
wanted to bring it up just in case this was overlooked.

PR-URL: nodejs#10851
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@bnoordhuis @targos
src: move trace_event.h include to internal header
70a34da 
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>
@Trott @targos
test: increase strictness for test-trace-event
0842c60 
Change test-trace-event such that it checks that all expected values are
within the same trace object rather than scattered across multiple trace
objects.

PR-URL: nodejs#11065
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos added trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. v7.x labels Mar 1, 2017
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. v7.x labels Mar 1, 2017
@targos targos mentioned this pull request Mar 1, 2017
Closed
@targos
Copy link
Member Author

targos commented Mar 1, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/6640/

Those are all clean cherry-picks and tests pass on my computer.
I'm opening this to run CI.

/cc @nodejs/release @nodejs/ctc
I think we should release a 7.7.1 ASAP because compilation of native addons is broken on 7.7.0.

@JCount JCount mentioned this pull request Mar 1, 2017
Closed
@italoacasas
Copy link
Contributor

Waiting for CI, to start the release

@italoacasas
Copy link
Contributor

@targos any idea how I miss this? CI and CITGM were green on the proposal. Should I be looking for something else?

@targos
Copy link
Member Author

targos commented Mar 1, 2017

@italoacasas See nodejs/testing#15. Perhaps CITGM could have caught this?

@ilovezfs
Copy link

ilovezfs commented Mar 1, 2017

Note that the Homebrew test block caught it:
https://github.com/ilovezfs/homebrew-core/blob/e989c9cc3ba1a91cf26540e54593f0f86c61808b/Formula/node.rb#L126-L149
https://bot.brew.sh/job/Homebrew%20Core/17271/
https://bot.brew.sh/job/Homebrew%20Core/17271/version=sierra/console

@jasnell
Copy link
Member

jasnell commented Mar 1, 2017

@italoacasas .... no worries. We definitely need to make sure that CITGM is set up to catch these kinds of things. This also demonstrates that few of us who do test releases rarely touch native modules in our own tests, which likely needs to be fixed also. Let's definitely make sure 7.7.1 get's out today.

@ilovezfs
Copy link

ilovezfs commented Mar 1, 2017

I'm still seeing failure locally even after rebuilding with this PR in place.
https://gist.github.com/ilovezfs/b87b1b16f10229c1856a02b4512b021f

@targos
Copy link
Member Author

targos commented Mar 1, 2017

@italoacasas Yeah, no worries. I'm absolutely not blaming you here. Something is missing in the process and we will have to find a solution.

@ilovezfs it looks like it's still using 7.7.0 headers somehow: https://gist.github.com/ilovezfs/b87b1b16f10229c1856a02b4512b021f#file-gistfile1-txt-L8513

@ilovezfs
Copy link

ilovezfs commented Mar 1, 2017

@targos it's 7.7.0 with this PR cherry-picked on top. Here's a full log:
https://gist.githubusercontent.com/ilovezfs/cbb12869fdd8b6734d69ae6adedc3c03/raw/276922b1ae0a9e225dc0032a1c2c389f55182715/gistfile1.txt

@addaleax addaleax mentioned this pull request Mar 1, 2017
Closed
@targos
Copy link
Member Author

targos commented Mar 1, 2017

@ilovezfs see https://gist.github.com/ilovezfs/b87b1b16f10229c1856a02b4512b021f#file-gistfile1-txt-L8579-L8580
node-pre-gyp downloads the headers from the release. I don't know if it's possible to configure it to look somewhere else.

@ilovezfs
Copy link

ilovezfs commented Mar 1, 2017

@targos Ah, OK! Thanks for taking a look.

@targos
Copy link
Member Author

targos commented Mar 1, 2017

Failure on pi2:

not ok 113 parallel/test-npm-install
  ---
  duration_ms: 120.286
  severity: fail
  stack: |-
    timeout
  ...

Tests are still running on pi1. The rest is green.

@italoacasas
Copy link
Contributor

@targos good to go?

@addaleax addaleax mentioned this pull request Mar 1, 2017
Closed
@addaleax
Copy link
Member

addaleax commented Mar 1, 2017

Also Fixes: #11627

@mhdawson
Copy link
Member

mhdawson commented Mar 1, 2017

Test failed on arm:

not ok 113 parallel/test-npm-install
  ---
  duration_ms: 120.286
  severity: fail
  stack: |-
    timeout

@mhdawson
Copy link
Member

mhdawson commented Mar 1, 2017

That test does not seem to have anything to do with native modules, so may just have been a flake.

mhdawson
mhdawson approved these changes Mar 1, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

Rubber stamp LGTM as its a clean cherry-pick and the test failure on Arm ran/passed on 2 of the 3 arm variants and the test does not look related to any of the changes being made.

@italoacasas
Copy link
Contributor

@nodejs/collaborators @nodejs/ctc anyone else want to LGTM the backport?

@italoacasas
Copy link
Contributor

Landed.. working in the release.

@italoacasas italoacasas closed this Mar 1, 2017
@targos targos deleted the cherry-pick-tracing-v7 branch March 1, 2017 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Qard Qard Qard approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.