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

src: fix etw provider include on Windows #16639

Closed
wants to merge 1 commit into from
Closed

src: fix etw provider include on Windows #16639

wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

src

#16548 landed despite compilation errors on Windows. This attempts to fix that. If this fix works then we don't have to revert it (#16622)

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. windows Issues and PRs related to the Windows platform. labels Oct 31, 2017
@joyeecheung
Copy link
Member Author

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

joyeecheung
joyeecheung commented Oct 31, 2017
View reviewed changes
src/node_win32_etw_provider.cc
@@ -19,6 +19,7 @@
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

#include "node_win32_etw_provider.h"
Copy link
Member Author

@joyeecheung joyeecheung Oct 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Oddly, this does not work if I put node_win32_etw_provider.h behind node_etw_provider.h, but node_etw_provider.h is generated with a manifest and from what I can tell it only contains a bunch of constants, so should be self-contained..

Copy link
Member Author

Choose a reason for hiding this comment

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

Which reminds me if the self-contain-ness can't be fixed properly then I will have to turn clang-format off here in #16122 ...

@joyeecheung
Copy link
Member Author

joyeecheung commented Oct 31, 2017 •
edited
Loading

Tests are still runing, but looks like they all compile on Windows now https://ci.nodejs.org/job/node-compile-windows/13057/ , I want to fast-track this to get the master back to working on Windows once the tests are done

@joyeecheung
Copy link
Member Author

CI looks green, all failures are the flaky test-async-wrap-uncaughtexception...

@addaleax addaleax mentioned this pull request Oct 31, 2017
Merged
jasnell
jasnell approved these changes Oct 31, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

This needs to be fast tracked as it is holding up the 9.0.0 release

@jasnell
Copy link
Member

jasnell commented Oct 31, 2017

(moving ahead with landing)

jasnell pushed a commit that referenced this pull request Oct 31, 2017
@joyeecheung @jasnell
src: fix etw provider include on Windows
dfcaf28 
PR-URL: #16639
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 31, 2017

Landed in dfcaf28

@jasnell jasnell closed this Oct 31, 2017
@jasnell jasnell mentioned this pull request Oct 31, 2017
Closed
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@joyeecheung @Qard
src: fix etw provider include on Windows
614505d 
PR-URL: nodejs/node#16639
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@joyeecheung @Qard
src: fix etw provider include on Windows
5c4c654 
PR-URL: nodejs/node#16639
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn gibfahn mentioned this pull request Nov 2, 2017
Closed
4 tasks
joyeecheung added a commit to joyeecheung/node that referenced this pull request Nov 2, 2017
@joyeecheung
src: fix etw provider include on Windows
d735be9 
PR-URL: nodejs#16639
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joyeecheung joyeecheung mentioned this pull request Nov 3, 2017
Closed
4 tasks
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
@joyeecheung @cjihrig
src: fix etw provider include on Windows
c4736cf 
PR-URL: nodejs#16639
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
Merged
gibfahn pushed a commit that referenced this pull request Nov 14, 2017
@joyeecheung @gibfahn
src: fix etw provider include on Windows
cecd1e3 
PR-URL: #16639
Backport-PR-URL: #16609
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

@joyeecheung I've set this to not land on v6.x. Please lmk if this is a mistake

@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 14, 2017 •
edited
Loading

@MylesBorins I would need to update #16610 to include this patch on v6.x

joyeecheung added a commit to joyeecheung/node that referenced this pull request Nov 15, 2017
@joyeecheung
src: fix etw provider include on Windows
00bd0c8 
PR-URL: nodejs#16639
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
@joyeecheung @MylesBorins
src: fix etw provider include on Windows
c8cf488 
Backport-PR-URL: #16610
PR-URL: #16639
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
Merged
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
@joyeecheung @MylesBorins
src: fix etw provider include on Windows
b5a4717 
Backport-PR-URL: #16610
PR-URL: #16639
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
@joyeecheung @MylesBorins
src: fix etw provider include on Windows
42cb64e 
Backport-PR-URL: #16610
PR-URL: #16639
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@joyeecheung @addaleax
src: fix etw provider include on Windows
47c8eb0 
PR-URL: nodejs/node#16639
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@joyeecheung @jasnell @MylesBorins @addaleax @cjihrig @nodejs-github-bot