Thanks for the review Adam. Will get back after I'm done with applying
the suggestions.
On 21/02/13 06:25, Adam Murdoch wrote:
On 20/02/2013, at 9:09 AM, Marcin Erdmann wrote:
You can checkout my efforts on the 'must run after' ordering over
here: https://github.com/erdi/gradle/compare/always-runs-after.
Excellent stuff. Thanks for this. I've added a few comments to the
commits.
Following is the list of what I did and some questions I have:
- Added void mustRunAfter(Task... tasks) to Task and implemented it
in AbstractTask. Currently it only works with task in contrast to
dependsOn which accepts all sorts of things (strings, buildables,
etc) as this was enough to use it with build dashboard plugin. The
question here is do we want to support more objects via mustRunAfte
out of the boxr or should we only add support if a need for it is
driven by new feature requirements?
I think I'd prefer to allow must-run-after dependencies to be lazy,
like task dependencies. We can just use another TaskDependency
instance to collect up the must-run-after tasks.
- DefaultTaskExecutionPlan now uses TaskDependencyGraph (not sure
about the name as it seems a bit too generic) to determine the
execution plan. Interface of the graph class was driven by how it's
used in DefaultTaskExecutionPlan. As suggested by Adam in the first
phase I'm building the graph with both dependency and 'must run
after' edges and in the second phase I'm toposorting it.
- I don't know how to provide a better exception message when there
are no edges without incoming edges
(https://github.com/erdi/gradle/blob/always-runs-after/subprojects/core/src/main/groovy/org/gradle/execution/taskgraph/DefaultTaskExecutionPlan.java#L99)
in the task graph when determining execution plan - there clearly is
a cycle then but it's hard to come up with which tasks are in the
cycle. Any ideas?
I think we actually don't need to collect the nodes with no incoming
edges. We can just start traversing at the tasks that were passed as
parameters to addToTaskGraph(). This way, the cycle will be detected
later on in the algorithm with some context (ie 2 of the tasks in the
cycle).
We can then improve this. CachingDirectedGraphWalker actually has
support for collecting up the nodes of a strongly connected component
(ie the cycle). So, one option would be, when we hit a cycle, traverse
the graph again using this and report which nodes are in the
component. We could possibly just use CachingDirectedGraphWalker to do
the toposort (this was the plan, just never got around to it).
- I decided to leave addToTaskGraph() contract as is because it is
core functionality and any changes to it might introduce regressions
in places I couldn't be aware of with my level of knowledge about the
codebase.
- I didn't directly test getters and setters on
TaskDependencyGraphNode. I believe that it's not necessary as it's
trivial code and it's indirectly tested via TaskDependencyGraphTest -
correct me if I'm wrong.
We don't bother testing simple getters and setters.
- Apart from adding tests around mustRunAfter() in
DefaultTaskExecutionPlanTest I have also refactored how the tasks are
specified for tests in that class to make it easier to define both
dependsOn and mustRunAfter.
- buildDashboard tasks must now always run after any Reporting tasks
apart from itself
I will of course add documentation and mark mustRunAfter() as
incubating when you confirm that you are happy with the implementation.
Marcin
---------------------------------------------------------------------
To unsubscribe from this list, please visit:
http://xircles.codehaus.org/manage_email
--
Adam Murdoch
Gradle Co-founder
http://www.gradle.org
VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting
http://www.gradleware.com
