Locking task container after configuration phase

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Locking task container after configuration phase

erdi
While working on the new configuration model Luke and I have noticed that we are not locking task container after the configuration phase so the following does not throw an error (even though it essentially has no useful effect):

task foo {
  doFirst {
    tasks.create("bar")
  }
}

As implemented at the moment tasks references are added to the new configuration model based on the task container contents at the end of the configuration phase therefore it probably makes sense to prevent any more modifications to the container after that point in time.

Any thoughts?

Cheers,
Marcin
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Locking task container after configuration phase

Szczepan Faber
+1 to prevent adding tasks after DAG is ready

On Tue, Sep 2, 2014 at 6:29 PM, Marcin Erdmann
<[hidden email]> wrote:

> While working on the new configuration model Luke and I have noticed that we
> are not locking task container after the configuration phase so the
> following does not throw an error (even though it essentially has no useful
> effect):
>
> task foo {
>   doFirst {
>     tasks.create("bar")
>   }
> }
>
> As implemented at the moment tasks references are added to the new
> configuration model based on the task container contents at the end of the
> configuration phase therefore it probably makes sense to prevent any more
> modifications to the container after that point in time.
>
> Any thoughts?
>
> Cheers,
> Marcin



--
Szczepan Faber
Core dev@gradle; Founder@mockito

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Locking task container after configuration phase

Adam Murdoch
In reply to this post by erdi

On 3 Sep 2014, at 2:29 am, Marcin Erdmann <[hidden email]> wrote:

While working on the new configuration model Luke and I have noticed that we are not locking task container after the configuration phase so the following does not throw an error (even though it essentially has no useful effect):

task foo {
  doFirst {
    tasks.create("bar")
  }
}

As implemented at the moment tasks references are added to the new configuration model based on the task container contents at the end of the configuration phase therefore it probably makes sense to prevent any more modifications to the container after that point in time.

It makes sense to prevent (or warn about) pointless creation of tasks. We do have to be a little careful about when we lock the task container for a given project, for backwards compatibility reasons.

Currently, any build script can define a task in any other project, so we can’t lock until after every project build script has been executed and the afterEvaluate() events fired. We could certainly deprecate this behaviour, but we still need to support it.

In the future, we want to be able to interleave tasks and rules (for example, configure my publications after I’ve generated my source to see what needs to be published, or build my plugin and then apply it to my project).

So, we could potentially lock the task container after the DAG for the project has been finalised, and we’re not going to consider any further tasks in that project for execution.


--
Adam Murdoch
Gradle Co-founder
http://www.gradle.org
CTO Gradleware Inc. - Gradle Training, Support, Consulting
http://www.gradleware.com



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Locking task container after configuration phase

Luke Daley-2


On 3 September 2014 at 5:52:44 am, Adam Murdoch ([hidden email]) wrote:


On 3 Sep 2014, at 2:29 am, Marcin Erdmann <[hidden email]> wrote:

While working on the new configuration model Luke and I have noticed that we are not locking task container after the configuration phase so the following does not throw an error (even though it essentially has no useful effect):

task foo {
  doFirst {
    tasks.create("bar")
  }
}

As implemented at the moment tasks references are added to the new configuration model based on the task container contents at the end of the configuration phase therefore it probably makes sense to prevent any more modifications to the container after that point in time.

It makes sense to prevent (or warn about) pointless creation of tasks. We do have to be a little careful about when we lock the task container for a given project, for backwards compatibility reasons.

Currently, any build script can define a task in any other project, so we can’t lock until after every project build script has been executed and the afterEvaluate() events fired. We could certainly deprecate this behaviour, but we still need to support it.

The story that triggered this is about defining the lifecycle WRT the “configuration phase” and the new model rule stuff.  At the moment we interleave the two, which we want to now change.

What I want to do is register the tasks into the model space _just_ before the task graph is built, and do this via iteration instead of .all(). This is cleaner, and avoids some awkwardness with task removal/replacement during the configuration phase. If we do this, we should ensure that no tasks are yet to be created. So, it’s at this point I want to “lock” the task container. 

This leaves only two “holes” for tasks to still be added: during task dependency traversal (a Buildable input could create a task in its getTaskDependency() impl) and during execution time proper. I can’t think of any valid reasons to do either of those. I suspect that creating tasks during the creation of the task graph is going to lead to CMEs at worst, and is undefined at best. Probably the same for creating tasks at execution time. 

Three options as I see them:

1. Once we start building the task graph, have all task containers warn that adding tasks at this time is deprecated but still try and add it

2. Once we start building the task graph, have all task containers warn that adding tasks at this time is not support it and discard the task

3. Once we start building the task graph, have all task containers error if someone tries to add a task

Given how edge case this is, I’m for jumping straight to 3 as a breaking change.

In the future, we want to be able to interleave tasks and rules (for example, configure my publications after I’ve generated my source to see what needs to be published, or build my plugin and then apply it to my project).

Yes, but I think this will only be possible with the extra insight we get from the model system. Therefore, not interleaving the “configuration phase” make sense I think. That is, what you describe here will only be possible when the relevant bits are managed model.

So, we could potentially lock the task container after the DAG for the project has been finalised, and we’re not going to consider any further tasks in that project for execution.

Right, that’s less aggressive than what I want to do. This doesn’t quite help us for the model stuff as we need to actualise the tasks produced my model rules before the task graph is built. The only practical difference would be that locking after the task graph is built would still let tasks be created as we mine task inputs to build the graph. I’d really like to close that hole as well.






--
Adam Murdoch
Gradle Co-founder
http://www.gradle.org
CTO Gradleware Inc. - Gradle Training, Support, Consulting
http://www.gradleware.com







Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Locking task container after configuration phase

Adam Murdoch

On 3 Sep 2014, at 9:45 am, Luke Daley <[hidden email]> wrote:



On 3 September 2014 at 5:52:44 am, Adam Murdoch ([hidden email]) wrote:


On 3 Sep 2014, at 2:29 am, Marcin Erdmann <[hidden email]> wrote:

While working on the new configuration model Luke and I have noticed that we are not locking task container after the configuration phase so the following does not throw an error (even though it essentially has no useful effect):

task foo {
  doFirst {
    tasks.create("bar")
  }
}

As implemented at the moment tasks references are added to the new configuration model based on the task container contents at the end of the configuration phase therefore it probably makes sense to prevent any more modifications to the container after that point in time.

It makes sense to prevent (or warn about) pointless creation of tasks. We do have to be a little careful about when we lock the task container for a given project, for backwards compatibility reasons.

Currently, any build script can define a task in any other project, so we can’t lock until after every project build script has been executed and the afterEvaluate() events fired. We could certainly deprecate this behaviour, but we still need to support it.

The story that triggered this is about defining the lifecycle WRT the “configuration phase” and the new model rule stuff.  At the moment we interleave the two, which we want to now change.

What I want to do is register the tasks into the model space _just_ before the task graph is built, and do this via iteration instead of .all().


You should make sure you don’t trigger the placeholder actions when doing this. These could be translated across to creation rules without being triggered.

This is cleaner, and avoids some awkwardness with task removal/replacement during the configuration phase. If we do this, we should ensure that no tasks are yet to be created. So, it’s at this point I want to “lock” the task container.


The problem is that it’s too early. There are some examples below.

From a usability point of view, we need to avoid 2 things:

1. Tasks that are created that can never be executed.
2. Tasks that are created after some predicate that they match has been closed in the rules.

They’re really 2 separate problems, and should be solved in separate ways.

The first one would be solved by closing the task container for a project after the dag has been finalised for that project (however that dag happens to have been built).

The second one would be solved by listening for changes to the task container.

 

This leaves only two “holes” for tasks to still be added: during task dependency traversal (a Buildable input could create a task in its getTaskDependency() imll)


In configure-on-demand mode, the target projects are configured during task dependency traversal. The logic for the target project could, but is not supposed to, create tasks in the current project.

An example of task creation during task dependency traversal would be tasks created by the old rules added via NamedDomainObjectCollection.addRule():

clean.dependsOn(“cleanJavaCompile”)

and during execution time proper. I can’t think of any valid reasons to do either of those. I suspect that creating tasks during the creation of the task graph is going to lead to CMEs at worst, and is undefined at best. Probably the same for creating tasks at execution time. 

Three options as I see them:

1. Once we start building the task graph, have all task containers warn that adding tasks at this time is deprecated but still try and add it

2. Once we start building the task graph, have all task containers warn that adding tasks at this time is not support it and discard the task

3. Once we start building the task graph, have all task containers error if someone tries to add a task

Given how edge case this is, I’m for jumping straight to 3 as a breaking change.


I think we need to lock on a per-project basis, otherwise configure-on-demand is going to be broken. 


--
Adam Murdoch
Gradle Co-founder
http://www.gradle.org
CTO Gradleware Inc. - Gradle Training, Support, Consulting
http://www.gradleware.com



Loading...