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

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

Closed
wants to merge 9 commits into from
Closed

Conversation

jbajwa
Copy link
Contributor

@jbajwa jbajwa commented Nov 1, 2016

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.
@nodejs-github-bot 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
@mscdex 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
rm -rf .v8old
if [ "$BRANCH" == "master" ]; then
echo "git cleanup if branch is master"
git ls-files -m | xargs git checkout --
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git reset --hard HEAD?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jbajwa
Copy link
Contributor Author

jbajwa commented Nov 3, 2016

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why cleaning up only for master?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we only need the cleanup for the v8 bundled with node (here master would be v8 master). If you want to test eg [email protected], 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 [email protected] does not really make sense to me. If you said test using the tools from [email protected] then that might make more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: git clean has a quiet option as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I've updated accordingly.

mv $v8ver v8
mv v8 .v8old

echo "Fetching v8 from chromium.googlesource.com"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We may not have to mention the location.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

else
# eg: 5.4-lkgr
BRANCH="$1"
fi
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mhdawson
Copy link
Member

mhdawson commented Nov 4, 2016

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 ?

@jbajwa
Copy link
Contributor Author

jbajwa commented Nov 4, 2016

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

@mhdawson
Copy link
Member

mhdawson commented Nov 4, 2016

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.

@jbajwa
Copy link
Contributor Author

jbajwa commented Nov 4, 2016

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

@richardlau
Copy link
Member

richardlau commented Nov 4, 2016

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

@mhdawson
Copy link
Member

mhdawson commented Nov 4, 2016

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

@mhdawson
Copy link
Member

mhdawson commented Nov 4, 2016

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.

@jbajwa
Copy link
Contributor Author

jbajwa commented Nov 4, 2016

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

@mhdawson
Copy link
Member

mhdawson commented Nov 4, 2016

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

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - CI run was good.

@mhdawson mhdawson self-assigned this Nov 4, 2016
cd v8

echo "Checking out branch:$BRANCH"
if [ "$BRANCH" != "master" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, I've updated the script.

@thefourtheye
Copy link
Contributor

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

  1. Determine branch from v8-version.h file
  2. Rename the current v8 directory to .v8old
  3. Fetch v8 code and store it in v8 directory
  4. Enter v8 directory; checkout branch; sync dependencies and come back to previous directory
  5. Hard reset node repo
  6. git clean the .v8old
  7. Copy files from .v8old to v8
  8. 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?

@jbajwa
Copy link
Contributor Author

jbajwa commented Nov 8, 2016

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.

@thefourtheye
Copy link
Contributor

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

@jbajwa
Copy link
Contributor Author

jbajwa commented Nov 8, 2016

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)

@mhdawson
Copy link
Member

mhdawson commented Nov 8, 2016

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

@thefourtheye
Copy link
Contributor

thefourtheye commented Nov 9, 2016

Thanks @mhdawson :)

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

@jbajwa
Copy link
Contributor Author

jbajwa commented Nov 9, 2016

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

@thefourtheye
Copy link
Contributor

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

@jbajwa
Copy link
Contributor Author

jbajwa commented Nov 9, 2016

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

@thefourtheye
Copy link
Contributor

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?

@jbajwa
Copy link
Contributor Author

jbajwa commented Nov 9, 2016

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.

@thefourtheye
Copy link
Contributor

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

@jbajwa
Copy link
Contributor Author

jbajwa commented Nov 9, 2016

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

@mhdawson
Copy link
Member

mhdawson commented Nov 9, 2016

Landed as 4aca347

mhdawson pushed a commit that referenced this pull request Nov 9, 2016
google build tool gclient doesn't support
svn anymore. Updating v8 build script to use
git instead.

PR-URL: #9393
Reviewed By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

landed as 4aca347

@mhdawson mhdawson closed this Nov 10, 2016
@thefourtheye
Copy link
Contributor

Thanks @jbajwa for helping me understand :-)

MylesBorins pushed a commit that referenced this pull request Nov 10, 2016
google build tool gclient doesn't support
svn anymore. Updating v8 build script to use
git instead.

PR-URL: #9393
Reviewed By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 10, 2016
google build tool gclient doesn't support
svn anymore. Updating v8 build script to use
git instead.

PR-URL: #9393
Reviewed By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins
Copy link
Contributor

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

MylesBorins pushed a commit that referenced this pull request Nov 11, 2016
google build tool gclient doesn't support
svn anymore. Updating v8 build script to use
git instead.

PR-URL: #9393
Reviewed By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
This was referenced Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants