Skip to content
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

V8 build config #28964

Closed
gengjiawen opened this issue Aug 5, 2019 · 5 comments
Closed

V8 build config #28964

gengjiawen opened this issue Aug 5, 2019 · 5 comments

Comments

@gengjiawen
Copy link
Member

gengjiawen commented Aug 5, 2019 •
edited
Loading

Issue one

This file no longer exists. Maybe replaced with this file special-case.h ?

node/deps/v8/BUILD.gn

Line 2731 in 2dcc366

"src/regexp/regexp-special-case.h",

looks like an issue from upstream.
https://github.com/v8/v8/blob/0d07272a7fac4d5e84c9aa5dd96e6dbce6e9d673/BUILD.gn#L2749

cc @nodejs/v8 @targos @hashseed

Issue two

The file has moved to src/init

node/tools/v8_gypfiles/v8_external_snapshot.gypi

Line 39 in 3b5773f

'<(V8_ROOT)/src/setup-isolate-deserialize.cc',

cc @nodejs/gyp
@gengjiawen gengjiawen changed the title V8 gn config V8 build config Aug 5, 2019
@ryzokuken
Copy link
Contributor

@gengjiawen I think I'm qualified to answer this.

  1. While this might be a legitimate issue, we do not use or own the BUILD.gn files since they're a part of the V8 project. If you think it's problematic that they include that file, please feel free to create an issue upstream at https://bugs.chromium.org/p/v8/issues/entry.

  2. The gyp files we do infact own, and this might have slipped through one of our updates. That said, this neither breaks nor causes any other problems with the build, and I'd rather not make any changes to our gypfiles unless they break something.

@gengjiawen
Copy link
Member Author

@refack Is v8_external_snapshot.gypi still in use ?

@ryzokuken
Copy link
Contributor

@gengjiawen @refack seems to be on a break. And yes, it's still used. I'll make a PR to fix it now.

@ryzokuken
Copy link
Contributor

I'm sorry, literally the first line of the file says # Keeping this separate since Node.js does use it. Let me try to build after removing the file, see if that works.

ryzokuken added a commit to ryzokuken/node that referenced this issue Aug 29, 2019
@ryzokuken
tools: update paths in v8_external_snapshot.gypi
db509c4 
Update the paths to source files in v8_external_snapshot.gypi this was
broken and went unnoticed since this target is never really built by
default.

Fixes: nodejs#28964
@ryzokuken ryzokuken mentioned this issue Aug 29, 2019
Closed
4 tasks
ryzokuken added a commit to ryzokuken/node that referenced this issue Aug 29, 2019
@ryzokuken
tools: delete v8_external_snapshot.gypi
5adb3c1 
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
@ryzokuken ryzokuken mentioned this issue Aug 29, 2019
Closed
4 tasks
targos pushed a commit that referenced this issue Sep 20, 2019
@ryzokuken @targos
tools: delete v8_external_snapshot.gypi
9de417a 
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>
@gengjiawen
Copy link
Member Author

Looks like GN is improving on this

https://bugs.chromium.org/p/chromium/issues/detail?id=991547

#29694 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@gengjiawen @ryzokuken