Skip to content

Revert #16548 - #16622

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

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

Conversation

@seishun

@seishunseishun commented Oct 30, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

#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-botnodejs-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

seishun commented Oct 30, 2017

Copy link
Copy Markdown
ContributorAuthor

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

@refack

refack commented Oct 30, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@refack

refack commented Oct 30, 2017

Copy link
Copy Markdown
Contributor

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

@seishun

seishun commented Oct 30, 2017

Copy link
Copy Markdown
ContributorAuthor

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

refack commented Oct 30, 2017

Copy link
Copy Markdown
Contributor

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.

@addaleaxaddaleax mentioned this pull request Oct 30, 2017
Merged
2 tasks
@gibfahn

gibfahn commented Oct 30, 2017

Copy link
Copy Markdown
Member

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.

@gibfahngibfahn mentioned this pull request Oct 30, 2017
Closed
4 tasks
joyeecheung
joyeecheung approved these changes Oct 31, 2017
View reviewed changes

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

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

@joyeecheung

joyeecheung commented Oct 31, 2017

Copy link
Copy Markdown
Member

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

joyeecheung commented Oct 31, 2017

Copy link
Copy Markdown
Member

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

joyeecheung commented Oct 31, 2017

Copy link
Copy Markdown
Member

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

@seishun

seishun commented Oct 31, 2017

Copy link
Copy Markdown
ContributorAuthor

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

@joyeecheung

joyeecheung commented Oct 31, 2017

Copy link
Copy Markdown
Member

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

@joyeecheungjoyeecheung mentioned this pull request Oct 31, 2017
Closed
4 tasks
@seishun

seishun commented Oct 31, 2017

Copy link
Copy Markdown
ContributorAuthor

No longer needed.

@seishunseishun closed this Oct 31, 2017
@seishun
seishun deleted the revert-16548 branch October 31, 2017 16:37
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrigcjihrigcjihrig approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes
@gireeshpunathilgireeshpunathilgireeshpunathil approved these changes
+2 more reviewers
@refackrefackrefack approved these changes
@gibfahngibfahngibfahn 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 / srcIssues 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