DevTools: Create recipe and builder for devtools

DevTools: Create recipe and builder for devtools

The goal is to run eslint if the most recent patch changes any devtools file.
Initially, we'll start off by installing node.js and npm in a local directory,
but the plan is to eventually rely on a global node.js installation.

BUG=642046
Committed: https://chromium.googlesource.com/chromium/tools/build/+/356a3d2a273a3ce2872a92d90f4ed1ef46fadf6e

[chenwilliam, 2016-08-30 01:27:54]

chenwilliam@chromium.org changed reviewers:
+ hinoka@chromium.org

[chenwilliam, 2016-08-30 01:27:55]

This is a first draft of the recipe for devtools. I'm not a 100% confident that
it works, but I tried running it locally and it seems to work.

https://codereview.chromium.org/2290923002/diff/1/scripts/slave/recipes/devto...
File scripts/slave/recipes/devtools.py (right):

https://codereview.chromium.org/2290923002/diff/1/scripts/slave/recipes/devto...
scripts/slave/recipes/devtools.py:25: files =
api.chromium_tests.get_files_affected_by_patch()
What I'm trying to do is figure out if the most recent patch has any devtools
files in it. I'm not sure if this is the right way.

[Ryan Tseng, 2016-08-30 18:13:38]

hinoka@google.com changed reviewers:
+ hinoka@google.com

[Ryan Tseng, 2016-08-30 18:13:39]

https://codereview.chromium.org/2290923002/diff/1/scripts/slave/recipes/devto...
File scripts/slave/recipes/devtools.py (right):

https://codereview.chromium.org/2290923002/diff/1/scripts/slave/recipes/devto...
scripts/slave/recipes/devtools.py:25: files =
api.chromium_tests.get_files_affected_by_patch()
On 2016/08/30 01:27:55, chenwilliam wrote:
> What I'm trying to do is figure out if the most recent patch has any devtools
> files in it. I'm not sure if this is the right way.

I'm not super familiar with this but it looks right.

https://codereview.chromium.org/2290923002/diff/1/scripts/slave/recipes/devto...
scripts/slave/recipes/devtools.py:34: devtoolsPath =
api.path['checkout'].join('third_party', 'WebKit', 'Source',
Did you get any python coverage errors here? I don't think this line is covered.

https://codereview.chromium.org/2290923002/diff/1/scripts/slave/recipes/devto...
scripts/slave/recipes/devtools.py:36: npmPath =
api.path['checkout'].join('third_party', 'WebKit', 'Source', 'devtools',
80 char

https://codereview.chromium.org/2290923002/diff/1/scripts/slave/recipes/devto...
scripts/slave/recipes/devtools.py:37: 'scripts', 'nodejs', 'npm.py')
where's this from? I can't find it

[chenwilliam, 2016-09-01 18:00:21]

Description was changed from

==========
DevTools: Create recipe to run eslint if patch contains devtools file

The goal is to run eslint if the most recent patch changes any devtools file.
Initially, we'll start off by installing node.js and npm in a local directory,
but the plan is to eventually rely on a global node.js installation.

BUG=642046
==========

to

==========
DevTools: Create recipe and builder for devtools

The goal is to run eslint if the most recent patch changes any devtools file.
Initially, we'll start off by installing node.js and npm in a local directory,
but the plan is to eventually rely on a global node.js installation.

BUG=642046
==========

[chenwilliam, 2016-09-01 18:00:45]

PTAL. I improved the recipe's test coverage and added config for new builder.
Let me know if I'm missing anything to setup a new builder.

https://codereview.chromium.org/2290923002/diff/1/scripts/slave/recipes/devto...
File scripts/slave/recipes/devtools.py (right):

https://codereview.chromium.org/2290923002/diff/1/scripts/slave/recipes/devto...
scripts/slave/recipes/devtools.py:34: devtoolsPath =
api.path['checkout'].join('third_party', 'WebKit', 'Source',
On 2016/08/30 18:13:39, Ryan Tseng wrote:
> Did you get any python coverage errors here? I don't think this line is
covered.

Added coverage.

https://codereview.chromium.org/2290923002/diff/1/scripts/slave/recipes/devto...
scripts/slave/recipes/devtools.py:36: npmPath =
api.path['checkout'].join('third_party', 'WebKit', 'Source', 'devtools',
On 2016/08/30 18:13:39, Ryan Tseng wrote:
> 80 char

Done.

https://codereview.chromium.org/2290923002/diff/1/scripts/slave/recipes/devto...
scripts/slave/recipes/devtools.py:37: 'scripts', 'nodejs', 'npm.py')
On 2016/08/30 18:13:39, Ryan Tseng wrote:
> where's this from? I can't find it

Just landed in https://codereview.chromium.org/2278803004/

https://codereview.chromium.org/2290923002/diff/20001/scripts/slave/recipes/d...
File scripts/slave/recipes/devtools.py (right):

https://codereview.chromium.org/2290923002/diff/20001/scripts/slave/recipes/d...
scripts/slave/recipes/devtools.py:51: api.git.checkout(
Changed from npm install to just checking out our npm_modules repo.

[Ryan Tseng, 2016-09-01 23:57:56]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 23:58:04]

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

[commit-bot: I haz the power, 2016-09-02 00:00:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 00:00:15]

Dry run: Try jobs failed on following builders:
  Build Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/310101daa1b5a010)

[Ryan Tseng, 2016-09-02 00:12:59]

Can you explain why you're deleting a folder just to replace it with another
checkout?

Also I think you need to update and rebase.

https://codereview.chromium.org/2290923002/diff/20001/scripts/slave/recipes/d...
File scripts/slave/recipes/devtools.py (right):

https://codereview.chromium.org/2290923002/diff/20001/scripts/slave/recipes/d...
scripts/slave/recipes/devtools.py:18: 'buildername': 'DevTools',
update this

https://codereview.chromium.org/2290923002/diff/20001/scripts/slave/recipes/d...
scripts/slave/recipes/devtools.py:56: api.shutil.rmtree(node_modules_dest_path)
why?

[chenwilliam, 2016-09-02 18:01:25]

PTAL. Thanks!

https://codereview.chromium.org/2290923002/diff/20001/scripts/slave/recipes/d...
File scripts/slave/recipes/devtools.py (right):

https://codereview.chromium.org/2290923002/diff/20001/scripts/slave/recipes/d...
scripts/slave/recipes/devtools.py:18: 'buildername': 'DevTools',
On 2016/09/02 00:12:59, Ryan Tseng wrote:
> update this

Done.

https://codereview.chromium.org/2290923002/diff/20001/scripts/slave/recipes/d...
scripts/slave/recipes/devtools.py:56: api.shutil.rmtree(node_modules_dest_path)
On 2016/09/02 00:12:59, Ryan Tseng wrote:
> why?

If the npm_modules repo gets updated, I want to make sure we're using the latest
node modules. This is a naive approach since most of the time between build
runs, the node modules won't be different, but it's simple.

I wanted to execute the node modules from the repo checkout directly, but npm
expects them to be in a certain directory. (E.g. running eslint in devtools
directory, it should be in devtools/node_modules/eslint).

I thought of symlinks but I can't find a recipe module for it (I looked at
shutil and file) and symlinks won't work in Windows, which we'll eventually want
to use.

Since our node_modules folder is not very big, this will work OK in the
short-term but let me know if you have a better solution.

[Ryan Tseng, 2016-09-02 18:10:47]

https://codereview.chromium.org/2290923002/diff/20001/scripts/slave/recipes/d...
File scripts/slave/recipes/devtools.py (right):

https://codereview.chromium.org/2290923002/diff/20001/scripts/slave/recipes/d...
scripts/slave/recipes/devtools.py:56: api.shutil.rmtree(node_modules_dest_path)
On 2016/09/02 18:01:25, chenwilliam wrote:
> On 2016/09/02 00:12:59, Ryan Tseng wrote:
> > why?
> 
> If the npm_modules repo gets updated, I want to make sure we're using the
latest
> node modules. This is a naive approach since most of the time between build
> runs, the node modules won't be different, but it's simple.
> 
> I wanted to execute the node modules from the repo checkout directly, but npm
> expects them to be in a certain directory. (E.g. running eslint in devtools
> directory, it should be in devtools/node_modules/eslint).
> 
> I thought of symlinks but I can't find a recipe module for it (I looked at
> shutil and file) and symlinks won't work in Windows, which we'll eventually
want
> to use.
> 
> Since our node_modules folder is not very big, this will work OK in the
> short-term but let me know if you have a better solution.

why not just deps in the npm_modules repository into that path?

[Ryan Tseng, 2016-09-02 18:49:04]

lgtm % add comments specified below

https://codereview.chromium.org/2290923002/diff/80001/scripts/slave/recipes/d...
File scripts/slave/recipes/devtools.py (right):

https://codereview.chromium.org/2290923002/diff/80001/scripts/slave/recipes/d...
scripts/slave/recipes/devtools.py:55: api.git.checkout(
add todo to add this to DEPS

https://codereview.chromium.org/2290923002/diff/80001/scripts/slave/recipes/d...
scripts/slave/recipes/devtools.py:57: ref='master',
May want to pin this?

https://codereview.chromium.org/2290923002/diff/80001/scripts/slave/recipes/d...
scripts/slave/recipes/devtools.py:60: api.shutil.rmtree(node_modules_dest_path)
Document why you have to do this

[chenwilliam, 2016-09-02 20:50:37]

The CQ bit was checked by chenwilliam@chromium.org

[chenwilliam, 2016-09-02 20:50:38]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@google.com
Link to the patchset: https://codereview.chromium.org/2290923002/#ps100001
(title: "add comments to devtools.py")

[commit-bot: I haz the power, 2016-09-02 20:50:44]

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

[chenwilliam, 2016-09-02 20:51:04]

Added comments.

https://codereview.chromium.org/2290923002/diff/80001/scripts/slave/recipes/d...
File scripts/slave/recipes/devtools.py (right):

https://codereview.chromium.org/2290923002/diff/80001/scripts/slave/recipes/d...
scripts/slave/recipes/devtools.py:55: api.git.checkout(
On 2016/09/02 18:49:04, Ryan Tseng wrote:
> add todo to add this to DEPS

Done.

https://codereview.chromium.org/2290923002/diff/80001/scripts/slave/recipes/d...
scripts/slave/recipes/devtools.py:57: ref='master',
On 2016/09/02 18:49:04, Ryan Tseng wrote:
> May want to pin this?
Added a todo.

https://codereview.chromium.org/2290923002/diff/80001/scripts/slave/recipes/d...
scripts/slave/recipes/devtools.py:60: api.shutil.rmtree(node_modules_dest_path)
On 2016/09/02 18:49:04, Ryan Tseng wrote:
> Document why you have to do this

Done.

[commit-bot: I haz the power, 2016-09-02 20:55:01]

Description was changed from

==========
DevTools: Create recipe and builder for devtools

The goal is to run eslint if the most recent patch changes any devtools file.
Initially, we'll start off by installing node.js and npm in a local directory,
but the plan is to eventually rely on a global node.js installation.

BUG=642046
==========

to

==========
DevTools: Create recipe and builder for devtools

The goal is to run eslint if the most recent patch changes any devtools file.
Initially, we'll start off by installing node.js and npm in a local directory,
but the plan is to eventually rely on a global node.js installation.

BUG=642046

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/356a3d2a273a3ce2872a...
==========

[commit-bot: I haz the power, 2016-09-02 20:55:02]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/tools/build/+/356a3d2a273a3ce2872a...