FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: fix etw provider include on Windows by joyeecheung · Pull Request #16639 · nodejs/node · GitHub

/ node Public

src: fix etw provider include on Windows - #16639

Closed
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:fix-include
ConversationCommits (1)ChecksFiles changed
Closed

src: fix etw provider include on Windows#16639
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:fix-include

Conversation

joyeecheung commented Oct 31, 2017

Copy link
Copy Markdown
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 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 commented Oct 31, 2017

Copy link
Copy Markdown
Member Author

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

joyeecheung commented Oct 31, 2017
View reviewed changes
Comment thread src/node_win32_etw_provider.cc
// 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"

joyeecheung Oct 31, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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..

joyeecheung Oct 31, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Oct 31, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 commented Oct 31, 2017

Copy link
Copy Markdown
Member Author

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

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

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell commented Oct 31, 2017

Copy link
Copy Markdown
Member

(moving ahead with landing)

jasnell pushed a commit that referenced this pull request Oct 31, 2017
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 commented Oct 31, 2017

Copy link
Copy Markdown
Member

Landed in dfcaf28

jasnell closed this Oct 31, 2017
jasnell mentioned this pull request Oct 31, 2017
Closed
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
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
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 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
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 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
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 mentioned this pull request Nov 6, 2017
Merged
gibfahn pushed a commit that referenced this pull request Nov 14, 2017
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 commented Nov 14, 2017

Copy link
Copy Markdown
Contributor

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

joyeecheung commented Nov 14, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

@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
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
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 mentioned this pull request Nov 21, 2017
Merged
gibfahn mentioned this pull request Nov 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
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
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
cjihrig cjihrig 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

Back | FazBrowse Home | New Git URL