Add script to update relevant changes to Node.js.

Add script to update relevant changes to Node.js.

NOTRY=true
BUG=v8:6091
R=franzih@chromium.org, machenbach@chromium.org, ofrobots@google.com

Review-Url: https://codereview.chromium.org/2744663005
Cr-Commit-Position: refs/heads/master@{#43764}
Committed: https://chromium.googlesource.com/v8/v8/+/f52a483305b2c524b6f2fa9066c8fb097fc11a47

[Michael Achenbach, 2017-03-10 13:35:06]

Could you maybe add some simple system tests? Or I could also give it a try
after it lands.

I thought about a test that calls the script "as is" on a fake v8 directory
structure and a fake node directory structure in a temp workdir, and then checks
if the target files look right.

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.py
File tools/release/update_node.py (right):

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:21: cwd=path, shell=True, stdout=None)
nit: stdout=None is the default, can be dropped.

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:26: print ">> Updating target directory %s" %
source
nit - readability: it's a bit confusing to read "target directory" and then
print source var, though there's a target var as well...

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:28: git_commands = [
Is there any assumption about the directory state, e.g. being a git repo already
or not? What if a previous run left this in a bad state?

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:30: "git remote add origin %s" % source,  # point
to the source repo
Does this work if the git repro in source folder uses git cache?

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:37: subprocess.call(command, cwd=target,
shell=True, stdout=None)
nit: drop stdout, like above

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:39: print ">> Failed!"
Do we need this?

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:51: with open(file_name, "a") as gitignore:
Won't this append over and over again? What if the entries are already there?

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:76: [UpdateTarget(repo, options) for repo in
REPOSITORIES]
I'm having a one-line fetish too, but maybe here it'd be more clear that this is
a procedure and not a pure function by writing it imperative:
for repo in REPOSITORIES:
  UpdateTarget(repo, options)

[Yang, 2017-03-13 08:25:49]

I'm not sure how to create a system test like the one you have in mind. I was
thinking that we could use this script in our Node.js bot, thereby testing it.

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.py
File tools/release/update_node.py (right):

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:21: cwd=path, shell=True, stdout=None)
On 2017/03/10 13:35:06, Michael Achenbach wrote:
> nit: stdout=None is the default, can be dropped.

Done.

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:26: print ">> Updating target directory %s" %
source
On 2017/03/10 13:35:05, Michael Achenbach wrote:
> nit - readability: it's a bit confusing to read "target directory" and then
> print source var, though there's a target var as well...

You are right. It should print target first and source in the second line :)

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:28: git_commands = [
On 2017/03/10 13:35:05, Michael Achenbach wrote:
> Is there any assumption about the directory state, e.g. being a git repo
already
> or not? What if a previous run left this in a bad state?

Done.

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:30: "git remote add origin %s" % source,  # point
to the source repo
On 2017/03/10 13:35:06, Michael Achenbach wrote:
> Does this work if the git repro in source folder uses git cache?

I'm not familiar with git cache. Isn't it provided by depot_tools? How would it
interfere with this?

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:37: subprocess.call(command, cwd=target,
shell=True, stdout=None)
On 2017/03/10 13:35:06, Michael Achenbach wrote:
> nit: drop stdout, like above

Done.

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:39: print ">> Failed!"
On 2017/03/10 13:35:06, Michael Achenbach wrote:
> Do we need this?

Removed

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:51: with open(file_name, "a") as gitignore:
On 2017/03/10 13:35:05, Michael Achenbach wrote:
> Won't this append over and over again? What if the entries are already there?

Done.

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:76: [UpdateTarget(repo, options) for repo in
REPOSITORIES]
On 2017/03/10 13:35:05, Michael Achenbach wrote:
> I'm having a one-line fetish too, but maybe here it'd be more clear that this
is
> a procedure and not a pure function by writing it imperative:
> for repo in REPOSITORIES:
>   UpdateTarget(repo, options)

Done.

[Michael Achenbach, 2017-03-13 10:33:23]

Working on a test... will send you the patch.

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.py
File tools/release/update_node.py (right):

https://codereview.chromium.org/2744663005/diff/1/tools/release/update_node.p...
tools/release/update_node.py:30: "git remote add origin %s" % source,  # point
to the source repo
On 2017/03/13 08:25:49, Yang wrote:
> On 2017/03/10 13:35:06, Michael Achenbach wrote:
> > Does this work if the git repro in source folder uses git cache?
> 
> I'm not familiar with git cache. Isn't it provided by depot_tools? How would
it
> interfere with this?

Not sure if everything works as expected when making a mirror from something
that itself uses a cache mirror. But I suggest we try and find out :)

https://codereview.chromium.org/2744663005/diff/40001/tools/release/update_no...
File tools/release/update_node.py (right):

https://codereview.chromium.org/2744663005/diff/40001/tools/release/update_no...
tools/release/update_node.py:104: UpdateGitIgnore(options)
Possibility for confusion - not sure how it could be more robust.

1. node/deps/v8 is updated - this also copies v8's .gitignore and overwrites
node's.
2. git clean -fd is called to delete files (based on just updated .gitignore)

3. Now subrepos are updated, e.g. gtest

I think it works as it is now, but the order is important. If the git clean in
V8 is called in the end, it might purge more stuff.

[Michael Achenbach, 2017-03-13 10:41:02]

Fun fact: In gerrit I could have uploaded a patchset with the test to your CL :)

[Michael Achenbach, 2017-03-13 10:46:50]

Reupped the CL with a test here: https://codereview.chromium.org/2748623003 -
maybe you can incorporate it?

Required minimal change to the script to make it easier testable.

[Yang, 2017-03-13 13:07:35]

On 2017/03/13 10:46:50, Michael Achenbach wrote:
> Reupped the CL with a test here: https://codereview.chromium.org/2748623003 -
> maybe you can incorporate it?
> 
> Required minimal change to the script to make it easier testable.

Ah I see what you mean now. Thanks. Will do.

[Franzi, 2017-03-13 17:02:32]

lgtm

[Michael Achenbach, 2017-03-13 17:13:20]

lgtm for the script as is. the test could also land separately, up to you.

https://codereview.chromium.org/2744663005/diff/40001/tools/release/update_no...
File tools/release/update_node.py (right):

https://codereview.chromium.org/2744663005/diff/40001/tools/release/update_no...
tools/release/update_node.py:21: ADD_TO_GITIGNORE = [ "/testing/gtest/*",
Maybe add a comment/documentation somewhere what to do when adding a new deps'ed
dependency. At least for gtest it's not really intuitive how we got here.

[Yang, 2017-03-14 07:49:00]

https://codereview.chromium.org/2744663005/diff/40001/tools/release/update_no...
File tools/release/update_node.py (right):

https://codereview.chromium.org/2744663005/diff/40001/tools/release/update_no...
tools/release/update_node.py:21: ADD_TO_GITIGNORE = [ "/testing/gtest/*",
On 2017/03/13 17:13:20, Michael Achenbach wrote:
> Maybe add a comment/documentation somewhere what to do when adding a new
deps'ed
> dependency. At least for gtest it's not really intuitive how we got here.

I added a comment.

https://codereview.chromium.org/2744663005/diff/40001/tools/release/update_no...
tools/release/update_node.py:104: UpdateGitIgnore(options)
On 2017/03/13 10:33:23, Michael Achenbach wrote:
> Possibility for confusion - not sure how it could be more robust.
> 
> 1. node/deps/v8 is updated - this also copies v8's .gitignore and overwrites
> node's.
> 2. git clean -fd is called to delete files (based on just updated .gitignore)
> 
> 3. Now subrepos are updated, e.g. gtest
> 
> I think it works as it is now, but the order is important. If the git clean in
> V8 is called in the end, it might purge more stuff.
> 

I don't think .gitignore applies to 'git clean -fd' for-sub repositories, since
git clean is in the context of the temporary git repository created to sync.

But I changed the script nevertheless to be on the safe side.

[Yang, 2017-03-14 08:01:52]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2017-03-14 08:01:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
franzih@chromium.org, machenbach@chromium.org
Link to the patchset: https://codereview.chromium.org/2744663005/#ps60001
(title: "added test")

[commit-bot: I haz the power, 2017-03-14 08:02:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Michael Achenbach, 2017-03-14 08:20:25]

https://codereview.chromium.org/2744663005/diff/60001/tools/release/update_no...
File tools/release/update_node.py (right):

https://codereview.chromium.org/2744663005/diff/60001/tools/release/update_no...
tools/release/update_node.py:35: if subprocess.call("gclient sync --nohooks",
cwd=path, shell=True) != 0:
nit: you can use a check_call to get this behavior.

https://codereview.chromium.org/2744663005/diff/60001/tools/release/update_no...
tools/release/update_node.py:49: if not os.path.exists(target):
just a remark - if we ever add a new folder structure with depth two, e.g.
bar/foo this will fail as it will attempt to make foo in bar and will complain
that bar doesn't exist.

[Yang, 2017-03-14 08:20:46]

The CQ bit was unchecked by yangguo@chromium.org

[Yang, 2017-03-14 08:21:28]

Description was changed from

==========
Add script to update relevant changes to Node.js.

R=franzih@chromium.org, machenbach@chromium.org, ofrobots@google.com
==========

to

==========
Add script to update relevant changes to Node.js.

NOTRY=true

R=franzih@chromium.org, machenbach@chromium.org, ofrobots@google.com
==========

[Yang, 2017-03-14 08:24:12]

Addressed nits.

https://codereview.chromium.org/2744663005/diff/60001/tools/release/update_no...
File tools/release/update_node.py (right):

https://codereview.chromium.org/2744663005/diff/60001/tools/release/update_no...
tools/release/update_node.py:35: if subprocess.call("gclient sync --nohooks",
cwd=path, shell=True) != 0:
On 2017/03/14 08:20:25, Michael Achenbach wrote:
> nit: you can use a check_call to get this behavior.

Done.

https://codereview.chromium.org/2744663005/diff/60001/tools/release/update_no...
tools/release/update_node.py:49: if not os.path.exists(target):
On 2017/03/14 08:20:25, Michael Achenbach wrote:
> just a remark - if we ever add a new folder structure with depth two, e.g.
> bar/foo this will fail as it will attempt to make foo in bar and will complain
> that bar doesn't exist.

os.makedirs creates folders recursively though.

[Yang, 2017-03-14 08:24:17]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2017-03-14 08:24:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
franzih@chromium.org, machenbach@chromium.org
Link to the patchset: https://codereview.chromium.org/2744663005/#ps80001
(title: "address nits")

[commit-bot: I haz the power, 2017-03-14 08:24:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Michael Achenbach, 2017-03-14 08:24:50]

I assume markupsafe and jinja test files are only added for the script to not
bail out under test? Note, that they're not gitified like gtest in the test...

[commit-bot: I haz the power, 2017-03-14 08:26:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489479857288950,
"parent_rev": "2cd2f5feff3a811eb1d2a69996f8be9f501c44a5", "commit_rev":
"f52a483305b2c524b6f2fa9066c8fb097fc11a47"}

[commit-bot: I haz the power, 2017-03-14 08:26:17]

Description was changed from

==========
Add script to update relevant changes to Node.js.

NOTRY=true

R=franzih@chromium.org, machenbach@chromium.org, ofrobots@google.com
==========

to

==========
Add script to update relevant changes to Node.js.

NOTRY=true

R=franzih@chromium.org, machenbach@chromium.org, ofrobots@google.com

Review-Url: https://codereview.chromium.org/2744663005
Cr-Commit-Position: refs/heads/master@{#43764}
Committed:
https://chromium.googlesource.com/v8/v8/+/f52a483305b2c524b6f2fa9066c8fb097fc...
==========

[commit-bot: I haz the power, 2017-03-14 08:26:19]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/v8/v8/+/f52a483305b2c524b6f2fa9066c8fb097fc...

[Yang, 2017-03-14 08:26:27]

https://codereview.chromium.org/2744663005/diff/80001/tools/release/test_upda...
File tools/release/test_update_node.py (right):

https://codereview.chromium.org/2744663005/diff/80001/tools/release/test_upda...
tools/release/test_update_node.py:71: gitify(os.path.join(v8_cwd, *repository))
They are gitified here :)

[Michael Achenbach, 2017-03-14 08:37:26]

https://codereview.chromium.org/2744663005/diff/60001/tools/release/update_no...
File tools/release/update_node.py (right):

https://codereview.chromium.org/2744663005/diff/60001/tools/release/update_no...
tools/release/update_node.py:49: if not os.path.exists(target):
On 2017/03/14 08:24:12, Yang wrote:
> On 2017/03/14 08:20:25, Michael Achenbach wrote:
> > just a remark - if we ever add a new folder structure with depth two, e.g.
> > bar/foo this will fail as it will attempt to make foo in bar and will
complain
> > that bar doesn't exist.
> 
> os.makedirs creates folders recursively though.

You're right! Learned something today. Then there's another method in Python
somewhere that had problems with that... maybe something in shutil...

https://codereview.chromium.org/2744663005/diff/80001/tools/release/test_upda...
File tools/release/test_update_node.py (right):

https://codereview.chromium.org/2744663005/diff/80001/tools/release/test_upda...
tools/release/test_update_node.py:38: 'testing/gtest/baz/gtest_new',
nit: The files from marupsafe and jinja2 could be added here too.

https://codereview.chromium.org/2744663005/diff/80001/tools/release/test_upda...
tools/release/test_update_node.py:71: gitify(os.path.join(v8_cwd, *repository))
On 2017/03/14 08:26:27, Yang wrote:
> They are gitified here :)

Ah right, then this nit:
Unused variable gtest_cwd above :) - just looked at that line...

[Michael Achenbach, 2017-03-14 08:50:58]

Any tracker bug for work on this?

[Yang, 2017-03-14 09:28:03]

On 2017/03/14 08:50:58, Michael Achenbach wrote:
> Any tracker bug for work on this?

It does now: https://bugs.chromium.org/p/v8/issues/detail?id=6091

[Yang, 2017-03-14 09:28:14]

Description was changed from

==========
Add script to update relevant changes to Node.js.

NOTRY=true

R=franzih@chromium.org, machenbach@chromium.org, ofrobots@google.com

Review-Url: https://codereview.chromium.org/2744663005
Cr-Commit-Position: refs/heads/master@{#43764}
Committed:
https://chromium.googlesource.com/v8/v8/+/f52a483305b2c524b6f2fa9066c8fb097fc...
==========

to

==========
Add script to update relevant changes to Node.js.

NOTRY=true
BUG=v8:6091
R=franzih@chromium.org, machenbach@chromium.org, ofrobots@google.com

Review-Url: https://codereview.chromium.org/2744663005
Cr-Commit-Position: refs/heads/master@{#43764}
Committed:
https://chromium.googlesource.com/v8/v8/+/f52a483305b2c524b6f2fa9066c8fb097fc...
==========

[Michael Achenbach, 2017-03-16 11:29:19]

https://codereview.chromium.org/2744663005/diff/80001/tools/release/update_no...
File tools/release/update_node.py (right):

https://codereview.chromium.org/2744663005/diff/80001/tools/release/update_no...
tools/release/update_node.py:15: ["third_party", "jinja2"],
Why isn't base/trace_event/common added here? It's depsed into V8:
https://cs.chromium.org/chromium/src/v8/DEPS?l=20

And it is checked into node. I assume we would miss updates to it? The clean
command below is executed before altering the .gitignore, like that, node's
version of it is not cleaned up.