-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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.
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
test: Revert add option to use node in path #10613
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically, revert commits are like Revert "<commit message>"
. I think changelog-maker even expects them to be that way.
@evanlucas But that exceeds the 50 chars limit. Is that okay? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @evanlucas's comemnt, this should be made via git revert
unless there is some other issue.
Please merge some form of this ASAP.
@thefourtheye I think so for reverts. I would wager it's happened before. |
@thefourtheye yes. It should just be with |
This reverts commit 4198253.
9cc20d1
to
a3c8fac
Compare
@evanlucas @Fishrock123 Thanks 👍 Updated the commit. |
Oy good catch. Missed that entirely on my initial review. LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert looks identical to git revert
. +1 to landing straightaway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Also, +1 to landing sooner than the 48 minimum
Landed in 26bf954 |
This reverts commit 4198253. PR-URL: #10613 Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post-merge approval. Can confirm.
This reverts commit 4198253. PR-URL: nodejs#10613 Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
tools
Description
This reverts commit 4198253, landed in
#9674.
This fails at
File "tools/test.py", line 1429, in ProcessOptions
options.j = int(cores) if cores is not None else
multiprocessing.cpu_count()
with the error
NotImplementedError: cannot determine number of cpus
from
File "...multiprocessing/init.py", line 136, in cpu_count
raise NotImplementedError('cannot determine number of cpus')
on MacOS machines.