-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
deps: backport bc2e393 from v8 upstream #3792
Conversation
LGTM |
1 similar comment
LGTM |
line); | ||
|
||
if (match): | ||
klass = match.group(1); | ||
klass = match.group(1).rstrip().lstrip(); |
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.
I have to ask: why not just .strip()?
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.
Looking back on it, that is what it probably should have been...
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.
Submitted a patch for the same: https://codereview.chromium.org/1443963004
LGTM |
weird, looks like only arm and windows builds ran? |
ok, trying CI again https://ci.nodejs.org/job/node-test-pull-request/726/ |
@joaocgreis same problem as yesterday, something's up with Jenkins, perhaps it's more complicated than just a wonky freshness detection? |
I guess I'll hold off on landing until we get it sorted out? |
Yes please @evanlucas, also @nodejs/collaborators please note that CI is not currently doing full runs, until you get a full run passing for your tests you won't have full assurance that your changes can land. The @nodejs/build team is working on resolving this but we're fighting with Jenkins on this and it's a bit like playing wac-a-mole while riding a bucking bull. |
Cool. Thanks. Let me know if I can help in any way :] |
@evanlucas CI should be good again, started a run for this PR: https://ci.nodejs.org/job/node-test-pull-request/731/ |
Original commit message: [tools] Make gen-postmortem-metadata.py more reliable Instead of basing matches off of whitespace, walk the inheritance chain and include any classes that inherit from Object. R=machenbach@chromium.org,jkummerow@chromium.org NOTRY=true Review URL: https://codereview.chromium.org/1435643002 Cr-Commit-Position: refs/heads/master@{nodejs#31964} This adds some missing classes to postmortem info like JSMap and JSSet. PR-URL: nodejs#3792 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Thanks! Landed in 70405d4 |
Original commit message: [tools] Make gen-postmortem-metadata.py more reliable Instead of basing matches off of whitespace, walk the inheritance chain and include any classes that inherit from Object. R=machenbach@chromium.org,jkummerow@chromium.org NOTRY=true Review URL: https://codereview.chromium.org/1435643002 Cr-Commit-Position: refs/heads/master@{#31964} This adds some missing classes to postmortem info like JSMap and JSSet. PR-URL: #3792 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Original commit message: [tools] Make gen-postmortem-metadata.py more reliable Instead of basing matches off of whitespace, walk the inheritance chain and include any classes that inherit from Object. R=machenbach@chromium.org,jkummerow@chromium.org NOTRY=true Review URL: https://codereview.chromium.org/1435643002 Cr-Commit-Position: refs/heads/master@{nodejs#31964} This adds some missing classes to postmortem info like JSMap and JSSet. PR-URL: nodejs#3792 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Original commit message: [tools] Make gen-postmortem-metadata.py more reliable Instead of basing matches off of whitespace, walk the inheritance chain and include any classes that inherit from Object. R=machenbach@chromium.org,jkummerow@chromium.org NOTRY=true Review URL: https://codereview.chromium.org/1435643002 Cr-Commit-Position: refs/heads/master@{nodejs#31964} This adds some missing classes to postmortem info like JSMap and JSSet. Ref: nodejs#3792 PR-URL: nodejs#4106 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com> Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
Original commit message: [tools] Make gen-postmortem-metadata.py more reliable Instead of basing matches off of whitespace, walk the inheritance chain and include any classes that inherit from Object. R=machenbach@chromium.org,jkummerow@chromium.org NOTRY=true Review URL: https://codereview.chromium.org/1435643002 Cr-Commit-Position: refs/heads/master@{#31964} This adds some missing classes to postmortem info like JSMap and JSSet. Ref: #3792 PR-URL: #4106 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com> Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
I'm going to add LTS watch to this and backport it when we do the next batch @nodejs/collaborators last chance to veto |
@nodejs/lts |
looks OK to me for LTS I think, would love input from @nodejs/post-mortem wrt v4 support for this and how much it actually matters. |
perhaps @mhdawson has some thoughts |
What's the question with respect to postmortem? |
Original commit message: [tools] Make gen-postmortem-metadata.py more reliable Instead of basing matches off of whitespace, walk the inheritance chain and include any classes that inherit from Object. R=machenbach@chromium.org,jkummerow@chromium.org NOTRY=true Review URL: https://codereview.chromium.org/1435643002 Cr-Commit-Position: refs/heads/master@{nodejs#31964} This adds some missing classes to postmortem info like JSMap and JSSet. Ref: nodejs#3792 PR-URL: nodejs#4106 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com> Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
Original commit message: [tools] Make gen-postmortem-metadata.py more reliable Instead of basing matches off of whitespace, walk the inheritance chain and include any classes that inherit from Object. R=machenbach@chromium.org,jkummerow@chromium.org NOTRY=true Review URL: https://codereview.chromium.org/1435643002 Cr-Commit-Position: refs/heads/master@{nodejs#31964} This adds some missing classes to postmortem info like JSMap and JSSet. Ref: nodejs#3792 PR-URL: nodejs#4106 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com> Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
@davepacheco if this should be backported to v4 |
I'm not aware of postmortem tools that use JSMap or JSSet. If those are the only affected classes, I don't think this would matter. But then, I don't know how anybody would have noticed in the first place, so maybe something does use these? Or maybe there's some other metadata that's affected? (Edited to clarify I was talking about JSMap, not Map.) |
@thealphanerd the IBM tools don't currently use that port mortem data so not needed from that perspective. I know that @yunong does use the mdb port mortem tools and mentioned above that he'd like this to be landed so maybe he can comment on the need to backport. |
I noticed it when using llnode. |
ping @indutny re: llnode |
llnode doesn't really have support for this, at least not yet. +0 from me |
@nodejs/collaborators does anyone have an interest in this being backported? |
Not me. |
hmm.. I'm +0 on it I think. |
Original commit message: [tools] Make gen-postmortem-metadata.py more reliable Instead of basing matches off of whitespace, walk the inheritance chain and include any classes that inherit from Object. R=machenbach@chromium.org,jkummerow@chromium.org NOTRY=true Review URL: https://codereview.chromium.org/1435643002 Cr-Commit-Position: refs/heads/master@{#31964} This adds some missing classes to postmortem info like JSMap and JSSet. PR-URL: #3792 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
I've gone ahead and landed this on staging. If anyone has any objections please feel free to make them here or on the proposal issue that will be incoming tomorrow |
Original commit message: [tools] Make gen-postmortem-metadata.py more reliable Instead of basing matches off of whitespace, walk the inheritance chain and include any classes that inherit from Object. R=machenbach@chromium.org,jkummerow@chromium.org NOTRY=true Review URL: https://codereview.chromium.org/1435643002 Cr-Commit-Position: refs/heads/master@{#31964} This adds some missing classes to postmortem info like JSMap and JSSet. PR-URL: #3792 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Original commit message: [tools] Make gen-postmortem-metadata.py more reliable Instead of basing matches off of whitespace, walk the inheritance chain and include any classes that inherit from Object. R=machenbach@chromium.org,jkummerow@chromium.org NOTRY=true Review URL: https://codereview.chromium.org/1435643002 Cr-Commit-Position: refs/heads/master@{#31964} This adds some missing classes to postmortem info like JSMap and JSSet. PR-URL: #3792 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This LTS release comes with 89 commits. This includes 46 commits that are docs related, 11 commits that are test related, 8 commits that are build related, and 4 commits that are benchmark related. Notable Changes: - debugger: - All properties of an array (aside from length) can now be printed in the repl (cjihrig) #6448 - npm: - Upgrade npm to 2.15.8 (Rebecca Turner) #7412 - stream: - Fix for a bug that became more prevalent with the stream changes that landed in v4.4.5. (Anna Henningsen) #7160 - V8: - Fix for a bug in crankshaft that was causing crashes on arm64 (Myles Borins) #7442 - Add missing classes to postmortem info such as JSMap and JSSet (evan.lucas) #3792
This LTS release comes with 89 commits. This includes 46 commits that are docs related, 11 commits that are test related, 8 commits that are build related, and 4 commits that are benchmark related. Notable Changes: - debugger: - All properties of an array (aside from length) can now be printed in the repl (cjihrig) #6448 - npm: - Upgrade npm to 2.15.8 (Rebecca Turner) #7412 - stream: - Fix for a bug that became more prevalent with the stream changes that landed in v4.4.5. (Anna Henningsen) #7160 - V8: - Fix for a bug in crankshaft that was causing crashes on arm64 (Myles Borins) #7442 - Add missing classes to postmortem info such as JSMap and JSSet (evan.lucas) #3792
Original commit message: [tools] Make gen-postmortem-metadata.py more reliable Instead of basing matches off of whitespace, walk the inheritance chain and include any classes that inherit from Object. R=machenbach@chromium.org,jkummerow@chromium.org NOTRY=true Review URL: https://codereview.chromium.org/1435643002 Cr-Commit-Position: refs/heads/master@{#31964} This adds some missing classes to postmortem info like JSMap and JSSet. PR-URL: nodejs/node#3792 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Original commit message:
This adds some missing classes to postmortem info like
JSMap and JSSet.
R=@indutny