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.

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

Revert "deps: update V8 to 6.2.414.33" #16513

Closed
wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Oct 26, 2017

This reverts commit d4033c1.

Ref: #16412

The commit broke cross-compilation and I missed it: https://ci.nodejs.org/job/node-test-commit/13396/

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

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Oct 26, 2017
@targos targos mentioned this pull request Oct 26, 2017
2 tasks
@targos
Copy link
Member Author

targos commented Oct 26, 2017

CI is green. Landed in 6645126

@targos targos closed this Oct 26, 2017
@targos targos deleted the revert-v8-update branch October 26, 2017 12:43
targos added a commit that referenced this pull request Oct 26, 2017
This reverts commit d4033c1.

The commit broke cross-compilation and it was missed.

PR-URL: #16513
Refs: #16412
Reviewed-By: Ben Noordhuis <[email protected]>
@seishun
Copy link
Contributor

seishun commented Oct 26, 2017

I investigated this before it got reverted. Seems to be a problem on V8's side.

  1. msbuild tries to build v8_dump_build_config.vcxproj.
  2. The project calls call call python "..\tools\testrunner\utils\dump_build_config_gyp.py" "C:\Users\Nikolai\Downloads\node\deps\v8\src\Debug\v8_build_config.json" "dcheck_always_on=0" "is_asan=0" "is_cfi=0" "is_component_build="static_library"" "is_debug="Debug"" "is_gcov_coverage=0" "is_msan=0" "is_tsan=0" "is_ubsan_vptr=0" "target_cpu="x64"" "v8_enable_i18n_support=1" "v8_target_cpu="x64"" "v8_use_snapshot=true"
  3. cmd removes extra quotes and the 5th argument ends up as is_component_build=static_library.
  4. This line falls back to static_library since GYP_GN_CONVERSION only has "static_library".
  5. json.loads throws since static_library isn't valid JSON.

The action is defined here. Looks good to me. Apparently it's GYP that doesn't generate the cmd line properly. Yet another GYP bug.

Perhaps it's time to revisit moving off GYP?

addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
This reverts commit d4033c15475ff854b645751025135f7899890fcd.

The commit broke cross-compilation and it was missed.

PR-URL: nodejs/node#16513
Refs: nodejs/node#16412
Reviewed-By: Ben Noordhuis <[email protected]>
@seishun seishun mentioned this pull request Oct 30, 2017
2 tasks
@gibfahn
Copy link
Member

gibfahn commented Oct 30, 2017

Marked dont-land-on-v8.x as the original commit was don't land.

@refack
Copy link
Contributor

refack commented Oct 30, 2017

The action is defined here. Looks good to me. Apparently it's GYP that doesn't generate the cmd line properly. Yet another GYP bug.

IMHO it's not a GYP bug, but rather a cmd weirdness, even if I set msvs_quote_cmd: 0 and get

python ..\tools\testrunner\utils\dump_build_config_gyp.py D:\code\node-github-desktop\deps\v8\src\Debug\v8_build_config.json dcheck_always_on=0 is_asan=0 is_cfi=0 is_component_build="static_library" is_debug="Debug" is_gcov_coverage=0 is_msan=0 is_tsan=0 is_ubsan_vptr=0 target_cpu="x64" v8_enable_i18n_support=1 v8_target_cpu="x64" v8_use_snapshot=true

(no double quotes) it doesn't work.

tl;dr this is just a bug in the .gyp file and the accompanying v8/tools/testrunner/utils/dump_build_config_gyp.py (expecting shell parameter to keep " is super fragile)

@targos
Copy link
Member Author

targos commented Oct 31, 2017

I propose that we simply don't apply this change. It is not a problem in V8 master and I don't think we need it (?)

@seishun
Copy link
Contributor

seishun commented Oct 31, 2017

IMHO it's not a GYP bug, but rather a cmd weirdness

cmd has had this weirdness for much longer than GYP has existed, and it's probably documented somewhere. It's GYP's job to work around it (by escaping quotes when necessary).

tl;dr this is just a bug in the .gyp file and the accompanying v8/tools/testrunner/utils/dump_build_config_gyp.py (expecting shell parameter to keep " is super fragile)

Disagree. The .gyp file should be shell-agnostic. It's possible to pass quotes in command line arguments via cmd, so the behavior of dump_build_config_gyp.py is perfectly reasonable too.

@seishun
Copy link
Contributor

seishun commented Nov 1, 2017

@targos

It is not a problem in V8 master

Is it not? I just checked out V8 and tried to build it on Windows, got that JSON error.

I think this should be reported in either V8's or GYP's issue tracker.

addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
This reverts commit d4033c15475ff854b645751025135f7899890fcd.

The commit broke cross-compilation and it was missed.

PR-URL: nodejs/node#16513
Refs: nodejs/node#16412
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants