Skip to content

Conversation

wolfs
Copy link
Member

@wolfs wolfs commented Mar 12, 2019

Instead of only being able to query all file changes, we introduce a new interface which allows querying individual input file property changes.

Replaces #8658.

@wolfs wolfs added this to the 5.4 RC1 milestone Mar 12, 2019
@wolfs wolfs self-assigned this Mar 12, 2019
@wolfs wolfs force-pushed the wolfs/xforms/incremental-task-inputs branch from a40e1cd to 2e1dc7e Compare March 12, 2019 10:54
@wolfs wolfs marked this pull request as ready for review March 12, 2019 20:30
@wolfs wolfs changed the base branch from wolfs/xforms/clean-up-incremental-task-execution to master March 12, 2019 20:34
It is already logged by the execution engine.
@wolfs wolfs requested a review from lptr March 13, 2019 10:26
import spock.lang.Unroll

class IncrementalTasksIntegrationTest extends AbstractIntegrationSpec {
abstract class IncrementalTasksIntegrationTest extends AbstractIntegrationSpec {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this to AbstractIncrementalTasksIntegrationTest, or IncrementalTasksIntegrationSpec.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

public <T> T visitInputFileChanges(IncrementalInputsVisitor<T> visitor) {
return isRebuildRequired() ?
visitor.visitRebuild(thisExecution.getInputFileProperties()) : visitor.visitIncrementalChange(collectInputFileChanges());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be so nice if we could detect beforehand if rebuild is necessary, instead of having to carry around all this state to make the decision late...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am now eagerly detecting the rebuild and other changes, so we don't need to carry the state around.

* {@literal @}TaskAction
* void execute(IncrementalInputs inputs) {
* if (!inputs.incremental)
* project.delete(outputDir.listFiles())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the task declares itself to be incremental, we should do this automatically whenever rebuild is required. IncrementalInputs gives us an opportunity to introduce this behavior. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, what's with the lack of braces?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the braces.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without overlapping outputs that would be easy - visit the output properties and wipe out the things. Shall we disallow overlapping outputs for "new" incremental tasks? Or only wipe out the outputs if no overlapping outputs have been detected?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should think about breaking the build on overlapping outputs. What do we still need to be able to do that?

Until then I guess leaving the outputs in place (and disabling incrementality) when overlaps are present is the way to go. I think it's okay to disable incrementality for both old and new incremental tasks. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR, I'm fine implementing this behavior in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I resurrected https://github.com/gradle/build-cache/issues/711 to implement this behaviour.

public Iterable<InputFileDetails> getChanges(Object property) {
CollectingChangeVisitor visitor = new CollectingChangeVisitor();
CurrentFileCollectionFingerprint currentFileCollectionFingerprint = currentInputs.get(determinePropertyName(property, propertyNameByValue));
currentFileCollectionFingerprint.visitChangesSince(FileCollectionFingerprint.EMPTY, "Input", true, visitor);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the best way to get the files?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now directly get the changes from the fingerprints in the file collection.

*
* @param property The instance of the property to query.
*/
Iterable<InputFileDetails> getFileChanges(Object property);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider using Stream<> here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could. When using stream, we could add some logic later to only produce the changes which are consumed if we wanted to.

What other benefits do we get from returning Stream here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do lazy like that with Iterable<>, too. I'm not sure if Stream<> has any additional benefits. Maybe it's harder to use from Kotlin? Or simpler?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the benefits of Stream is that you can only iterate it once. That makes it clearer for the user of the API that they should only do it once.

}

private void visitAllFileChanges(CurrentFileCollectionFingerprint currentFileCollectionFingerprint, CollectingChangeVisitor visitor) {
currentFileCollectionFingerprint.visitChangesSince(FileCollectionFingerprint.EMPTY, "Input", true, visitor);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the best way to get the files?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the fingerprint directly now.


@Override
public ExecutionOutcome execute(IncrementalChangesContext context) {
public ExecutionOutcome execute(Optional<ExecutionStateChanges> changes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could pass IncrementalChangesInternal here, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also have two methods instead of passing an Optional.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am now passing InputChangesInternal.

Regarding the two methods, I don't think this will make it easier for the one implementing the unit of work. How did you imagine that to work?

wolfs added 3 commits March 13, 2019 17:25
That allows us to determine whether or not a rebuild is required more
easily.
@wolfs
Copy link
Member Author

wolfs commented Mar 14, 2019

@lptr PTAL!

wolfs added 2 commits March 14, 2019 11:35
This replaces `InputFileDetails` from the old incremental task API.

@Override
public void contextualise(TaskExecutionContext context) {
public void contextualise(InputChangesInternal inputChanges) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a better name for this method now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setInputChanges maybe? The other method would be called clearInputChanges I guess.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also renamed ContextAwareTaskAction to InputChangesAwareTaskAction.

Optional<ExecutionStateChanges> getExecutionStateChanges();

void setExecutionStateChanges(ExecutionStateChanges executionStateChanges);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So good to see more methods go from TaskExecutionContext! I'm going to get rid of outputRemovedBeforeExecution soon, too.

? incremental
? ExecutionOutcome.EXECUTED_INCREMENTALLY
: ExecutionOutcome.EXECUTED_NON_INCREMENTALLY
: ExecutionOutcome.UP_TO_DATE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we say that this logic should be moved to the execution engine?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. So UnitOfWork.execute will then return WorkResult?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

@Override
public void visitIncrementalFileInputs(InputFilePropertyVisitor visitor) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why "incremental" in the name here? We are just visiting the input properties here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to only visit the incremental inputs in a follow up. What you say makes sense as well: provide a flag to say that the input is incremental or not, so we only need one visit method for the inputs.

@Override
public void visitIncrementalFileInputs(InputFilePropertyVisitor visitor) {
for (InputFilePropertySpec inputFileProperty : context.getTaskProperties().getInputFileProperties()) {
Object value = inputFileProperty.getValue().call();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we ever use the returned Callable other than to immediately call call() on it? Can we return an Optional<Object> (or a @Nullable Object) instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any preference for @Nullable or Optional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it @Nullable for now.

@wolfs
Copy link
Member Author

wolfs commented Mar 14, 2019

@lptr PTAL!

@wolfs wolfs force-pushed the wolfs/xforms/incremental-task-inputs branch from 945848f to ae9ee8e Compare March 14, 2019 16:59
case DID_WORK:
return incremental ? ExecutionOutcome.EXECUTED_INCREMENTALLY : ExecutionOutcome.EXECUTED_NON_INCREMENTALLY;
default:
throw new IllegalArgumentException("Unknown result: " + result);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think throwing an empty AssertionError here is good.

public interface ContextAwareTaskAction extends ImplementationAwareTaskAction, Describable {
void contextualise(InputChangesInternal inputChanges);
void releaseContext();
public interface InputChangesAwareTaskAction extends ImplementationAwareTaskAction, Describable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

@lptr
Copy link
Member

lptr commented Mar 15, 2019

LGTM.

@wolfs wolfs merged commit d51d89b into master Mar 15, 2019
@wolfs wolfs deleted the wolfs/xforms/incremental-task-inputs branch March 15, 2019 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants