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

Revert #16548 by seishun · Pull Request #16622 · nodejs/node · GitHub

/ node Public

Revert #16548 - #16622

Closed
seishun wants to merge 2 commits into
nodejs:masterfrom
seishun:revert-16548
ConversationCommits (2)ChecksFiles changed
Closed

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

Conversation

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

Copy link
Copy Markdown
Contributor Author

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

refack commented Oct 30, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

refack commented Oct 30, 2017

Copy link
Copy Markdown
Contributor

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

seishun commented Oct 30, 2017

Copy link
Copy Markdown
Contributor Author

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

addaleax mentioned this pull request Oct 30, 2017
Merged
2 tasks

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.

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

joyeecheung 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

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

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

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

Copy link
Copy Markdown
Member

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

seishun commented Oct 31, 2017

Copy link
Copy Markdown
Contributor Author

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

joyeecheung commented Oct 31, 2017

Copy link
Copy Markdown
Member

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

joyeecheung mentioned this pull request Oct 31, 2017
Closed
4 tasks

seishun commented Oct 31, 2017

Copy link
Copy Markdown
Contributor Author

No longer needed.

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

cjihrig cjihrig approved these changes
joyeecheung joyeecheung approved these changes
gireeshpunathil gireeshpunathil approved these changes
+2 more reviewers
refack refack approved these changes
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

Back | FazBrowse Home | New Git URL