Just a few main issues fwict:

Tasks - many instances of these issues - I pointed out just a few in the review below:
1.) for each Task that today calls self.context.products.require* and/or accesses self.context.products in execute to retrieve product mappings, an override of prepare is required to schedule each one of the product types required/used.
2.) for each product mapping written in execute, and only for those product types and no others, Tasks should report product_types.

RoundEngine:
3.) The RoundManager should do context.products.require* on the Tasks' behalf so the Tasks can ask for products in 1 place.

Otherwise this lgtm.  Product promises can come later - I'd enjoy sending you that review later after round 2 - subgraphs goes in.
 

AntlrGen does grammar -> .java so the product type is more like 'sources'.  That said - sources are special currently as products and we map them by injecting synthetic targets owning them into the build graph instead of using the products registry.  I'd stick to only advertising product_type for Tasks that register items in the context.products in the execute call flow today.  Making this a uniform way to communicate products is for follow up.

This is too much detail - no where else are these product types accessed in this class.  Up above self.register_jvm_tool(compiler, tools) is called and down below self.tool_classpath(compiler) is called indicating the actual interaction with the 'jvm_build_tools*' products happens in a base class.  This prepare should move there.

This task does not access 'classes_by_source' in the context.products - kill.

This call should not be needed.  So this RB adds RoundManager.schedule which stands to the side of context.products; however the idea is they become one - or 1 interface.  It would be good to combine this call and schedule for 1 way to require a product mapping be made available in execute.  It may make sense to change round_manager.schedule to hand a method-per-products.require* for ease of transition.  So here this line would go and in prepare:
  round_manager.require_data('exclusives_groups')

This implies invalidation/caching is per Task and not per Task per product type.  That's how things are done today anyway, but this stands out as arbitrary.  No suggestions, just talking out loud.

Its worth noting the new part of the contract - that this is a well-known name other Tasks can bind to to get product mapping - thus its a public api.  The doc as it stands is only focused on the jvm group case where javac and scalac produce classes both IIUC.

'jvm_build_tools' is required_data below so it can't be a product of this Task.

and context.products.require* call implies a prepare is needed where the requirement moves to a schedule call on the round_manager for the product mapping.

Not true - thats provided by check_exclusives.py

Forgot - it would also be great to do a clean switch over to RoundEngine.  So make round_manager a required non-None arg to prepare now and fixup the RoundEngine or just the test to ensure constructors are called in the same order prepare is.  Today most tasks rely on __init__ being called in prepare order.  IE: I introduced prepare but did not convert Tasks to it, instead LinearEngine calls (__init__, prepare), (__init__, prepare), ...  Keep that semantic for the 1st change and the risk is low here since the key guaranty of orderings in the Task lifecycle will be maintained.  The only risky bit is getting all the product_type lists and prepare round_manager schedules faithfully reflecting the context.products uses. 

I some high level questions:

 - Why does it run 'gen' over the entire graph. Shouldn't we just need to run 'gen' over the graph that the target or targets depend on?  

 - When 'pants goal idea <target>' was working, it seemed to do more work than I thought it had to.  What is the purpose of the 'compile' step?  When you are authoring new functionality  or trying to bring up the IDE on a broken tree, the fail of compile seemed to preclude the ide config being generated correctly.  This is a minor annoyance when all of your java code is under /src/java as in the Pants repo, but not really recoverable when you have a tree with hundreds of java source roots (maven style layout). 

Does every code generator need updating like this? I didn't see jaxb_gen.py (we recently added this) in the list.  

First of all, I am super-excited for this change. This will be a significant improvement over manually registering phase deps. 

I have several comments, with the caveat that I may be misunderstanding the design, and where it's heading. See below for details inline.

Cheers!

For the apparently common case where the scheduled products are hard-coded per type, we could have a classmethod that returns them, and the Task base class could use that as its default implementation of prepare(). It would save a little bit of boilerplate.

OrderedSet? Since you test for membership below.

This comment seems stale - there's no way to specify a graph, and there are no phases here, just products.

You don't need to construct an instance here. goal.task_type.product_type() will work, as it's a classmethod.

phases_by_product can be a defaultdict(set) or (defaultdict(OrderedSet) if order matters) and then you won't need this check.

It's a bit weird to modify a mutable argument, that would need to be well documented. 

Or you could nest this method inside _prepare(), so it can access phases and phases_by_product from its closure. That would be more intuitive I think.

round_manager could be local to this method, right? 

Is phase the right resolution for rounds, or should it be goals? In other words, must we execute all goals in a phase when the product-based selection is happening at the goal level anyway?

Say we allow each task to look at the target graph when scheduling products (and isn't that the plan?). Then, for example, junit_test could see that it has no targets to act on, it would not schedule any products, and then none of the jvm machinery would be invoked, which seems like a good thing.

Or, am I missing some obvious reason why this is wrong?

Iterating over a collection that you're mutating is bug-prone. Maybe better to let update_phases return the new phases?

This will update for all products ever seen, not just for new products, as the signature of update_phases implies.

There's no harm in the end because update_phases won't add the same phase twice, but it's nice to be precise.

I'm a little confused by this being the product type for codegen, given that codegen generally just injects synthetic targets into the graph.  So its type is kind of "targets by source", though I don't know if that's a meaningful concept.  Could you elaborate in a comment why this is the type, given that codegen is a bit tricky already?

I really like that we're starting to separate out the preparation phases of goals into meaningful pieces.

I might be in the minority here, but I'm not a big fan of nested classes that aren't trivial (like exception aliases).  I feel like it makes the methods difficult to distinguish from the outer class's methods, and doesn't particularly buy us much versus just defining them at the top level in the module.

Very minor style nit though.

This return can't be hit

.format please

We rely on modifying a mutable argument. That's not awful but it's a bit unintuitive so document it well!

Also, the round_manager knows the context, so there's no need to pass both around everywhere. 

But even better, these classmethods could be instance methods and get the context and the round manager from the instance. 

In fact, could they be methods *on* roundmanager?

One way or another, chains of static methods that call each other with non-trivial argument lists are a code smell, implying that they probably should be instance methods.

This return value isn't used. 

Probably good to set outer_lock_holder to None here, so we don't release lock again in the finally block.

check 1st, construct the round_manager after

Now that the actions are all in a specified order, 1 list should be used to collect them.

The result should be:
construct_action('bootstrap-jvm-tools')
prepare_action('bootstrap-jvm-tools')
construct_action('thrift')
prepare_action('thrift')
...

then the execute actions in expected order.

I only see 1 big issue - JvmBinaryTask _should_ be blowing up.  Fixup that and triple check the prepare request vs execute uses - I think I found all the misses remaining, but... and this should be ready to go.

I missed this before, I think context.acquire_lock() and the other 2 lines below at the same indent level all need a +1 - they have only a +1 indent now.

And this is safe because all product.require* are moved out of constructors now - that's great!

'missing_intransitive_deps' is asked for below as well.

Its slighty odd to have prepare ordered before init, generally constructor then instance methods

self.product_type() was used on the LHS and that defaulted to self.__class__.__name__ - so a non-empty string unique to the Task subclass.  I think you can just inline self.__class__.__name__ for suffix_type and then all tasks get an unambiguous single invalidator dir.  Right now the '' is degenerate and 2 tasks could also become degenerate if they happen to produce the same product_type.

Kill - this task doesn't actually populate a 'benchmark-run' product fwict.

This has product_types of ['jars', 'javadoc_jars', 'source_jars']

javadoc_jars too

It would be good to keep the same grouping as in the constructor - the comment goes with the class_by_*

This will need the utf-8 header line of a unit test should fail that checks for this.

I think you need a 1-1 mapping here from RoundManager call to products call.
schedule -> products.require
require_data -> products.require_data

That said - its probably a good time to align the names and s/schedule/require/

Also, you need to reflect the underlying signatures too; for example: JvmBinaryTask passes the optional predicate: https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/tasks/jvm_binary_task.py#L36 

_get_phases_by_product - this looks like its private

pydoc - the ordering is key here and it would be good to document what it is.

This is missing a call to super and JarTask has some prep to do too.  This problem is semi-widespread fwict.

This is looking plausible to me.  I'll `rbt patch` your next diff and work it hard and I think it might just be gold.

You don't actually provide this or have people hack depending on it yet, so I'd kill

I was just looking over the spreadsheet  https://docs.google.com/spreadsheets/d/1R7-SqcKrQt5407pJL5KKgV_Ng2ZWB12ocJWuoCE6Wy4/edit#gid=0. 

I just wanted you to be aware that we are refactoring pants goal idea and at this point have added a consumer of 'ivy_jar_products' in ide_gen.py 

https://github.com/pantsbuild/pants/pull/283

Kill these prints

I think the return code is all you need.  It's which's job to check the exists & is executable part.

This is still a problem though:
$ PANTS_DEV=1 ./pants goal clean-all killserver server
*** Running pants in dev mode from /home/jsirois/dev/3rdparty/pantsbuild-pants/src/python/pants/bin/pants_exe.py ***
(using pantsrc expansion: pants goal clean-all killserver server --server-open)
Launching server with pid 30208 at http://localhost:43116
Killed server with pid 30208 at http://localhost:43116
Created new window in existing browser session.

It would be good to fix or circle back real fast and fix + add unit for unconstrained CLI phases maintaining CLI order.