v8: update make-v8.sh to use git by jbajwa · Pull Request #9393 · nodejs/node · GitHub

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

v8

Description of change

google build tool gclient doesn't support svn anymore. Updating v8 build script
to use git instead. More info #9222

google build tool gclient doesn't support svn anymore. Updating v8 build script to use git instead.

git reset --hard HEAD?

Indeed this is a better way, I've updated accordingly. Thank you.

If the goal is to bring the tree back to a pristine state consider changing this to -dfx so .gitignored files are also deleted.

I want to keep the dependencies which are part of .gitignore to be able to build v8 standalone, eg v8/third_party, v8/buildtools etc. git clean -fd should remove the rest of the files that .gitignore doesn't ignore, eg newer src/test files. The goal is to have node/deps/v8 with all the third party dependencies in it.

files which are not present in the remote repo.

LGTM. Let's see how the V8 CI fares: https://ci.nodejs.org/job/node-test-commit-v8-linux/390/

The V8 CI passed successfully. Although I have one comment about the job itself, I noticed it does, "make v8" and then "make test-v8", but "make test-v8" itself does "make v8" first before running the suite, so there is a redundant "make v8". I think just "make test-v8" should be good. I was going to edit the job but don't have permissions to do so.

Why cleaning up only for master?

we only need the cleanup for the v8 bundled with node (here master would be v8 master). If you want to test eg v8@5.5, then you don't need the cleanup since thats not the v8 bundled with node and can be tested standalone. Use case for that would be, if you want to test a specific v8 version before merging into node.

What about the case were somebody has a pr and launches the job against their id/repo ?

More specifically as per my other comment I think we need more info on what BRANCH is. This job tests branches of node.js not v8 so saying you are going to test v8@5.5 does not really make sense to me. If you said test using the tools from v8@5.5 then that might make more sense.

Launching a job with somebody's pr should still work, since their git repo will be tracking the files they updated. Like the job Ben started with my changes https://ci.nodejs.org/job/node-test-commit-v8-linux/390/
Regarding BRANCH, yes it indeed meant to test specific v8 version if needed, otherwise it will get the tools from V8 master. I might have misunderstood the exact purpose of this job, as you mentioned in the other comment if the purpose is to get the tools of a specific v8 version then I will have to do the cleanup for any branch. Will make the necessary changes.

Nit: git clean has a quiet option as well

Thanks, I've updated accordingly.

Nit: We may not have to mention the location.

for someone looking at the job's console log, this would be useful information?

We may need a bit more doc with respect to this. I interpret this to be the v8 branch from which we are going to get the additional tools required to build. So while we may be testing Node.js in branch X, in this case the branch refers to the v8 branch to get the tools from. If my interpretation is correct, adding some explanation of that would be useful.

I've updated the nightly job to temporarily apply this patch until it lands so that we get coverage and people can run for regression testing. I also updated to remove the redundant step of make v8.

Looking at the output though it looks like there is an issue with the rsync:

rsync: change_dir "/data/iojs/build/workspace/node-test-commit-v8-linux/nodes/rhel72-s390x/v8test/v8test/deps//.v8old" failed: No such file or directory (2)

Is it possible that because you have done the reset/clean that you end up removing the v8old directory ?

@mhdawson Yes, that might be the case, although I did test my changes before committing. I'll take a look at it now.

One more questions. In terms of validating that we end up testing the right thing.

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

The node version we check out was reported as :7537718460c7b964ffbbc0910b12eaff9cd8b7a8

but after the reset/clean I see this:

HEAD is now at 164d9f6 Using git clean quiet option instead of directing stdout to /dev/null

I'm trying to understand as my first through is that we should have seen the same hash.

@mhdawson not sure how the job is setup, shouldn't the initial checkout also point to 164d9f6? like the previous job .
I've fixed the rsync issue (I didn't catch earlier in my local testrun because my .v8old copy had the .git repo and git clean -fd ignores those directories with .git).
Also, I was thinking instead of passing the branch as argument, the script should be able to query the current v8 version being used and checkout that V8 branch accordingly. Do you know whats the optimal way to query current v8 version? The only place I see is doc/changelogs/CHANGELOG_V7.md but that might not work if I want to run this job with other node versions.

V8 version is in include/v8-version.h in the V8 source tree.

@jbajwa , this is what I see in the job you pointed to:

git checkout -f 7537718460c7b964ffbbc0910b12eaff9cd8b7a8

As for the v8 version checking include/v8-versions.h as suggested by @richardlau seems reasonable.

Ok talked to @jbajwa and figured out my mistake. It's that I'm applying his commit in the job as a temporary fix so of course its always the last commit and therefore what we reset to.

deps/v8/include/v8-version.h

I've updated the script to query v8 branch from include/v8-version.h.

Looks good, CI run to validate https://ci.nodejs.org/job/node-test-commit-v8-linux/397/

LGTM - CI run was good.

This check is not necessary anymore, as the branch is determined from the file.

agreed, I've updated the script.

I may be missing something. Let me list down what this change does

Determine branch from v8-version.h file
Rename the current v8 directory to .v8old
Fetch v8 code and store it in v8 directory
Enter v8 directory; checkout branch; sync dependencies and come back to previous directory
Hard reset node repo
git clean the .v8old
Copy files from .v8old to v8
delete .v8old

I am not able to understand the last four steps. Reset node, this will remove all the changes done. Cleanup .v8old, so that all the compilations related files will be cleaned up and copy it back to v8. Isn't this more like a no-op or am I missing something?

Hi @thefourtheye ,
git clean is cleaning all files that are untracked by node/.git, in v8 (not .v8old since I'm excluding it git clean -e .v8old
The purpose to keep .v8old is to preserve any local files that were modified/added by the current user. Once v8 files are brought back to node HEAD, rsync .v8old v8 will copy over local changes that are not present in node remote repo.

@jbajwa Ah, thanks for clarifying :-) I guess I needed some sleep. I totally misunderstood the exclusion part.

Now, if the user makes some changes, that is stored in .v8old, new changes are fetched and then the user made changes are restored, with rsync, what if there are conflicts?

There shouldn't be any conflicts, for eg:

original deps/v8 has state --> A
user adds/modifies some files (uncommitted) --> A+x , Now the user runs the script to build/test
v8 is renamed to .v8old, so .v8old --> A+x
v8 is fetched from google, v8 --> A'
after git reset hard and git clean, v8 --> A (with third party deps)
rsync will copy the delta from .v8old to v8 that is --> (A+x) - A = x
so the delta should cleanly apply and v8 --> A+x (with third party deps)

Just holding off until @thefourtheye confirms he's happy with all of the answers.

Thanks @mhdawson :)

@jbajwa doesn't this sound like a simple stashing procedure?

@thefourtheye , thanks for pointing it out :) , I've updated the script to use git stash.

Actually, I want to avoid doing fetch v8. It downloads a lot of files from the server. Will something like this work?

git stash
fetch v8
git checkout branch
gclient sync
git stash pop

@thefourtheye
hmm, the steps you mentioned is what I'm doing at the moment.
Don't think we can avoid fetch v8, that's the only way to get v8 code. The other alternate is to config the .gclient file and do a gclient sync but that is way slower for the initial checkout.

If we kept the v8 directory only the new changes will be fetched right? It doesn't have to download the entire repository with the history?

The fetch cmd only works for initial checkout, otherwise you get the following error:

>fetch v8
Running: gclient root
Your current directory appears to already contain, or be part of, 
a checkout. "fetch" is used only to get new checkouts. Use 
"gclient sync" to update existing checkouts.

Fetch also does not yet deal with partial checkouts, so if fetch
failed, delete the checkout and start over (crbug.com/230691).

Since there is no deps/v8/.git gclient sync would also perform a full initial checkout, which is significantly slower than fetch v8.

@jbajwa Ah, you are correct. One last thing, git stash will not work if the users have temporarily committed the changes

@thefourtheye if the user has committed the changes in their working tree then git reset should point to that.

Landed as 4aca347

google build tool gclient doesn't support svn anymore. Updating v8 build script to use git instead. PR-URL: #9393 Reviewed By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

landed as 4aca347

Thanks @jbajwa for helping me understand :-)

google build tool gclient doesn't support svn anymore. Updating v8 build script to use git instead. PR-URL: #9393 Reviewed By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

I've gone ahead and backported this change to both v4 and v6 to allow us to test changes to v8 in CI

google build tool gclient doesn't support svn anymore. Updating v8 build script to use git instead. PR-URL: #9393 Reviewed By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

v8: update make-v8.sh to use git

c3deb42

google build tool gclient doesn't support svn anymore. Updating v8 build script to use git instead.

nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Nov 1, 2016

jbajwa mentioned this pull request Nov 1, 2016

V8 nightly tests failing, svn no longer supported #9222

Closed

mscdex added v8 engine Issues and PRs related to the V8 dependency. and removed build Issues and PRs related to build files or the CI. labels Nov 1, 2016

bnoordhuis requested changes Nov 2, 2016

View reviewed changes

addressed comments and added a rsync command to copy local

3c54a7e

files which are not present in the remote repo.

bnoordhuis approved these changes Nov 3, 2016

View reviewed changes

thefourtheye reviewed Nov 3, 2016

View reviewed changes

Using git clean quiet option instead of directing stdout to /dev/null

73e7c69

mhdawson reviewed Nov 4, 2016

View reviewed changes

perform git cleanup for all branches

7d8effd

removed passing branch as an argument instead quering from

fd9fd66

deps/v8/include/v8-version.h

mhdawson approved these changes Nov 4, 2016

View reviewed changes

mhdawson self-assigned this Nov 4, 2016

thefourtheye reviewed Nov 5, 2016

View reviewed changes

remove unnecessary branch checking

0171280

Remove .git directory from fetched copy of v8

971ef91

Use git stash to preserve local changes

8c995f4

thefourtheye approved these changes Nov 9, 2016

View reviewed changes

mhdawson closed this Nov 10, 2016

MylesBorins added land-on-v4.x labels Nov 10, 2016

This was referenced Nov 22, 2016

v6.9.2 proposal #9735

Merged

v4.7.0 proposal #9736

Merged

joaocgreis mentioned this pull request Jul 9, 2017

build,test: run v8 tests on windows #13992

Merged

3 tasks

+                rm -rf .v8old
+                if [ "$BRANCH" == "master" ]; then
+                  echo "git cleanup if branch is master"
+                  git ls-files -m | xargs git checkout --

+                if [ "$BRANCH" == "master" ]; then
+                  echo "git cleanup if branch is master"
+                  git ls-files -m | xargs git checkout --
+                  git clean -fd >/dev/null

-                  mv v8 $v8ver
-                  mv .v8old v8
+                if [ "$BRANCH" == "master" ]; then
+                  echo "git cleanup if branch is master"

+                if [ "$BRANCH" == "master" ]; then
+                  echo "git cleanup if branch is master"
+                  git reset --hard HEAD
+                  git clean -fd >/dev/null

+              else
+                # eg: 5.4-lkgr
+                BRANCH="$1"
+              fi

-                mv $v8ver v8
+              mv v8 .v8old
+              echo "Fetching v8 from chromium.googlesource.com"

+              cd v8
+              echo "Checking out branch:$BRANCH"
+              if [ "$BRANCH" != "master" ]; then

v8: update make-v8.sh to use git by jbajwa · Pull Request #9393 · nodejs/node · GitHub

Uh oh!

Uh oh!

v8: update make-v8.sh to use git - #9393

v8: update make-v8.sh to use git#9393jbajwa wants to merge 9 commits intonodejs:masternodejs/node:masterfrom jbajwa:patch-make-v8jbajwa/node:patch-make-v8Copy head branch name to clipboard

Conversation

jbajwa commented Nov 1, 2016

Checklist

Affected core subsystem(s)

Description of change

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!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

jbajwa commented Nov 3, 2016

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!

mhdawson commented Nov 4, 2016

Uh oh!

jbajwa commented Nov 4, 2016

Uh oh!

mhdawson commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbajwa commented Nov 4, 2016

Uh oh!

richardlau commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Nov 4, 2016

Uh oh!

mhdawson commented Nov 4, 2016

Uh oh!

jbajwa commented Nov 4, 2016

Uh oh!

mhdawson commented Nov 4, 2016

Uh oh!

mhdawson 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!

thefourtheye commented Nov 8, 2016

Uh oh!

jbajwa commented Nov 8, 2016

Uh oh!

thefourtheye commented Nov 8, 2016

Uh oh!

jbajwa commented Nov 8, 2016

Uh oh!

mhdawson commented Nov 8, 2016

v8: update make-v8.sh to use git#9393
jbajwa wants to merge 9 commits into
nodejs:masterfrom
jbajwa:patch-make-v8

mhdawson commented Nov 4, 2016 •
edited
Loading

richardlau commented Nov 4, 2016 •
edited
Loading

thefourtheye commented Nov 9, 2016 •
edited
Loading