'recipes.py isolate' command to build an isolate of the current package's toolchain

'recipes.py isolate' command to build an isolate of the current package's toolchain

BUG=459840
R=iannucci@chromium.org,martiniss@chromium.org,maruel@chromium.org

[luqui, 2016-01-09 00:37:14]

ptal

[M-A Ruel, 2016-01-09 00:52:00]

https://codereview.chromium.org/1570333002/diff/1/recipe_engine/package.py
File recipe_engine/package.py (right):

https://codereview.chromium.org/1570333002/diff/1/recipe_engine/package.py#ne...
recipe_engine/package.py:375: """Returns a collection of directories containing
recipes."""
Returns the list of

https://codereview.chromium.org/1570333002/diff/1/recipe_engine/package.py#ne...
recipe_engine/package.py:380: """Returns a collection of directories containing
recipe modules."""
same

https://codereview.chromium.org/1570333002/diff/1/recipe_engine/package.py#ne...
recipe_engine/package.py:401: """Returns a generator listing all file paths this
package needs to run."""
Yields all file ...

https://codereview.chromium.org/1570333002/diff/1/recipe_engine/package.py#ne...
recipe_engine/package.py:410: dirs = list(self.recipe_dirs) +
list(self.module_dirs)
The the two properties return a list, I don't think it's needed to create 2
temporary copies

https://codereview.chromium.org/1570333002/diff/1/recipe_engine/package.py#ne...
recipe_engine/package.py:427: if not EXCLUDE.search(fname):
a regexp is overkill here when this would be fine:
if not fname.endswith('.pyc'):

https://codereview.chromium.org/1570333002/diff/1/recipe_engine/package.py#ne...
recipe_engine/package.py:430: for dep in self.deps.values():
itervalues

https://codereview.chromium.org/1570333002/diff/1/recipe_engine/package.py#ne...
recipe_engine/package.py:434: def _normalize_extra_line(self, line):
This doesn't need to be a method. IMHO, it'd be more readable if you inlined it.

https://codereview.chromium.org/1570333002/diff/1/recipes.py
File recipes.py (right):

https://codereview.chromium.org/1570333002/diff/1/recipes.py#newcode227
recipes.py:227: def norm(p):
I would have written it as:
norm = lambda *p: os.path.relpath(os.path.join(*p), base_path)

then line 229 becomes:
recipes_tool = norm(package_deps.get_package('recipe_engine').recipes_dir,
'recipes.py')

https://codereview.chromium.org/1570333002/diff/1/recipes.py#newcode234
recipes.py:234: 'files': [norm(f) for f in
package_deps.root_package.all_files(context)],
sorted()

https://codereview.chromium.org/1570333002/diff/1/recipes.py#newcode238
recipes.py:238: '--package', norm(
put the norm()'ed item in a separate line.

https://codereview.chromium.org/1570333002/diff/1/recipes.py#newcode243
recipes.py:243: }, indent=2, separators=(',', ': '))
return 0
2 empty lines

https://codereview.chromium.org/1570333002/diff/1/unittests/package_setup_tes...
File unittests/package_setup_test.py (right):

https://codereview.chromium.org/1570333002/diff/1/unittests/package_setup_tes...
unittests/package_setup_test.py:59: os.path.join(ROOT_DIR, 'recipes.py'),
sys.executable,

[martiniss, 2016-01-11 19:43:17]

https://codereview.chromium.org/1570333002/diff/20001/recipes.extra
File recipes.extra (right):

https://codereview.chromium.org/1570333002/diff/20001/recipes.extra#newcode1
recipes.extra:1: recipe_engine
I have no clue what this file does, from first seeing the file. Would like this
to be documented somehow... allow comments in the file? Put something in the
README?

[iannucci, 2016-01-15 04:19:46]

On 2016/01/11 at 19:43:17, martiniss wrote:
> https://codereview.chromium.org/1570333002/diff/20001/recipes.extra
> File recipes.extra (right):
> 
> https://codereview.chromium.org/1570333002/diff/20001/recipes.extra#newcode1
> recipes.extra:1: recipe_engine
> I have no clue what this file does, from first seeing the file. Would like
this to be documented somehow... allow comments in the file? Put something in
the README?

Is this CL WIP or ready to rock?

[luqui, 2016-01-15 23:11:27]

ready to rock

https://chromiumcodereview.appspot.com/1570333002/diff/1/recipe_engine/packag...
File recipe_engine/package.py (right):

https://chromiumcodereview.appspot.com/1570333002/diff/1/recipe_engine/packag...
recipe_engine/package.py:375: """Returns a collection of directories containing
recipes."""
On 2016/01/09 00:51:59, M-A Ruel wrote:
> Returns the list of

Done.

https://chromiumcodereview.appspot.com/1570333002/diff/1/recipe_engine/packag...
recipe_engine/package.py:380: """Returns a collection of directories containing
recipe modules."""
On 2016/01/09 00:51:59, M-A Ruel wrote:
> same 

Done.

https://chromiumcodereview.appspot.com/1570333002/diff/1/recipe_engine/packag...
recipe_engine/package.py:401: """Returns a generator listing all file paths this
package needs to run."""
On 2016/01/09 00:51:59, M-A Ruel wrote:
> Yields all file ...

Done.

https://chromiumcodereview.appspot.com/1570333002/diff/1/recipe_engine/packag...
recipe_engine/package.py:410: dirs = list(self.recipe_dirs) +
list(self.module_dirs)
On 2016/01/09 00:51:59, M-A Ruel wrote:
> The the two properties return a list, I don't think it's needed to create 2
> temporary copies

Good obs. One upon a time they were generators.

https://chromiumcodereview.appspot.com/1570333002/diff/1/recipe_engine/packag...
recipe_engine/package.py:427: if not EXCLUDE.search(fname):
On 2016/01/09 00:51:59, M-A Ruel wrote:
> a regexp is overkill here when this would be fine:
> if not fname.endswith('.pyc'):

I want the idea of EXCLUDE to be more visible than 3 levels nested in a loop, in
case people are wondering why their pycs aren't getting included.

https://chromiumcodereview.appspot.com/1570333002/diff/1/recipe_engine/packag...
recipe_engine/package.py:430: for dep in self.deps.values():
On 2016/01/09 00:51:59, M-A Ruel wrote:
> itervalues

Done.

https://chromiumcodereview.appspot.com/1570333002/diff/1/recipe_engine/packag...
recipe_engine/package.py:434: def _normalize_extra_line(self, line):
On 2016/01/09 00:51:59, M-A Ruel wrote:
> This doesn't need to be a method. IMHO, it'd be more readable if you inlined
it.

Ah yeah, and get rid of the imap.  Agreed, Done.

https://chromiumcodereview.appspot.com/1570333002/diff/1/recipes.py
File recipes.py (right):

https://chromiumcodereview.appspot.com/1570333002/diff/1/recipes.py#newcode234
recipes.py:234: 'files': [norm(f) for f in
package_deps.root_package.all_files(context)],
On 2016/01/09 00:51:59, M-A Ruel wrote:
> sorted()

Done.

https://chromiumcodereview.appspot.com/1570333002/diff/1/recipes.py#newcode238
recipes.py:238: '--package', norm(
On 2016/01/09 00:51:59, M-A Ruel wrote:
> put the norm()'ed item in a separate line.

Done.

https://chromiumcodereview.appspot.com/1570333002/diff/1/recipes.py#newcode243
recipes.py:243: }, indent=2, separators=(',', ': '))
On 2016/01/09 00:51:59, M-A Ruel wrote:
> return 0
> 2 empty lines

Done.

https://chromiumcodereview.appspot.com/1570333002/diff/20001/recipes.extra
File recipes.extra (right):

https://chromiumcodereview.appspot.com/1570333002/diff/20001/recipes.extra#ne...
recipes.extra:1: recipe_engine
On 2016/01/11 19:43:17, martiniss wrote:
> I have no clue what this file does, from first seeing the file. Would like
this
> to be documented somehow... allow comments in the file? Put something in the
> README?

Done.

[M-A Ruel, 2016-01-16 00:42:39]

https://chromiumcodereview.appspot.com/1570333002/diff/40001/recipe_engine/pa...
File recipe_engine/package.py (right):

https://chromiumcodereview.appspot.com/1570333002/diff/40001/recipe_engine/pa...
recipe_engine/package.py:428: if not EXCLUDE.search(fname):
if not fname.endswith('.pyc'):

is much more readable and shorter, you can delete line 408-409.

https://chromiumcodereview.appspot.com/1570333002/diff/40001/recipes.py
File recipes.py (right):

https://chromiumcodereview.appspot.com/1570333002/diff/40001/recipes.py#newco...
recipes.py:233: overrides = [ '-O %s=%s' % (k, norm(v))
formatting is inconsistent

https://chromiumcodereview.appspot.com/1570333002/diff/40001/recipes.py#newco...
recipes.py:234: for k,v in args.project_override.iteritems() ]
sorted()
so the output is deterministic

https://chromiumcodereview.appspot.com/1570333002/diff/40001/recipes.py#newco...
recipes.py:248: }, indent=2, separators=(',', ': '))
sort_keys=True,

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
File unittests/package_setup_test.py (right):

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
unittests/package_setup_test.py:202: self.assertEqual(set(spec.keys()),
set(['variables']))
remove .keys(), not needed

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
unittests/package_setup_test.py:204: set(['command', 'files']))
{'command', 'files'}

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
unittests/package_setup_test.py:205: files = list(spec['variables']['files'])
why convert to a list first?

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
unittests/package_setup_test.py:206: self.assertTrue(not any( i.endswith('.pyc')
for i in files ))
no space between ( and i
why not assertNotIn?

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
unittests/package_setup_test.py:214: set(['infra/config/recipes.cfg',
{}

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
unittests/package_setup_test.py:236: 
2 lines

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
unittests/package_setup_test.py:238: return ([ item for item in items if
pred(item) ],
I'd use i instead of items, it'd fit one line and be as readable.

[luqui, 2016-01-16 03:55:30]

addressed

https://chromiumcodereview.appspot.com/1570333002/diff/40001/recipe_engine/pa...
File recipe_engine/package.py (right):

https://chromiumcodereview.appspot.com/1570333002/diff/40001/recipe_engine/pa...
recipe_engine/package.py:428: if not EXCLUDE.search(fname):
On 2016/01/16 00:42:39, M-A Ruel wrote:
> if not fname.endswith('.pyc'):
> 
> is much more readable and shorter, you can delete line 408-409.

I already replied to this comment.

https://chromiumcodereview.appspot.com/1570333002/diff/40001/recipes.py
File recipes.py (right):

https://chromiumcodereview.appspot.com/1570333002/diff/40001/recipes.py#newco...
recipes.py:233: overrides = [ '-O %s=%s' % (k, norm(v))
On 2016/01/16 00:42:39, M-A Ruel wrote:
> formatting is inconsistent

Fixed... I think?

https://chromiumcodereview.appspot.com/1570333002/diff/40001/recipes.py#newco...
recipes.py:234: for k,v in args.project_override.iteritems() ]
On 2016/01/16 00:42:39, M-A Ruel wrote:
> sorted()
> so the output is deterministic

Done.

https://chromiumcodereview.appspot.com/1570333002/diff/40001/recipes.py#newco...
recipes.py:248: }, indent=2, separators=(',', ': '))
On 2016/01/16 00:42:39, M-A Ruel wrote:
> sort_keys=True,

Done.

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
File unittests/package_setup_test.py (right):

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
unittests/package_setup_test.py:202: self.assertEqual(set(spec.keys()),
set(['variables']))
On 2016/01/16 00:42:39, M-A Ruel wrote:
> remove .keys(), not needed

Done.

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
unittests/package_setup_test.py:204: set(['command', 'files']))
On 2016/01/16 00:42:39, M-A Ruel wrote:
> {'command', 'files'}

no kidding?  I didn't know about that

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
unittests/package_setup_test.py:205: files = list(spec['variables']['files'])
On 2016/01/16 00:42:39, M-A Ruel wrote:
> why convert to a list first?

Ah, leftover from when I was modifying files (which indeed was still not
strictly necessary, but I like to make local copies whenever I treat a variable
as mutable to ease locality of reasoning).  Anyway fixed.

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
unittests/package_setup_test.py:206: self.assertTrue(not any( i.endswith('.pyc')
for i in files ))
On 2016/01/16 00:42:39, M-A Ruel wrote:
> no space between ( and i
Done.
> why not assertNotIn?
Because it doesn't really fit the pattern.  Like self.assertNotIn(True,
i.endswith...) ?  I just find it awkward.

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
unittests/package_setup_test.py:214: set(['infra/config/recipes.cfg',
On 2016/01/16 00:42:39, M-A Ruel wrote:
> {}

Done.

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
unittests/package_setup_test.py:236: 
On 2016/01/16 00:42:39, M-A Ruel wrote:
> 2 lines

Done.

https://chromiumcodereview.appspot.com/1570333002/diff/40001/unittests/packag...
unittests/package_setup_test.py:238: return ([ item for item in items if
pred(item) ],
On 2016/01/16 00:42:39, M-A Ruel wrote:
> I'd use i instead of items, it'd fit one line and be as readable.

Done.

[M-A Ruel, 2016-01-16 23:30:48]

https://chromiumcodereview.appspot.com/1570333002/diff/60001/recipe_engine/pa...
File recipe_engine/package.py (right):

https://chromiumcodereview.appspot.com/1570333002/diff/60001/recipe_engine/pa...
recipe_engine/package.py:416: fname = re.sub(r'#.*', '', line).replace('/',
os.sep).strip()
You should use r'^#.*' otherwise no file name can contain '#'.

I'd prefer you to add:
if not name:
  continue

otherwise you are adding the self.recipes_dir due to comments.

https://chromiumcodereview.appspot.com/1570333002/diff/60001/recipes.py
File recipes.py (right):

https://chromiumcodereview.appspot.com/1570333002/diff/60001/recipes.py#newco...
recipes.py:234: '-O %s=%s' % (k, norm(v)) for k, v in
args.project_override.iteritems())
Actually, you want ['-O', '%s=%s'] otherwise this will be quoted. A way to write
this is:

overrides = sum(
    (['-O', '%s=%s' % (k, norm(v))] for k, v in
sorted(args.project_override.iteritems())),
    [])

[martiniss, 2016-03-16 01:11:30]

Should I take this over? We need this, correct?

[M-A Ruel, 2016-03-16 12:56:12]

Description was changed from

==========
'recipes.py isolate' command to build an isolate of the current package's
toolchain

BUG=459840
R=iannucci@chromium.org,martiniss@chromium.org,maruel@chromium.org
==========

to

==========
'recipes.py isolate' command to build an isolate of the current package's
toolchain

BUG=459840
R=iannucci@chromium.org,martiniss@chromium.org,maruel@chromium.org
==========

[M-A Ruel, 2016-03-16 12:57:28]

On 2016/03/16 01:11:30, martiniss wrote:
> Should I take this over? We need this, correct?

cc'ing pawel, erik and nodir

I don't know how it fits with kitchen.

[estaab, 2016-03-29 08:43:21]

This CL on it's own doesn't solve the bootstrapping problem since something
would need the full checkout to run recipes.py isolate.

Robbie and I talked about wrapping up some gitiles requests into a pretty
class that basically just looks like:
recipe_fetcher = GitilesBasedFetcher(repo_url, revision)
dir = tempfile.mkdtemp()
recipe_fetcher.Fetch(manifest_or_isolate_or_something, dir)
# do something with the downloaded recipes in 'dir'

This would let us avoid fetching a full, potentially massive checkout (e.g.
chromium) and let us specify files outside of some recipe root being used
by some misbehaving recipe_modules / recipes.

But really what we might want in the longer term is isolating as a service,
i.e. some magic RPC rpc_call(isolate_server, repo, revision, isolate_path)
-> isolate_hash where the git client state is handled by the service.
Maybe? I can dream.

On Wed, Mar 16, 2016 at 9:57 PM, <maruel@chromium.org> wrote:

> On 2016/03/16 01:11:30, martiniss wrote:
> > Should I take this over? We need this, correct?
>
> cc'ing pawel, erik and nodir
>
> I don't know how it fits with kitchen.
>
> https://codereview.chromium.org/1570333002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.