Skip to content

build: disable custom v8 snapshot by default #27365

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.

Sign up for GitHub

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

Jump to bottom
Closed
joyeecheung wants to merge 1 commit into from
Closed

build: disable custom v8 snapshot by default #27365

joyeecheung wants to merge 1 commit into from

Conversation

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Apr 23, 2019 •
edited
Loading

This currently breaks test/pummel/test-hash-seed.js

Refs: #27321

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@joyeecheung
build: disable custom v8 snapshot by default
4c3c877 
This currently breaks `test/pummel/test-hash-seed.js`
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 23, 2019

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3339

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 23, 2019
@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 23, 2019

pummel/test-hash-seed on master https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/6416/
pummel/test-hash-seed for this PR https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/6417/

@joyeecheung joyeecheung added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 23, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 23, 2019

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

refack
refack approved these changes Apr 23, 2019
View reviewed changes
@refack

This comment has been minimized.

Sign in to view
@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 23, 2019

Please thumb this up if you are +1 to fast track. This breaks pummel/test-hash-seed on master (which breaks the daily build).

@joyeecheung joyeecheung mentioned this pull request Apr 23, 2019
Closed
ryzokuken
ryzokuken approved these changes Apr 23, 2019
View reviewed changes
Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 23, 2019

This fixed the regression on master as shown in #27365 (comment) . Landing..

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 23, 2019

Landed in a41a4ee

joyeecheung added a commit that referenced this pull request Apr 23, 2019
@joyeecheung
build: disable custom v8 snapshot by default
a41a4ee 
This currently breaks `test/pummel/test-hash-seed.js`

PR-URL: #27365
Refs: #27321
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
targos pushed a commit that referenced this pull request Apr 27, 2019
@joyeecheung @targos
build: disable custom v8 snapshot by default
5023446 
This currently breaks `test/pummel/test-hash-seed.js`

PR-URL: #27365
Refs: #27321
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
@targos targos mentioned this pull request Apr 27, 2019
Merged
joyeecheung added a commit to joyeecheung/v8 that referenced this pull request May 9, 2019
@joyeecheung
[api] Implement StartupData::CanBeRehashed() for the snapshot blob
e0a109c 
This enables the embedder to check if the snapshot generated
from SnapshotCreator::CreateBlob() can be rehashed and the seed
can be recomputed during deserialization.

The lack of this functionality resulted in a temporary vunerability
in Node.js: nodejs/node#27365

Change-Id: I88d52337217c40f79c26438be3c87d2db874d980
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1578661
Commit-Queue: Joyee Cheung <[email protected]>
Reviewed-by: Yang Guo <[email protected]>
Cr-Commit-Position: refs/heads/master@{#61175}
joyeecheung added a commit to joyeecheung/node that referenced this pull request Jun 6, 2019
@joyeecheung
deps: V8: cherry-pick e0a109c
2702abb 
Original commit message:

    [api] Implement StartupData::CanBeRehashed() for the snapshot blob

    This enables the embedder to check if the snapshot generated
    from SnapshotCreator::CreateBlob() can be rehashed and the seed
    can be recomputed during deserialization.

    The lack of this functionality resulted in a temporary vunerability
    in Node.js: nodejs#27365

    Change-Id: I88d52337217c40f79c26438be3c87d2db874d980
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1578661
    Commit-Queue: Joyee Cheung <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#61175}

Refs: v8/v8@e0a109c
joyeecheung added a commit that referenced this pull request Jun 11, 2019
@joyeecheung
deps: V8: cherry-pick e0a109c
d2634be 
Original commit message:

    [api] Implement StartupData::CanBeRehashed() for the snapshot blob

    This enables the embedder to check if the snapshot generated
    from SnapshotCreator::CreateBlob() can be rehashed and the seed
    can be recomputed during deserialization.

    The lack of this functionality resulted in a temporary vunerability
    in Node.js: #27365

    Change-Id: I88d52337217c40f79c26438be3c87d2db874d980
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1578661
    Commit-Queue: Joyee Cheung <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#61175}

Refs: v8/v8@e0a109c

PR-URL: #27533
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@joyeecheung joyeecheung mentioned this pull request Jun 11, 2019
Closed
2 tasks
joyeecheung added a commit that referenced this pull request Jun 17, 2019
@joyeecheung
tools: assert that the snapshot can be rehashed in node_mksnapshot
b6e41f0 
PR-URL: #28181
Refs: #27365
Refs: #17058
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
joyeecheung added a commit that referenced this pull request Jun 17, 2019
@joyeecheung
build: turn on custom V8 snapshot by default
28d3f19 
This patch re-enables custom V8 snapshot integration. Also renames
the experimental configure switch from `--with-node-snapshot`
to `--without-node-snapshot` to be consistent with`--without-snapshot`
which is used for the default V8 snapshot.

PR-URL: #28181
Refs: #27365
Refs: #17058
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
@joyeecheung @BridgeAR
deps: V8: cherry-pick e0a109c
9382b3b 
Original commit message:

    [api] Implement StartupData::CanBeRehashed() for the snapshot blob

    This enables the embedder to check if the snapshot generated
    from SnapshotCreator::CreateBlob() can be rehashed and the seed
    can be recomputed during deserialization.

    The lack of this functionality resulted in a temporary vunerability
    in Node.js: #27365

    Change-Id: I88d52337217c40f79c26438be3c87d2db874d980
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1578661
    Commit-Queue: Joyee Cheung <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#61175}

Refs: v8/v8@e0a109c

PR-URL: #27533
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
@joyeecheung @BridgeAR
tools: assert that the snapshot can be rehashed in node_mksnapshot
9085412 
PR-URL: #28181
Refs: #27365
Refs: #17058
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
@joyeecheung @BridgeAR
build: turn on custom V8 snapshot by default
2201484 
This patch re-enables custom V8 snapshot integration. Also renames
the experimental configure switch from `--with-node-snapshot`
to `--without-node-snapshot` to be consistent with`--without-snapshot`
which is used for the default V8 snapshot.

PR-URL: #28181
Refs: #27365
Refs: #17058
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jun 18, 2019
@joyeecheung @targos
tools: assert that the snapshot can be rehashed in node_mksnapshot
aeda0c3 
PR-URL: #28181
Refs: #27365
Refs: #17058
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jun 18, 2019
@joyeecheung @targos
build: turn on custom V8 snapshot by default
32b0803 
This patch re-enables custom V8 snapshot integration. Also renames
the experimental configure switch from `--with-node-snapshot`
to `--without-node-snapshot` to be consistent with`--without-snapshot`
which is used for the default V8 snapshot.

PR-URL: #28181
Refs: #27365
Refs: #17058
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@refack refack refack approved these changes

@devsnek devsnek devsnek approved these changes

@ryzokuken ryzokuken ryzokuken approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@joyeecheung @nodejs-github-bot @refack @devsnek @ryzokuken