CI-Change: tweaks to `make-v8` test script

[refack]

https://ci.nodejs.org/job/node-test-commit-v8-linux/jobConfigHistory/showDiffFiles?timestamp1=2018-09-11_18-19-03&timestamp2=2018-10-16_17-24-50
I've replaced the call to patch tools/make-v8, to simply overwrite it with the one in $HOME/build-tools/make-v8.patch:

-git apply $HOME/build-tools/make-v8.patch
+cp -f $HOME/build-tools/make-v8.patch tools/make-v8.sh

make-v8.patch is now a full and valid bash script:

#!/bin/bash -xe

BUILD_ARCH_TYPE=$1
V8_BUILD_OPTIONS=$2

cd deps/v8
tools/node/fetch_deps.py .

# set paths manually for now to use locally installed gn
export BUILD_TOOLS=/home/iojs/build-tools
export LD_LIBRARY_PATH=$BUILD_TOOLS:$LD_LIBRARY_PATH
export PATH=$BUILD_TOOLS:$PATH
CXX_PATH=`which $CXX |grep g++`
rm -f "$BUILD_TOOLS/g++"
rm -f "$BUILD_TOOLS/gcc"
ln -s $CXX_PATH "$BUILD_TOOLS/g++"
ln -s $CXX_PATH "$BUILD_TOOLS/gcc"
g++ --version
export PKG_CONFIG_PATH=$BUILD_TOOLS/pkg-config
gn gen -v out.gn/$BUILD_ARCH_TYPE --args='is_component_build=false is_debug=false use_goma=false goma_dir="None" use_custom_libcxx=false v8_target_cpu="s390x" target_cpu="s390x"'
ninja -v -C out.gn/$BUILD_ARCH_TYPE d8 cctest inspector-test

(added -xe to the shebang, and -v to the calls to gn and ninja)

Test job: https://ci.nodejs.org/job/node-test-commit-v8-linux/1767/nodes=rhel72-s390x,v8test=v8test/console
/CC @nodejs/v8-update @nodejs/platform-s390

[mhdawson]

My first question is what is the motivation for this change? I made it a patch so that :

1. ideally we'd get any changes if there are changes to the underlying file and
2. if that was not possible we'd know because the application of the patch would fail?

[refack]

My first question is what is the motivation for this change?

The main motivation was to add verbosity (since the build was failing). I tried to add it to tools/make-v8.sh but like you said the patch failed. That led me to dig into that whole mechanism... I found out that tools/make-v8.sh is only used in node-test-commit-v8-linux, so I thought it is not optimal that 2 out of 3 of its uses patch it significantly (with patches that come from the CI worker's drive, which is fragile).

I meant to follow up with a PR to add ifs in tools/make-v8.sh, eliminating the need to manipulate that file at all in CI...

[mhdawson]

Is the patch failing now ?

It was intended to be a short-medium term fix while we got changes into the google repos so I agree finding something better makes sense. Just not sure the copy over is better.

Unfortunately, the person who was working on it with me has left the team so we'll have to ask if when/the change might be making it into the v8 repos. @gdams adding you into the thread as well so you are up to speed.

I think next step is to figure out if/when that change might show up in the google repos so that we could remove any special behaviour and then based on how far out it is decide what to do.

[mhdawson]

@refack I guess the patch was failing, do you have the output of what the patch failure told us?

[refack]

04:04:08 Pythoning //build/config/linux/pkg-config.py took 212ms
04:04:08 Defining config //build/config/linux:glib(//build/toolchain/linux:s390x)
04:04:08 Pythoning python -- /data/iojs/build/workspace/node-test-commit-v8-linux/nodes/rhel72-s390x/v8test/v8test/deps/v8/build/config/linux/pkg-config.py atspi-2
04:04:08 Pythoning //build/config/linux/pkg-config.py took 38ms
04:04:08 ERROR at //build/config/linux/pkg_config.gni:103:17: Script returned non-zero exit code.
04:04:08     pkgresult = exec_script(pkg_config_script, args, "value")
04:04:08                 ^----------
04:04:08 Current dir: /data/iojs/build/workspace/node-test-commit-v8-linux/nodes/rhel72-s390x/v8test/v8test/deps/v8/out.gn/s390x.release/
04:04:08 Command: python -- /data/iojs/build/workspace/node-test-commit-v8-linux/nodes/rhel72-s390x/v8test/v8test/deps/v8/build/config/linux/pkg-config.py atspi-2
04:04:08 Returned 1.
04:04:08 stderr:
04:04:08 
04:04:08 Package atspi-2 was not found in the pkg-config search path.
04:04:08 Perhaps you should add the directory containing `atspi-2.pc'
04:04:08 to the PKG_CONFIG_PATH environment variable
04:04:08 No package 'atspi-2' found
04:04:08 Could not run pkg-config.
04:04:08 
04:04:08 See //build/config/linux/BUILD.gn:104:3: whence it was called.
04:04:08   pkg_config("atspi2") {
04:04:08   ^---------------------
04:04:08 See //build/config/compiler/BUILD.gn:224:18: which caused the file to be included.
04:04:08     configs += [ "//build/config/linux:compiler" ]
04:04:08                  ^------------------------------
04:04:08 make: *** [v8] Error 1

https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel72-s390x,v8test=v8test/1779/console

[mhdawson]

That does not look like a failure to apply the patch to the original tools/make-v8.sh file?

[refack]

Ohh, that fail:

17:02:36 + git apply /home/iojs/build-tools/make-v8.patch
17:02:36 /home/iojs/build-tools/make-v8.patch:23: trailing whitespace.
17:02:36 gn gen out.gn/$BUILD_ARCH_TYPE --args='is_component_build=false is_debug=false use_goma=false goma_dir="None" use_custom_libcxx=false v8_target_cpu="s390x" target_cpu="s390x"' 
17:02:36 error: patch failed: tools/make-v8.sh:5
17:02:36 error: tools/make-v8.sh: patch does not apply
17:02:36 Build step 'Execute shell' marked build as failure

After I patched tools/make-v8.sh with nodejs/node@0e62432

[mhdawson]

Thanks, that info lets me better understand the original issue and our current state.

And if I understand correctly, you were then going to PR in tools/make-v8.sh in the node.js repo to conditionally add the extra stuff for PPC and S390.

If that is what you were thinking, it probably makes sense. The patch has been in place a lot longer then I'd hoped and if you are going to build v8 using make-v8, even outside the CI you'd need those extra changes so maybe just putting them in the master version of make-v8 makes sense.

[refack]

@mhdawson submitted nodejs/node#23839