build,test: run v8 tests on windows by kunalspathak · Pull Request #13992 · nodejs/node · GitHub

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

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

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...

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.

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).

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.

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)

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?

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).

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?

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> .

@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.

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.

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.

@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.

Yep, i understand that 😄

ping. any other comments?

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

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

sure.

Is there a reason this is defined twice?

One of them need to go

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

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.

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 )?

popd

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.

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.

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.

One of them need to go

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

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

yep.

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.

let the output flow

sure.

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.

pushd

popd

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.

Is this still needed?

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

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

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

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

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 <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

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.

ping @nodejs/platform-windows

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.

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

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 by kunalspathak · Pull Request #13992 · nodejs/node · GitHub

Uh oh!

Uh oh!

build,test: run v8 tests on windows - #13992

build,test: run v8 tests on windows#13992kunalspathak merged 1 commit intonodejs:masternodejs/node:masterfrom kunalspathak:testv8kunalspathak/node:testv8Copy head branch name to clipboard

Conversation

kunalspathak commented Jun 29, 2017

Checklist

Affected core subsystem(s)

Uh oh!

kunalspathak commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Jun 30, 2017

Uh oh!

bnoordhuis commented Jul 2, 2017

Uh oh!

refack commented Jul 2, 2017

Uh oh!

kunalspathak commented Jul 3, 2017

Uh oh!

joaocgreis commented Jul 9, 2017

Uh oh!

kunalspathak commented Jul 9, 2017

Uh oh!

jbajwa commented Jul 9, 2017

Uh oh!

refack commented Jul 9, 2017

Uh oh!

jbajwa commented Jul 9, 2017 via email

Uh oh!

joaocgreis commented Jul 10, 2017

Uh oh!

refack commented Jul 10, 2017

Uh oh!

kunalspathak commented Jul 10, 2017

Uh oh!

joaocgreis commented Jul 10, 2017

Uh oh!

kunalspathak commented Jul 10, 2017

Uh oh!

kunalspathak commented Jul 14, 2017

Uh oh!

gibfahn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

build,test: run v8 tests on windows#13992
kunalspathak merged 1 commit into
nodejs:masterfrom
kunalspathak:testv8

kunalspathak commented Jun 29, 2017 •
edited
Loading

refack Jul 18, 2017 •
edited
Loading