tools: single, cross-platform tick processor #2868
Conversation
|
I guess it can't since this depends on core modules. |
|
Yea, unfortunately v8 doesn't want node specific scripts in their codebase (https://codereview.chromium.org/1179173009/). This change should be forward compatible for the most part since it copies in the logic from the JS scripts in v8. This script would only need updating when v8 processing scripts are added or removed and changes of this nature should be alerted through test/parallel/test-tick-processor |
| @@ -0,0 +1,67 @@ | |||
| // Copyright 2012 the V8 project authors. All rights reserved. | |||
There was a problem hiding this comment.
Does the copyright make sense here? This is all original code, right?
There was a problem hiding this comment.
I originally wasn't sure since I transliterated this from the bash script which was V8's. This is original code though. I've removed the copyright.
| path.join(toolsPath, 'SourceMap.js'), | ||
| path.join(toolsPath, 'tickprocessor-driver.js')]; | ||
|
|
||
| var tempScript = path.join(__dirname, 'tick-processor-input-script-$$'); |
There was a problem hiding this comment.
Unlike in shell, the $$ doesn't get expanded to be the process-id. I think you'll have to manually concatenate process.pid.
|
The previous CI failed to launch due to |
|
One more try, it looks like the CI was going through a spot of trouble: https://ci.nodejs.org/job/node-test-pull-request/313/ |
| path.join(toolsPath, 'tickprocessor-driver.js')]; | ||
|
|
||
| var tempScript = path.join(__dirname, 'tick-processor-tmp-' + process.pid); | ||
| var cat = cp.spawn('cat', scriptFiles); |
There was a problem hiding this comment.
Will cat be available in all the environments?
There was a problem hiding this comment.
Not on windows, it's type there. And why use cat, why not use node to read the files and pipe them to tempScript?
There was a problem hiding this comment.
Good catch! I've rewritten this but am not sure if my solution is idiomatic. @thefourtheye @sam-github Does this solution seem appropriate?
|
@bnoordhuis Do these CI build failures seem to be caused by my PR specifically or are these general? |
|
It's possible I forgot to tell the CI to rebase. Let's give it one more spin: https://ci.nodejs.org/job/node-test-pull-request/325/ |
|
Sigh, Jenkins was just restarted. New run: https://ci.nodejs.org/job/node-test-pull-request/330/ |
Currently there are three separate tick processor scripts for mac, windows, and linux. These have been replaced with a single node.js script to improve maintainability and remove the need to preserve parallel logic in these separate places.
|
Some of the Ubuntu machines are hanging but the CI looks clean otherwise. |
|
Something jenkins, something something. New CI: https://ci.nodejs.org/job/node-test-pull-request/337/ |
|
The armv7-wheezy failure is unrelated. Otherwise that run looks good! |
Currently there are three separate tick processor scripts for mac, windows, and linux. These have been replaced with a single node.js script to improve maintainability and remove the need to preserve parallel logic in these separate places. PR-URL: #2868 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
|
Thanks Matt, landed in e0c3d2a. @thefourtheye You didn't formally LGTM it but I put you down as a reviewer anyway. |
|
@bnoordhuis Oops, sorry. Code looks okay to me, though I didn't get a chance to test it. |
|
What This removes the old scripts entirely but really doesn't seem |
|
The profiler tools don't constitute part of the public API so I would guess |
|
Perhaps we should make those other files just call the new one for 4.x? |
|
@Fishrock123 Files changed in this PR aren't even packaged. Doesn't look like it deserves even a |
Currently there are three separate tick processor scripts for mac, windows, and linux. These have been replaced with a single node.js script to improve maintainability and remove the need to preserve parallel logic in these separate places. PR-URL: #2868 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Currently there are three separate tick processor scripts for
mac, windows, and linux. These have been replaced with a single
node.js script to improve maintainability and remove the need
to preserve parallel logic in these separate places.
/cc @ofrobots @bnoordhuis @Fishrock123 @joaocgreis @Trott