Skip to content

Revert #16548#16622

Closed
seishun wants to merge 2 commits intonodejs:masterfrom
seishun:revert-16548
Closed

Revert #16548#16622
seishun wants to merge 2 commits intonodejs:masterfrom
seishun:revert-16548

Conversation

@seishun
Copy link
Contributor

@seishun seishun commented Oct 30, 2017 •
edited
Loading

#16548 was landed despite failing on Windows (see https://ci.nodejs.org/job/node-compile-windows/12916/). Reverting to make CI green again.

According to https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#reverting-commits, I should include the reason for reverting in the commit message, but it doesn't say what to do when there are multiple commits.

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

doc, src

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 30, 2017
@seishun
Copy link
Contributor Author

seishun commented Oct 30, 2017

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

@refack
Copy link
Contributor

refack commented Oct 30, 2017 •
edited
Loading

+1 for fast tracking so that CI is back to green.

@refack
Copy link
Contributor

refack commented Oct 30, 2017

/cc @nodejs/release RE: how to format the message

@seishun
Copy link
Contributor Author

seishun commented Oct 30, 2017

I blame nodejs/build#790 for letting this happen by the way. A big red cross is much more obvious than a URL buried somewhere in the conversation.

@refack
Copy link
Contributor

refack commented Oct 30, 2017

I blame nodejs/build#790 for letting this happen by the way. A big red cross is much more obvious than a URL buried somewhere in the conversation.

I also assume nodejs/build#952, Windows CI was red most weekend.

@addaleax addaleax mentioned this pull request Oct 30, 2017
Merged
2 tasks
@gibfahn
Copy link
Member

gibfahn commented Oct 30, 2017

According to /COLLABORATOR_GUIDE.md@master#reverting-commits, I should include the reason for reverting in the commit message, but it doesn't say what to do when there are multiple commits.

What you did seems fine.

I blame nodejs/build#790 for letting this happen by the way. A big red cross is much more obvious than a URL buried somewhere in the conversation.

Good point, added good first issue and help wanted labels to that issue.

@gibfahn gibfahn mentioned this pull request Oct 30, 2017
Closed
4 tasks
joyeecheung
joyeecheung approved these changes Oct 31, 2017
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Sorry for not checking more thoroughly. Let's fast track this.

@joyeecheung
Copy link
Member

joyeecheung commented Oct 31, 2017

One jenkins build failure on Windows, others are know flaky tests. To be safe: https://ci.nodejs.org/job/node-test-commit-windows-fanned/13030/

@joyeecheung
Copy link
Member

joyeecheung commented Oct 31, 2017

Windows build got aborted, but I guess we only need to make sure that it compiles as before since this is just a revert for compilation errors...

@seishun
Revert "src: do not include x.h if x-inl.h is included"
0c0a570 
This reverts commit cdb263d.

That commit was landed without a green CI and is failing on Windows.
@joyeecheung
Copy link
Member

joyeecheung commented Oct 31, 2017

@seishun You have pushed new commits, did you rebased?

@seishun
Copy link
Contributor Author

seishun commented Oct 31, 2017

@joyeecheung No, I just changed the reason line to match previous revert commits.

@joyeecheung
Copy link
Member

joyeecheung commented Oct 31, 2017

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

@joyeecheung joyeecheung mentioned this pull request Oct 31, 2017
Closed
4 tasks
@seishun
Copy link
Contributor Author

seishun commented Oct 31, 2017

No longer needed.

@seishun seishun closed this Oct 31, 2017
@seishun seishun deleted the revert-16548 branch October 31, 2017 16:37
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

@joyeecheung joyeecheung joyeecheung approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

+2 more reviewers

@refack refack refack approved these changes

@gibfahn gibfahn gibfahn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@seishun @refack @gibfahn @joyeecheung @cjihrig @gireeshpunathil @nodejs-github-bot