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

make test-v8 is broken on Node.js v8.x #21433

Closed
mmarchini opened this issue Jun 21, 2018 · 22 comments
Closed

make test-v8 is broken on Node.js v8.x #21433

mmarchini opened this issue Jun 21, 2018 · 22 comments
Labels
build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.

Comments

@mmarchini
Copy link
Contributor

  • Version: v8.x
  • Platform: Linux (x64)
  • Subsystem: v8

make test-v8 is not working on Linux (x64) for any v8.x version. clang thinks standard libraries are "missing". From our V8 CI:

15:03:01 In file included from ../src/setup-isolate-full.cc:7:
15:03:01 .././src/base/logging.h:8:10: fatal error: 'cstring' file not found
15:03:01 #include <cstring>
15:03:01          ^~~~~~~~~
15:03:01 In file included from ../src/base/bits.cc:5:
15:03:01 .././src/base/bits.h:9:10: fatal error: 'type_traits' file not found
15:03:01 #include <type_traits>
15:03:01          ^~~~~~~~~~~~~
15:03:01 In file included from In file included from ../src/base/cpu.cc:5:
15:03:01 In file included from .././src/base/cpu.h:16:
15:03:01 In file included from .././src/base/base-export.h:8:
15:03:01 .././include/v8config.h:16:11: fatal error: 'features.h' file not found
15:03:01 # include <features.h>
15:03:01           ^~~~~~~~~~~~
15:03:01 ../src/base/division-by-constant.cc:5:
15:03:01 In file included from .././src/base/division-by-constant.h:10:
15:03:01 In file included from .././src/base/base-export.h:8:
15:03:01 .././include/v8config.h:16:11: fatal error: 'features.h' file not found
15:03:01 # include <features.h>
15:03:01           ^~~~~~~~~~~~
15:03:01 In file included from ../src/base/debug/stack_trace.cc:5:
15:03:01 .././src/base/debug/stack_trace.h:13:10: fatal error: 'iosfwd' file not found
15:03:01 #include <iosfwd>
15:03:01          ^~~~~~~~
15:03:01 1 error generated.

What is actually happening: clang can't find any standard library because the flag --sysroot is set to /path/to/node/deps/v8/build/linux/debian_jessie_amd64-sysroot when it should be set either unset or set to /path/to/node/deps/v8/build/linux/debian_sid_amd64-sysroot. Since /path/to/node/deps/v8/build/linux/debian_jessie_amd64-sysroot don't exist, clang can't find any standard library.

I think the flag is being set on ./deps/v8/gypfiles/standalone.gypi:

  91 'conditions': [
  92   # The system root for linux builds.
  93   ['OS=="linux" and use_sysroot==1', {
  94     'conditions': [
  95       ['target_arch=="arm"', {
  96         'sysroot%': '<!(cd <(DEPTH) && pwd -P)/build/linux/debian_jessie_arm-sysroot',
  97       }],
  98       ['target_arch=="x64"', {
  99         'sysroot%': '<!(cd <(DEPTH) && pwd -P)/build/linux/debian_jessie_amd64-sysroot',
 100       }],
 101       ['target_arch=="ia32"', {
 102         'sysroot%': '<!(cd <(DEPTH) && pwd -P)/build/linux/debian_jessie_i386-sysroot',
 103       }],
 104       ['target_arch=="mipsel"', {
 105         'sysroot%': '<!(cd <(DEPTH) && pwd -P)/build/linux/debian_jessie_mips-sysroot',
 106       }],
 107     ],
 108   }], # OS=="linux" and use_sysroot==1

I'm not sure why this just started to happen now, but it is blocking all V8 PRs to v8.x-staging.

Any help to solve this would be really appreciated.

/cc @targos @nodejs/v8 @nodejs/build-files

Ref: #21334

@mmarchini mmarchini added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Jun 21, 2018
@mmarchini
Copy link
Contributor Author

This patch seems to fix it:

diff --git a/deps/v8/gypfiles/standalone.gypi b/deps/v8/gypfiles/standalone.gypi
index 63930d8..d47e8c3 100644
--- a/deps/v8/gypfiles/standalone.gypi
+++ b/deps/v8/gypfiles/standalone.gypi
@@ -93,16 +93,16 @@
           ['OS=="linux" and use_sysroot==1', {
             'conditions': [
               ['target_arch=="arm"', {
-                'sysroot%': '<!(cd <(DEPTH) && pwd -P)/build/linux/debian_jessie_arm-sysroot',
+                'sysroot%': '<!(cd <(DEPTH) && pwd -P)/build/linux/debian_sid_arm-sysroot',
               }],
               ['target_arch=="x64"', {
-                'sysroot%': '<!(cd <(DEPTH) && pwd -P)/build/linux/debian_jessie_amd64-sysroot',
+                'sysroot%': '<!(cd <(DEPTH) && pwd -P)/build/linux/debian_sid_amd64-sysroot',
               }],
               ['target_arch=="ia32"', {
-                'sysroot%': '<!(cd <(DEPTH) && pwd -P)/build/linux/debian_jessie_i386-sysroot',
+                'sysroot%': '<!(cd <(DEPTH) && pwd -P)/build/linux/debian_sid_i386-sysroot',
               }],
               ['target_arch=="mipsel"', {
-                'sysroot%': '<!(cd <(DEPTH) && pwd -P)/build/linux/debian_jessie_mips-sysroot',
+                'sysroot%': '<!(cd <(DEPTH) && pwd -P)/build/linux/debian_sid_mips-sysroot',
               }],
             ],
           }], # OS=="linux" and use_sysroot==1

But I'm not sure if this is the right approach.

@joyeecheung
Copy link
Member

joyeecheung commented Jun 21, 2018

This could have something to do with changes to chromium's build infrastructure, test-v8 uses gclient (seems to be cloned from master in node_common.py) to pull the git tree so that's subject to change in the upstream. https://bugs.chromium.org/p/chromium/issues/detail?id=800977 may be helpful.

@mmarchini
Copy link
Contributor Author

Yeah, that's what I thought. Although sid is being downloaded instead of jessie since February, so I don't know why this only started to happen now.

@mhdawson
Copy link
Member

mhdawson commented Jun 21, 2018

@hashseed can you comment or pull in somebody from google who can comment on what we should do?

@mhdawson
Copy link
Member

Or I guess @ofrobots maybe you can also find somebody at Google who can comment/help seems to be an issue with a change in the google setup/tooling

@mhdawson
Copy link
Member

@mmarchini as a short term fix we could update the job to apply your patch but I'd not want to to stay in place for too long.

@mmarchini
Copy link
Contributor Author

I'd rather wait for comments or submit a PR with the patch to nodejs/node, otherwise, this will be yet-another-patch hanging in the node-test-commit-v8-linux machines.

@ofrobots
Copy link
Contributor

@hashseed @natorion any thoughts on this question from @mhdawson: #21433 (comment)

@hashseed
Copy link
Member

V8 and Chrome uses its own checkout of clang from gclient to build. Something must have changed there.

A while ago I posted some instructions in the discussion starting here, and with some help was able to make it work reasonably well in the end. Maybe someone can use that to reimplement make test-v8?

@mhdawson
Copy link
Member

@hashseed, its still works for master and 10.x so going forward I think its ok, its just that what used to work for 8.x no longer does.

@ofrobots
Copy link
Contributor

@hashseed I copied the deps/v8/tools/node directory from the master branch to the v8.x branch, but it doesn't quite work:

Running hooks:  80% (16/20) instrumented_libraries
________ running '/usr/bin/python v8/third_party/instrumented_libraries/scripts/download_binaries.py' in '.'
/usr/bin/python: can't open file 'v8/third_party/instrumented_libraries/scripts/download_binaries.py': [Errno 2] No such file or directory
Error: Command '/usr/bin/python v8/third_party/instrumented_libraries/scripts/download_binaries.py' returned non-zero exit status 2 in .
Uninitializing temporary git repository
>> Cleaning up /usr/local/google/home/ofrobots/src/node/deps/node-8/deps/v8/.git
Traceback (most recent call last):
  File "tools/node/fetch_deps.py", line 94, in <module>
    FetchDeps(sys.argv[1])
  File "tools/node/fetch_deps.py", line 78, in FetchDeps
    env=env)
  File "/usr/lib/python2.7/subprocess.py", line 186, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['/usr/bin/python', '/usr/local/google/home/ofrobots/src/node/deps/node-8/deps/v8/_depot_tools/gclient.py', 'sync', '--spec', "solutions = [{'url': 'https://chromium.googlesource.com/v8/v8.git', 'managed': False, 'name': 'v8', 'deps_file': 'DEPS', 'custom_deps': {'v8/third_party/catapult': None, 'v8/third_party/colorama/src': None, 'v8/testing/gmock': None, 'v8/third_party/markupsafe': None, 'v8/third_party/jinja2': None, 'v8/tools/swarming_client': None, 'v8/third_party/googletest/src': None, 'v8/base/trace_event/common': None, 'v8/third_party/instrumented_libraries': None, 'v8/third_party/android_tools': None, 'v8/test/benchmarks/data': None, 'v8/test/mozilla/data': None, 'v8/tools/luci-go': None, 'v8/test/test262/data': None, 'v8/test/test262/harness': None}}]"]' returned non-zero exit status 2

However, as @mhdawson said, why did the old mechanism stop working for v8.x?

@mmarchini
Copy link
Contributor Author

@hashseed is #21494 acceptable to fix the issue?

@hashseed
Copy link
Member

I honestly don't know. @mmarchini's fix may be correct.

@mhdawson
Copy link
Member

@hashseed do you know somebody in the google/chrome team that might be able to give us some confirmation that it's the right way to go?

@ofrobots
Copy link
Contributor

@mmarchini #21494 looks reasonable to me.

mmarchini pushed a commit to mmarchini/node that referenced this issue Jun 25, 2018
On Node.js v8.x, gn will pass a sysroot parameter to clang to use a
downloaded sysroot files while running `make test-v8`. Recently,
chromium build tools switched to use Debian sid sysroot files instead of
Debian jessie. This patch updates our V8 GYP files to conform with those
changes.

Ref: nodejs#21433

PR-URL: nodejs#21494
Refs: nodejs#21433
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
mmarchini pushed a commit to mmarchini/node that referenced this issue Jun 25, 2018
On Node.js v8.x, gn will pass a sysroot parameter to clang to use a
downloaded sysroot files while running `make test-v8`. Recently,
chromium build tools switched to use Debian sid sysroot files instead of
Debian jessie. This patch updates our V8 GYP files to conform with those
changes.

Ref: nodejs#21433

PR-URL: nodejs#21494
Refs: nodejs#21433
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this issue Jun 26, 2018
On Node.js v8.x, gn will pass a sysroot parameter to clang to use a
downloaded sysroot files while running `make test-v8`. Recently,
chromium build tools switched to use Debian sid sysroot files instead of
Debian jessie. This patch updates our V8 GYP files to conform with those
changes.

Ref: #21433

PR-URL: #21494
Refs: #21433
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@Trott
Copy link
Member

Trott commented Nov 21, 2018

#21494 has been merged. Should this be closed? @mmarchini @hashseed @ofrobots

@hashseed
Copy link
Member

Yes.

@hashseed
Copy link
Member

Ah wait. I mistook this as make test-v8 on master. I don't actually know about the status of this for the Node 8 branch.

@hashseed hashseed reopened this Nov 21, 2018
@refack
Copy link
Contributor

refack commented Nov 21, 2018

But AFAIK it works on our CI e.g. https://ci.nodejs.org/job/node-test-commit-v8-linux/1887/
with the following incantation (assuming depot_tools is in the PATH):

  rm deps/v8/test/mjsunit/d8-os.js || true
  ./configure
  make -j $(getconf _NPROCESSORS_ONLN) test-v8 V=1 $DISABLE_V8_I18N_OPTION DESTCPU=$DESTCPU ARCH=$DESTCPU.release $ADDITIONAL_CLANG_OPTIONS ENABLE_V8_TAP=True V8_EXTRA_TEST_OPTIONS="--progress=dots --timeout=120"

@refack
Copy link
Contributor

refack commented Nov 21, 2018

So I'm going to close this, @mmarchini feel free to reopen if I'm wrong.

@refack refack closed this as completed Nov 21, 2018
@jescen

This comment was marked as off-topic.

@bnoordhuis

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

9 participants