Skip to content

tools: delete v8_external_snapshot.gypi #29369

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 3 commits into from

Conversation

ryzokuken
Copy link
Contributor

Delete the v8_external_snapshot target from gyp and disable the
v8_use_external_startup_data option since it is never used anyway.

Refs: #29363 (comment)
Fixes: #28964

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

/cc @gengjiawen @bnoordhuis

Delete the v8_external_snapshot target from gyp and disable the
v8_use_external_startup_data option since it is never used anyway.

Refs: nodejs#29363 (comment)
Fixes: nodejs#28964
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Aug 29, 2019
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with the suggestion to get rid of the v8_use_external_startup_data variable altogether.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

👍

@nodejs-github-bot
Copy link
Collaborator

@ryzokuken
Copy link
Contributor Author

If CI passes as expected, this should be ready to land by Monday. Thanks all!

@targos
Copy link
Member

targos commented Aug 31, 2019

@ryzokuken looks like it needs some more work

@ryzokuken
Copy link
Contributor Author

Done! Will land after @bnoordhuis reviews.

@gengjiawen
Copy link
Member

Travis failed with

gyp: name 'v8_use_external_startup_data' is not defined while evaluating condition 'v8_use_external_startup_data' in /home/travis/build/nodejs/node/tools/v8_gypfiles/v8.gyp while loading dependencies of /home/travis/build/nodejs/node/node.gyp while trying to load /home/travis/build/nodejs/node/node.gyp
Error running GYP
The command "./configure" exited with 1.

@bnoordhuis
Copy link
Member

There's one more if-then-else clause in v8.gyp that can be simplified to just the else block.

@ryzokuken
Copy link
Contributor Author

@bnoordhuis @gengjiawen Ah! Must've missed that. Taking caring of it.

@ryzokuken
Copy link
Contributor Author

@bnoordhuis PTAL? I suppose this fixes everything.

@nodejs-github-bot
Copy link
Collaborator

ryzokuken added a commit that referenced this pull request Sep 5, 2019
Delete the v8_external_snapshot target from gyp and disable the
v8_use_external_startup_data option since it is never used anyway.

Refs: #29363 (comment)
Fixes: #28964

PR-URL: #29369
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@ryzokuken
Copy link
Contributor Author

Landed in 2882ce9 🎉

@ryzokuken ryzokuken closed this Sep 5, 2019
targos pushed a commit that referenced this pull request Sep 20, 2019
Delete the v8_external_snapshot target from gyp and disable the
v8_use_external_startup_data option since it is never used anyway.

Refs: #29363 (comment)
Fixes: #28964

PR-URL: #29369
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V8 build config
6 participants