build,test: run v8 tests on windows #13992

kunalspathak · 2017-06-29T21:59:14Z

This is a port of #4704 for windows.
Related discussion: nodejs/build#577 (comment)

vcbuild.bat test-v8 : Runs unit test from v8 repo
vcbuild.bat test-v8-intl : Runs intl test from v8 repo
vcbuild.bat test-v8 : Runs benchmarks from v8 repo

The runs needs depot_tools
installed on the machine expects environment variable DEPOT_TOOLS_PATH to be set to the path.

Set environment variable DISABLE_V8_I18N to disable i18n.

I verified this on my machine and it works as expected. But would love to setup a CI machine that contains depot_tools installation and try it out.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

build

kunalspathak · 2017-06-29T21:59:50Z

cc: @nodejs/build , @joaocgreis , @refack

refack · 2017-06-30T22:55:19Z

Could it be moved to in a separate .bat file (in tools/ or tools/msvs/)?

more tests yeah !!! 👍 🎉 💃 but:
vcbuild.bat is already a hot mess
depot_tools is a new dependency that only a subset of people want/have/not-discussed-by, so it makes sense to me to partition this to a new file. Maybe with an if not defined DEPOT_TOOLS_PATH exit /b 0 at the top...

bnoordhuis · 2017-07-02T10:41:06Z

depot_tools is a new dependency that only a subset of people want/have/not-discussed-by

@refack That's no different from the existing tools/make-v8.sh script though: it checks out depot_tools when you run make test-v8.

Tangential: I don't think depot_tools is needed to build d8 (could use ./configure --enable-d8 for that) but it also downloads things like test262 that are used in V8's test suite.

@kunalspathak This needs a rebase.

refack · 2017-07-02T22:45:39Z

Could it be moved to in a separate .bat file (in tools/ or tools/msvs/)?

@refack That's no different from the existing tools/make-v8.sh script though: it checks out depot_tools when you run make test-v8.

Just to explain myself: I ment a new batch file that could be called from vcbuild.bat (or on it's own if that's effortless).

kunalspathak · 2017-07-03T17:57:51Z

Could it be moved to in a separate .bat file (in tools/ or tools/msvs/)?

@refack - Yes, makes sense. I will rebase and update the PR with review comments.

joaocgreis · 2017-07-09T03:06:58Z

I believed the objective of running the V8 tests from our tree was to test our floating patches using the V8 test suite (as mentioned in nodejs/build#199, this would be used to test a PR that adds a floating patch to V8). So, I didn't understand the rd /S /Q v8 in this PR, but then I noticed a similar rm -rf v8 was added to make-v8.sh in #9393, which is currently in master.

Doesn't this completely defeat the purpose of testing V8 from our tree? Can't we trust that the V8 branch this ends up testing has been fully tested upstream? Shouldn't this tool use the V8 test suite on the V8 tree without removing our floating patches?

(also cc @jbajwa @mhdawson)

kunalspathak · 2017-07-09T08:16:00Z

Thanks @joaocgreis for bring this up. While I was working on refactoring to move make v8 part in separate script, i realized that in unix version, cleanup happens inside make-v8.sh before building and running v8 test. I am not sure if I follow this completely. Isn't it undoing the fetch/sync part or am i missing something?

jbajwa · 2017-07-09T23:21:30Z

Hi,
The idea of the script is to get the DEPS needed by V8 make and the run-tests.py script. I'll just briefly explain the script below:

git stash // preserve any local uncommitted changes
rm -rf v8 // need to remove v8 copy to fetch the latest v8 copy, with all DEPS
git checkout BRANCH // branch being the v8 version used in node
gclient sync // update the deps to that specific V8 version
git reset --hard HEAD // resets to current git HEAD, which will include any local committed changes
git clean -fdq // will clean any untracked src files, but deps will be ignored as its part of .gitignore
stash pop // will restore any uncommitted changes

So, it should test the V8 which is part of Node (including any floating patches).

refack · 2017-07-09T23:24:27Z

So, it should test the V8 which is part of Node (including any floating patches).

@jbajwa If I understand you correctly, here we do not need all the depot_tools actions, as the code in the node tree should be tested as is, right?

jbajwa · 2017-07-09T23:55:55Z

fetch v8 and gclient sync would still be needed to get the correct deps for v8 makefile to work. The v8 source is tested as is from the node tree.

…

On Sun, Jul 9, 2017 at 7:25 PM Refael Ackermann ***@***.***> wrote: So, it should test the V8 which is part of Node (including any floating patches). @jbajwa <https://github.com/jbajwa> If I understand you correctly, here we do not need all the depot_tools actions, as the code in the node tree should be tested as is, right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13992 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjQGojz8uqxXi_GQLtn6J_MYjLahPoqks5sMWFNgaJpZM4OJ61B> .

joaocgreis · 2017-07-10T01:22:10Z

@jbajwa I see now, thanks for the explanation! That's cleverly done.

@kunalspathak cleanup returns to the version of V8 in node with the floating patches, but keeping all the necessary files for building and testing V8 (because they are conveniently gitignored). So, this PR is missing that step before building. In the Unix version, cleanup is registered with trap to run if the script is cancelled. I don't think this is straightforward to do in cmd, but is not essential. You might also consider running make-v8.sh with Git Bash - we already require the git tools for testing and what this is doing actually increases the dependency on git. While for actually running tests it is always better to use cmd (to avoid tty being a pipe), for a setup stage I believe reusing the script would be better.

refack · 2017-07-10T01:32:33Z

running make-v8.sh with Git Bash - we already require the git tools for testing

Just backing up @joaocgreis - I'm using the Git-for-Windows make to run make lint and some git hooks, works very well.
Also AFAICT this is a opt-in operation so I would have considered adding the GfW toolset even as new requirement just like depot_tools.

kunalspathak · 2017-07-10T15:55:55Z

You might also consider running make-v8.sh with Git Bash

@joaocgreis - Thanks for the suggestion, i didn't realize that we could do this. I will test it out and if it works then I will re-use make-v8.sh.

So, this PR is missing that step before building

Yes, I realized it when i was refactoring. I mis-read cleanup function and was calling at the end of test. I will fix it.

joaocgreis · 2017-07-10T21:19:41Z

@kunalspathak just to be sure we're on the same page: those are mutually exclusive steps, if you re-use make-v8.sh then cleanup is not needed in vcbuild.bat.

kunalspathak · 2017-07-10T21:55:32Z

Yep, i understand that 😄

kunalspathak · 2017-07-14T17:43:05Z

ping. any other comments?

gibfahn

Some questions (not really sure about any of them, I don't know enough about batch).

gibfahn · 2017-07-15T23:13:05Z

vcbuild.bat

+echo Failed to find a suitable depot tools to test v8
+goto exit
+
+:restorepath


Maybe restorecwd or similar? I thought this was changing the %PATH%.

gibfahn · 2017-07-15T23:13:56Z

vcbuild.bat

@@ -216,6 +222,10 @@ echo Try to run in a "Developer Command Prompt" or consult
 echo https://github.com/nodejs/node/blob/master/BUILDING.md#windows-1
 goto exit

+:depot-tools-not-found


Is there a reason this is defined twice?

One of them need to go

This should not be defined twice. I will remove one.

gibfahn · 2017-07-15T23:15:40Z

vcbuild.bat

+call python tools\run-tests.py %common_v8_test_options% benchmarks --junitout ./v8-benchmarks-tap.xml
+goto restorepath
+
+:depot-tools-not-found


Second definition here.

Follow on question, as it's only called once, is there a need for a separate goto for it at all? IDK what others think, but for me every goto adds significant mental complexity, so if it's avoidable I'd like to avoid it.

gibfahn · 2017-07-15T23:17:38Z

vcbuild.bat

+goto exit
+
+:restorepath
+cd %~dp0


Maybe this should be part of the exit process for vcbuild.bat. I see we cd %~dp0 at the beginning, is there a reason not do do it at the end as well (cc/ @joaocgreis )?

gibfahn · 2017-07-15T23:22:26Z

vcbuild.bat

+echo running 'ninja -C out.gn/%test_args% %v8_build_options%'
+call ninja -C out.gn/%test_args% %v8_build_options% > nul 2> nul
+if errorlevel 1 goto exit
+set path=%savedpath%


AIUI if any of the above commands fail then the user's PATH is not restored. Same question as for %dp0, should we be doing this as part of exit?

I guess you could do:

if defined savedpath set path=%savedpath%

and then any other part of the script that modifies the path wouldn't have to worry about resetting it.

gibfahn · 2017-07-15T23:24:14Z

vcbuild.bat

+if "%config%"=="Debug" set test_args=%target_arch%.debug
+if "%config%"=="Release" set test_args=%target_arch%.release
+set savedpath=%path%
+set path=%DEPOT_TOOLS_PATH%;%path%


Is there a reason you went with CAPS for %DEPOT_TOOLS_PATH% but not elsewhere? I know Windows env vars are case-agnostic, but I'm not sure if there are any common defaults people use (e.g. lowercase for local, uppercase for global).

%DEPOT_TOOLS_PATH% if defined by depot_tools so it's not our call.

But if %DEPOT_TOOLS_PATH% is defined, doesn't that mean that %depot_tools_path% is also defined?

Yeah but that's not cricket.

@gibfahn - I followed the syntax of environment variables defined outside the script to be all caps. e.g. VS140COMNTOOLS, PROCESSOR_ARCHITEW6432, etc. I see that some tooling related environment variables are also all caps, e.g. GYP_MSVS_VERSION. But again i am open to change if you strongly feel like that.

gibfahn · 2017-07-15T23:28:14Z

vcbuild.bat

+call python tools\dev\v8gen.py %test_args% > nul 2> nul
+if errorlevel 1 goto exit
+echo running 'ninja -C out.gn/%test_args% %v8_build_options%'
+call ninja -C out.gn/%test_args% %v8_build_options% > nul 2> nul


So for these two commands if something fails then will there be any output? I assume you're &> nul because otherwise it spews too much out to the screen, but (e.g. if we start running this in CI) it might be useful to know what went wrong. Or maybe it just doesn't really fail?

I guess you could output to a logfile, and then echo something at the beginning saying errors logged to blah.log (or whatever it should be).

I see no reason to swallow or log to file. Let it flow just like in the call to MSbuild

Yeah, primary reason was it was outputing a lot. I like @refack suggestion and will let it flow like msbuild.

refack · 2017-07-15T23:51:51Z

vcbuild.bat

@@ -216,6 +222,10 @@ echo Try to run in a "Developer Command Prompt" or consult
 echo https://github.com/nodejs/node/blob/master/BUILDING.md#windows-1
 goto exit

+:depot-tools-not-found


One of them need to go

refack · 2017-07-15T23:54:47Z

vcbuild.bat

+call python tools\dev\v8gen.py %test_args% > nul 2> nul
+if errorlevel 1 goto exit
+echo running 'ninja -C out.gn/%test_args% %v8_build_options%'
+call ninja -C out.gn/%test_args% %v8_build_options% > nul 2> nul


I see no reason to swallow or log to file. Let it flow just like in the call to MSbuild

refack · 2017-07-15T23:58:03Z

vcbuild.bat

+echo calling: tools\make-v8.sh
+sh tools\make-v8.sh
+if errorlevel 1 goto exit
+cd %~dp0


not needed as it's a precondition for this whole script and done in L3

refack · 2017-07-16T00:02:36Z

vcbuild.bat

+if "%config%"=="Debug" set test_args=%target_arch%.debug
+if "%config%"=="Release" set test_args=%target_arch%.release
+set savedpath=%path%
+set path=%DEPOT_TOOLS_PATH%;%path%


I'd rather require this as a precondition for running.
Test, and if it's not set, err, like with if not defined DEPOT_TOOLS_PATH

If all you need is ninja you could

where ninja > nul 2>nul if errorlevel 1 echo depot_tools not set-up & goto exit

Didn't follow you. if not defined DEPOT_TOOLS_PATH would make sure depot_tools are in path. If ninja is not there, it will anyway fail when invoked. I know it is better to check upfront, but sync script calls other tools/scripts that also have to be in path and if not, things will fail when invoked.

AFAIK when depot_tools are set-up Path already includes DEPOT_TOOLS_PATH, so if you could test that and fail, it would be nicer than manipulation the Path.
If it's not simple, then current solution is Ok.

refack · 2017-07-16T00:02:59Z

vcbuild.bat

+cd %~dp0
+cd deps\v8
+echo running 'python tools\dev\v8gen.py %test_args%'
+call python tools\dev\v8gen.py %test_args% > nul 2> nul


let the output flow

refack · 2017-07-16T00:04:53Z

vcbuild.bat

@@ -83,6 +85,10 @@ if /i "%1"=="test-async-hooks"  set test_args=%test_args% async-hooks&goto arg-o
 if /i "%1"=="test-all"      set test_args=%test_args% gc internet pummel %common_test_suites%&set build_testgc_addon=1&set cpplint=1&set jslint=1&goto arg-ok
 if /i "%1"=="test-node-inspect" set test_node_inspect=1&goto arg-ok
 if /i "%1"=="test-check-deopts" set test_check_deopts=1&goto arg-ok
+if /i "%1"=="test-v8"       set test_v8=1&set custom_v8_test=1&goto arg-ok


not: I'd prefer you align to if /i "%1"=="test-check-deopts"

There are other checks that are not aligned. e.g. test-all above. test-v8 aligns to test-all and other shorter if checks.

refack · 2017-07-16T00:07:39Z

vcbuild.bat

+sh tools\make-v8.sh
+if errorlevel 1 goto exit
+cd %~dp0
+cd deps\v8


refack · 2017-07-16T00:08:21Z

vcbuild.bat

+goto exit
+
+:restorepath
+cd %~dp0


refack · 2017-07-16T00:10:11Z

vcbuild.bat

+goto test-v8
+
+:test-v8
+if not defined custom_v8_test goto cpplint


I'd still would like this block to move to tools\test-v8.cmd
and here leave

call tools\test-v8.cmd goto cpplint

AFAICT everything else will fall into place.

gibfahn · 2017-07-18T01:16:08Z

vcbuild.bat

@@ -513,9 +525,15 @@ echo %cmd1%
 %cmd1%
 exit /b %ERRORLEVEL%

+:test-v8-exit


Is this still needed?

weird, I thought I removed this and moved it to test-v8.bat. I will remove it. Sorry about that.

refack · 2017-07-18T23:42:38Z

tools/test-v8.bat

+if "%config%"=="Debug" set test_args=%target_arch%.debug
+if "%config%"=="Release" set test_args=%target_arch%.release
+set savedpath=%path%
+set path=%DEPOT_TOOLS_PATH%;%path%


Now you can SETLOCAL and ENDLOCAL in :test-v8-exit and then I don't care what monstrosities you do to poor old Path

refack · 2017-07-18T23:45:49Z

Ohhhh, so pretty. If only we used the sub-script approach earlier....

kunalspathak · 2017-07-20T18:58:29Z

@gibfahn - Do you have any other comments or should we land this?

kunalspathak · 2017-07-26T01:05:09Z

I will land this PR tomorrow if no one objects.

`vcbuild.bat test-v8` : Runs unit test from v8 repo `vcbuild.bat test-v8-intl` : Runs intl test from v8 repo `vcbuild.bat test-v8` : Runs benchmarks from v8 repo The runs needs https://www.chromium.org/developers/how-tos/install-depot-tools installed on the machine expects environment variable `DEPOT_TOOLS_PATH` to be set to the path. Set environment variable `DISABLE_V8_I18N` to disable i18n. PR-URL: nodejs#13992 Refs: nodejs#4704 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

kunalspathak · 2017-07-27T01:11:44Z

https://ci.nodejs.org/job/node-test-pull-request/9361/ looks green

`vcbuild.bat test-v8` : Runs unit test from v8 repo `vcbuild.bat test-v8-intl` : Runs intl test from v8 repo `vcbuild.bat test-v8` : Runs benchmarks from v8 repo The runs needs https://www.chromium.org/developers/how-tos/install-depot-tools installed on the machine expects environment variable `DEPOT_TOOLS_PATH` to be set to the path. Set environment variable `DISABLE_V8_I18N` to disable i18n. PR-URL: #13992 Refs: #4704 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

MylesBorins · 2017-08-16T08:10:03Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

MylesBorins · 2017-10-16T21:24:15Z

ping @nodejs/platform-windows

joaocgreis · 2017-10-19T15:19:35Z

I don't think this needs to be backported to v6. This is mainly to test floating patches on V8, I believe those will not be common in v6 in the future.

MylesBorins · 2017-10-24T00:05:34Z

Every time we need to float a new V8 patch we should run those tests. It isn’t 100% necessary but nice to have

refack · 2017-10-24T00:07:19Z

I'll backport.

nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Jun 29, 2017

jasnell approved these changes Jun 30, 2017

View reviewed changes

kunalspathak force-pushed the testv8 branch from 38d00bd to d391293 Compare July 11, 2017 01:10

gibfahn suggested changes Jul 15, 2017

View reviewed changes

refack suggested changes Jul 16, 2017

View reviewed changes

gibfahn reviewed Jul 18, 2017

View reviewed changes

refack approved these changes Jul 18, 2017

View reviewed changes

refack reviewed Jul 18, 2017

View reviewed changes

kunalspathak force-pushed the testv8 branch from 1e44d82 to 09dd6bb Compare July 27, 2017 01:15

kunalspathak merged commit 09dd6bb into nodejs:master Jul 27, 2017

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

MylesBorins added the lts-watch-v6.x label Aug 16, 2017

joaocgreis added dont-land-on-v6.x and removed lts-watch-v6.x labels Oct 19, 2017

refack self-assigned this Oct 24, 2017

refack mentioned this pull request Apr 7, 2018

win, build: add documentation support to vcbuild #19663

Closed

2 tasks

refack removed their assignment Oct 12, 2018

build,test: run v8 tests on windows #13992

build,test: run v8 tests on windows #13992

Conversation

kunalspathak commented Jun 29, 2017

Checklist

Affected core subsystem(s)

kunalspathak commented Jun 29, 2017 • edited Loading

refack commented Jun 30, 2017

bnoordhuis commented Jul 2, 2017

refack commented Jul 2, 2017

kunalspathak commented Jul 3, 2017

joaocgreis commented Jul 9, 2017

kunalspathak commented Jul 9, 2017

jbajwa commented Jul 9, 2017

refack commented Jul 9, 2017

jbajwa commented Jul 9, 2017 via email

joaocgreis commented Jul 10, 2017

refack commented Jul 10, 2017

kunalspathak commented Jul 10, 2017

joaocgreis commented Jul 10, 2017

kunalspathak commented Jul 10, 2017

kunalspathak commented Jul 14, 2017

gibfahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

refack commented Jul 18, 2017

kunalspathak commented Jul 20, 2017

kunalspathak commented Jul 26, 2017

kunalspathak commented Jul 27, 2017

MylesBorins commented Aug 16, 2017

MylesBorins commented Oct 16, 2017

joaocgreis commented Oct 19, 2017

MylesBorins commented Oct 24, 2017 via email

refack commented Oct 24, 2017

kunalspathak commented Jun 29, 2017 •

edited

Loading

refack Jul 18, 2017 •

edited

Loading