New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce support for running phased actions #4533

Merged
merged 10 commits into from Apr 24, 2018

Conversation

Projects
None yet
3 participants
@lsmaira
Contributor

lsmaira commented Feb 27, 2018

This commit introduces the ability of running multiple build actions in
different phases of the build. These actions are passed by the client
through the tooling api.

With this commit, a single action can be added to each one of the
supporting phases (after projects are loaded, after projects are
evaluated and after tasks are run).

This feature allows improvements like running actions that call a model
builder modifying the graph tasks, and then it is possible to first
fetch a model and then execute tasks, in this order. e.g. Android Studio
sync + source generation.

Signed-off-by: Lucas Smaira lsmaira@google.com

Context

This change will be useful for Android Studio to sync and generate sources in a single Gradle invocation.

Problem and solution discussed with Gradlers in private meetings instead of public forums. Design doc included.

Contributor Checklist

  • Review Contribution Guidelines
  • Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes -> updated java docs
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:check

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation
  • Recognize contributor in release notes
@lsmaira

This comment has been minimized.

Contributor

lsmaira commented Feb 27, 2018

I made a few changes concerning the reviewed doc:

  • Instead of having an afterSettingsEvaluatedAction, I replaced that with afterProjectsLoaded action, since we kind of want projects information (to ask user to select what projects to sync in the future)
  • Added a forTasks() method to PhasedActionExecuter to allow predefined tasks to be configured by the client
@@ -30,7 +30,7 @@ class ToolingApiClasspathIntegrationTest extends AbstractIntegrationSpec {
then:
resolve.classpath.size() == 2
resolve.classpath.any {it.name ==~ /slf4j-api-.*\.jar/}
resolve.classpath.find { it.name ==~ /gradle-tooling-api.*\.jar/ }.size() < 1.7 * 1024 * 1024
resolve.classpath.find { it.name ==~ /gradle-tooling-api.*\.jar/ }.size() < 1.8 * 1024 * 1024

This comment has been minimized.

@lsmaira

lsmaira Feb 27, 2018

Contributor

I'm not sure if this is the right "fix" for this test...
My understanding is that this test checks the size of the jar and it is normal to have it increased from time to time since code is added to it. Is that right?

This comment has been minimized.

@oehme

oehme Feb 28, 2018

Member

Yes that's right - This is just to make sure it is a minified JAR and we don't accidentally package unnecessary stuff in there. The failure serves as a reminder to ask "Did I really add that much code?"

@oehme oehme self-assigned this Feb 28, 2018

@oehme oehme added this to the 4.7 RC1 milestone Feb 28, 2018

@lsmaira

This comment has been minimized.

Contributor

lsmaira commented Mar 7, 2018

Hey Stefan,
I didn't really understand these failing tests. They seem unrelated. Is there a fair issue in my commit or was it an infrastructure problem?

@oehme

This comment has been minimized.

Member

oehme commented Mar 8, 2018

The compatibility check failed because new APIs weren't marked with @since and @Incubating. Can you have a look?

@lsmaira

This comment has been minimized.

Contributor

lsmaira commented Mar 8, 2018

Sure, done! Sorry, I hadn't found the logs of the tests. Now I see where they are.

@oehme

This comment has been minimized.

Member

oehme commented Mar 8, 2018

@lsmaira

This comment has been minimized.

Contributor

lsmaira commented Mar 14, 2018

@blindpirate Is there anything I should do before the code review? Or does the flag "waiting-for:contributor" means anything else? Tests pass and the feature is complete

@blindpirate

This comment has been minimized.

Member

blindpirate commented Mar 14, 2018

@lsmaira Sorry I didn't see the recent commit you pushed and thought @oehme was waiting for your feedback. Changed the tag now.

@oehme

I've taken a first pass over the API, documentation and test coverage (which is great!). There are a few things I think we should change before I do a detailed review of the implementation classes.

addBuildListener(phasedAction, buildController);
// We don't know if the model builders invoked in the after configuration action will add tasks to the graph or not

This comment has been minimized.

@oehme

oehme Mar 15, 2018

Member

This should be improved in buildController by providing an API similar to

buildController.build {
  loadSettings()
  configureBuild()
  runTasks()
}

That method would automatically fire the right buildStarted/buildFinished hooks and you would get a failure if you try to do things in the wrong order. Using such an API you could run the build only up to the point that you want and react to what happened in between.

This can be a follow-up improvement, but until this is done the projectsLoaded hook is kinda unhelpful, because it would still configure everything even though the user only wanted information based on the settings file.

Will you have time to look into this after we merge this change?

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Thank you for the suggestion :) I was thinking about a solution for that.
I'll try to implement it in another commit in this same pull request soon, so it doesn't block review for now and hopefully we can merge together.

This comment has been minimized.

@lsmaira

lsmaira Mar 27, 2018

Contributor

@oehme, I looked into this and implemented a solution based on this suggestion. As it is kind of orthogonal to this change, I thought it was better to submit as a separate PR (it touches progress events to make Loading projects and Configure Projects two separate build operations).

The commit is here and once we merge this PR, I'll rebase on top of master and send another PR.

Is it okay?

This comment has been minimized.

@oehme

oehme Apr 5, 2018

Member

Yes, making that another PR was my intention as well.

import org.gradle.tooling.internal.provider.serialization.SerializedPayload
import spock.lang.Specification
class ClientProvidedPhasedActionRunnerTest extends Specification {

This comment has been minimized.

@oehme

oehme Mar 15, 2018

Member

This comment applies to all unit tests: Can you please reduce the number of Mocks to only those interactions you are trying to verify? Everything else should be Stubs (or just the real implementation if it is a value object).

The reason behind this is that mocks lock you down to a very specific implementation, so that refactorings can become extremely painful.

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Got it, makes sense. I replaced mocks by stubs all around the code when possible, thank you!

*
* @since 4.7
*/
<T> Builder addAfterLoadingAction(BuildAction<T> buildAction, ResultHandler<? super T> handler) throws MultipleBuildActionsException;

This comment has been minimized.

@oehme

oehme Mar 15, 2018

Member

⭕️ projectsLoaded()

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Done

*
* @since 4.7
*/
<T> Builder addAfterConfigurationAction(BuildAction<T> buildAction, ResultHandler<? super T> handler) throws MultipleBuildActionsException;

This comment has been minimized.

@oehme

oehme Mar 15, 2018

Member

⭕️ projectsEvaluated()

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Done

*
* @since 4.7
*/
<T> Builder addAfterBuildAction(BuildAction<T> buildAction, ResultHandler<? super T> handler) throws MultipleBuildActionsException;

This comment has been minimized.

@oehme

oehme Mar 15, 2018

Member

⭕️ buildFinished()

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Done

/**
* Adds the given action and its result handler to be executed after tasks are run.
*
* <p>The action should not invoke task graph modifiers, since tasks have already been run.

This comment has been minimized.

@oehme

oehme Mar 15, 2018

Member

⭕️ "task graph modifier" is not a term we use. I think I would leave this out. It's also kind of obvious from the fact that this is called "buildFinished". I don't think anyone would expect a modification of the StartParameter to have any effect at that point.

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Done

/**
* Adds the given action and its result handler to be executed after projects are configured and before tasks are run.
*
* <p>The action has already access to plugins' model builders. These model builders can modify the task graph, since tasks have not yet been run.

This comment has been minimized.

@oehme

oehme Mar 15, 2018

Member

Let's not encourage people to do this. It's the only way to solve your use case right now, but I think we want a stronger model than mutating the StartParameter in the future.

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Makes sense, done

* @since 4.7
*/
@Incubating
public class MultipleBuildActionsException extends RuntimeException {

This comment has been minimized.

@oehme

oehme Mar 15, 2018

Member

This is overly specific in my opinion. This is a programming error, not something that should be specially reacted to. A simple IllegalArgumentException should do.

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Indeed, too specific. Done

*
* @since 4.7
*/
public interface AfterLoadingResult<T> extends PhasedActionResult<T> {

This comment has been minimized.

@oehme

oehme Mar 15, 2018

Member

⭕️ Why 3 different types instead of having a getPhase()?

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

No special reason, it seemed clearer to me. Changed to a getPhase() with an enum

* @since 4.7
*/
@Nullable
Throwable getFailure();

This comment has been minimized.

@oehme

oehme Mar 15, 2018

Member

Individual actions should not have failures, that just complicates the user code unnecessarily. If an action encounters a recoverable exception it should handle it itself. If an action throws an exception, Gradle should fail the build immediately.

As a result, only once failure callback is necessary and the individual result listeners don't need a failure callback.

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Cool, it makes life easier for developers indeed. Removed failures from listeners (they are sent to the build listener or thrown if synchronous)

@lsmaira

Thank you for the review :)

*
* @since 4.7
*/
<T> Builder addAfterLoadingAction(BuildAction<T> buildAction, ResultHandler<? super T> handler) throws MultipleBuildActionsException;

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Done

*
* @since 4.7
*/
<T> Builder addAfterConfigurationAction(BuildAction<T> buildAction, ResultHandler<? super T> handler) throws MultipleBuildActionsException;

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Done

*
* @since 4.7
*/
<T> Builder addAfterBuildAction(BuildAction<T> buildAction, ResultHandler<? super T> handler) throws MultipleBuildActionsException;

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Done

interface Builder {
/**
* Adds the given action and its result handler to be executed after projects are loaded.

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Done

/**
* Adds the given action and its result handler to be executed after projects are loaded.
*
* <p>The action or model builders invoked by it are run when plugins have not yet be applied, and so it should try to get only gradle default models.

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

True, thank you! Done

* @since 4.7
*/
@Incubating
public class MultipleBuildActionsException extends RuntimeException {

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Indeed, too specific. Done

*
* @since 4.7
*/
public interface AfterLoadingResult<T> extends PhasedActionResult<T> {

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

No special reason, it seemed clearer to me. Changed to a getPhase() with an enum

* @since 4.7
*/
@Nullable
Throwable getFailure();

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Cool, it makes life easier for developers indeed. Removed failures from listeners (they are sent to the build listener or thrown if synchronous)

import org.gradle.tooling.internal.provider.serialization.SerializedPayload
import spock.lang.Specification
class ClientProvidedPhasedActionRunnerTest extends Specification {

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Got it, makes sense. I replaced mocks by stubs all around the code when possible, thank you!

addBuildListener(phasedAction, buildController);
// We don't know if the model builders invoked in the after configuration action will add tasks to the graph or not

This comment has been minimized.

@lsmaira

lsmaira Mar 22, 2018

Contributor

Thank you for the suggestion :) I was thinking about a solution for that.
I'll try to implement it in another commit in this same pull request soon, so it doesn't block review for now and hopefully we can merge together.

@blindpirate blindpirate modified the milestones: 4.7 RC1, 4.8R, 4.8 RC1 Mar 27, 2018

@oehme

This comment has been minimized.

Member

oehme commented Mar 27, 2018

@lsmaira I'm on vacation this week, but will give this another review early next week so we can get it merged. The target release will be Gradle 4.8.

@lsmaira

This comment has been minimized.

Contributor

lsmaira commented Mar 28, 2018

Alright, I'll send a commit changing mentions of 4.7 to 4.8.

@lsmaira

This comment has been minimized.

Contributor

lsmaira commented Mar 28, 2018

Just updated target version to 4.8 and rebased everything to avoid later conflicts (like moving to StartParameterInternal) and take advantage that master is already in 4.8

@oehme

Awesome stuff, this is almost ready to go, I just had a few comments around exception handling.

return run(action, cancellationToken, listenerConfig, new PhasedActionEventConsumer(failsafeBuildSessionResultListener, payloadSerializer, listenerConfig.buildEventConsumer),
providerParameters, params);
} finally {
failsafeBuildSessionResultListener.rethrowErrors();

This comment has been minimized.

@oehme

oehme Apr 5, 2018

Member

This seems unnecessary. Why capture the exception and then rethrow it in a finally block? We can just let it blow up right away.

This comment has been minimized.

@lsmaira

lsmaira Apr 9, 2018

Contributor

I first implemented this thinking in continuing the build even when we have a failure in the listener implementation provided by the user so listeners of future actions would still be called if future actions are correct. But indeed while it makes sense for progress listeners, I think it doesn't make much sense here if we want to fail the build in the first global failure. I removed the FailsafeListener wrapper and added a description in the exception thrown in PhasedActionEventConsumer so the user knows which listener failed.

This comment has been minimized.

@lsmaira

lsmaira Apr 9, 2018

Contributor

However, just throwing the exception right away if the listener fails, make it be captured and treated by the ExceptionAnalyser in DefaultGradleLauncher, which will try to make it a LocationAwareException (but will find no reference in scripts, of course).
Let me know if this is what you expected?

addBuildListener(phasedAction, buildController);
// We don't know if the model builders invoked in the after configuration action will add tasks to the graph or not

This comment has been minimized.

@oehme

oehme Apr 5, 2018

Member

Yes, making that another PR was my intention as well.

addBuildListener(phasedAction, buildController);
// We don't know if the model builders invoked in the after configuration action will add tasks to the graph or not
// so we have to execute build until Build stage before finishing it.

This comment has been minimized.

@oehme

oehme Apr 5, 2018

Member

Can you add some test coverage to see what this means when I don't actually want to run any tasks? I suspect it will run the default tasks, which would be kinda surprising to the user.

This comment has been minimized.

@lsmaira

lsmaira Apr 9, 2018

Contributor

Great point! Indeed default tasks are run even when no tasks are defined by the user. I think the way to fix this is, as you suggested in a previous comment, introduce the new API in buildController, and then, before build stage executeTasks we can check if any tasks were defined.

I added "PhasedBuildActionCrossVersionSpec#default tasks are not run if no tasks are specified" and set it as "ignore" for now.

Is it okay to fix it in the other PR?

@Override
public void projectsLoaded(Gradle gradle) {
// If build controller has a result at this point, it is a failure.
if (!buildController.hasResult()) {

This comment has been minimized.

@oehme

oehme Apr 5, 2018

Member

This shouldn't be necessary anymore, since we throw failures right away.

This comment has been minimized.

@lsmaira

lsmaira Apr 9, 2018

Contributor

I think this is still necessary since we throw listener failures right away, but not action failures (we wrap them in internal protocol exceptions, serialize them, send back to the tooling API and then deserialize and convert them later - see runAction() in this class).

This is the same logic used by ClientProvidedBuildActionRunner so exceptions regarding the Tooling API are not treated by core exception transformers, etc, but sent back to the client directly and given more meaning (in my understanding).

We could, of course, make runAction() throw the exception directly instead of serialize it, set as result and let ProviderConnection deserialize it. But I think that this would make exceptions less understandable (they would be wrapped in all core exceptions - LocationAwareException, BuildException - instead of sending it back directly to the client throwing the exceptions defined in InternalPhasedActionConnection).

Let me know if that makes sense or I'm missing something, please, and I can change it if you still think it's better, of course.

*
* @return The result. {@code null} if a failure occurred.
*
* @since 4.8

This comment has been minimized.

@oehme

oehme Apr 5, 2018

Member

These methods don't need a since tag, the information is already on the class.

This comment has been minimized.

@lsmaira

lsmaira Apr 9, 2018

Contributor

Cool, thanks. Removed from here and all the other docs where I had done the same.

/**
* Gets the action result if it completed successfully.
*
* @return The result. {@code null} if a failure occurred.

This comment has been minimized.

@oehme

oehme Apr 5, 2018

Member

This shouldn't happen. If the action failed the build should fail and no result should be returned to the phased handler

This comment has been minimized.

@lsmaira

lsmaira Apr 9, 2018

Contributor

That's right, sorry. Done.

@lsmaira

Thank you very much for reviewing it :)

return run(action, cancellationToken, listenerConfig, new PhasedActionEventConsumer(failsafeBuildSessionResultListener, payloadSerializer, listenerConfig.buildEventConsumer),
providerParameters, params);
} finally {
failsafeBuildSessionResultListener.rethrowErrors();

This comment has been minimized.

@lsmaira

lsmaira Apr 9, 2018

Contributor

I first implemented this thinking in continuing the build even when we have a failure in the listener implementation provided by the user so listeners of future actions would still be called if future actions are correct. But indeed while it makes sense for progress listeners, I think it doesn't make much sense here if we want to fail the build in the first global failure. I removed the FailsafeListener wrapper and added a description in the exception thrown in PhasedActionEventConsumer so the user knows which listener failed.

return run(action, cancellationToken, listenerConfig, new PhasedActionEventConsumer(failsafeBuildSessionResultListener, payloadSerializer, listenerConfig.buildEventConsumer),
providerParameters, params);
} finally {
failsafeBuildSessionResultListener.rethrowErrors();

This comment has been minimized.

@lsmaira

lsmaira Apr 9, 2018

Contributor

However, just throwing the exception right away if the listener fails, make it be captured and treated by the ExceptionAnalyser in DefaultGradleLauncher, which will try to make it a LocationAwareException (but will find no reference in scripts, of course).
Let me know if this is what you expected?

@Override
public void projectsLoaded(Gradle gradle) {
// If build controller has a result at this point, it is a failure.
if (!buildController.hasResult()) {

This comment has been minimized.

@lsmaira

lsmaira Apr 9, 2018

Contributor

I think this is still necessary since we throw listener failures right away, but not action failures (we wrap them in internal protocol exceptions, serialize them, send back to the tooling API and then deserialize and convert them later - see runAction() in this class).

This is the same logic used by ClientProvidedBuildActionRunner so exceptions regarding the Tooling API are not treated by core exception transformers, etc, but sent back to the client directly and given more meaning (in my understanding).

We could, of course, make runAction() throw the exception directly instead of serialize it, set as result and let ProviderConnection deserialize it. But I think that this would make exceptions less understandable (they would be wrapped in all core exceptions - LocationAwareException, BuildException - instead of sending it back directly to the client throwing the exceptions defined in InternalPhasedActionConnection).

Let me know if that makes sense or I'm missing something, please, and I can change it if you still think it's better, of course.

/**
* Gets the action result if it completed successfully.
*
* @return The result. {@code null} if a failure occurred.

This comment has been minimized.

@lsmaira

lsmaira Apr 9, 2018

Contributor

That's right, sorry. Done.

*
* @return The result. {@code null} if a failure occurred.
*
* @since 4.8

This comment has been minimized.

@lsmaira

lsmaira Apr 9, 2018

Contributor

Cool, thanks. Removed from here and all the other docs where I had done the same.

addBuildListener(phasedAction, buildController);
// We don't know if the model builders invoked in the after configuration action will add tasks to the graph or not
// so we have to execute build until Build stage before finishing it.

This comment has been minimized.

@lsmaira

lsmaira Apr 9, 2018

Contributor

Great point! Indeed default tasks are run even when no tasks are defined by the user. I think the way to fix this is, as you suggested in a previous comment, introduce the new API in buildController, and then, before build stage executeTasks we can check if any tasks were defined.

I added "PhasedBuildActionCrossVersionSpec#default tasks are not run if no tasks are specified" and set it as "ignore" for now.

Is it okay to fix it in the other PR?

@oehme

This comment has been minimized.

Member

oehme commented Apr 9, 2018

I'm afraid just checking whether some tasks were defined is not enough. Think about a phased action which just wants to look at the projects after the settings were evaluated, but doesn't want to configure them/run tasks at all. How would Gradle distinguish that from an action that does want Gradle to run the default tasks even though it only has a projectsLoaded hook?

It seems like the action should explicitly define up to which phase Gradle should run. That could be execute tasks by default, but could be changed to just configure projects or just load the settings. I'm okay if we do this in a follow-up, but until then we need to document that the current API will always execute the default tasks if none are defined.

Regarding your comments about the individual failures: I think I confused two different things:

  1. the actions run inside the daemon
  2. the listeners receiving the results on the client side

If 1. fails, the build should terminate immediately. So if I provide an action that hooks into the projectsLoaded hook and that action fails, then Gradle should not configure or run tasks. We should have a test for that.

Failures in 2. should indeed be rethrown only after the build itself has finished.

@lsmaira

This comment has been minimized.

Contributor

lsmaira commented Apr 10, 2018

Got it! That makes sense and I had completely missed interrupting the build if an action fails (I had only guaranteed that future actions wouldn't be executed).

I removed the last commit concerning listener exception handling and sent a new commit addressing your comments:

  • I documented in PhasedActionExecuter that default tasks will be run in case none is specified. I also added a test to guarantee that default tasks are not run (it is, of course, ignored for now, and I'll enable it in my next PR, where I'll add, as you suggested, a upToPhase API when running a PhasedAction)

  • Exceptions in client provided listeners are rethrown after the build has finished

  • Now the build will immediately fail if one of the actions fails (e.g. if projectsLoadedAction fails projects will not be configured). I added a test in PhasedBuildActionCrossVersionSpec for that. To give more meaning to the exceptions, I unwrap them in ProviderConnection to be sent back to the ToolingAPI so they match the expected type. I think that a better fix, however, would be to check for failures in the API that will be introduced in BuildController, so we don't wrap and after unwrap the exceptions, instead we just don't execute future phases. I plan to do that in the next PR, if you're okay with that.

@oehme

Great work @lsmaira! I just found one more documentation issue.

try {
return run(action, cancellationToken, listenerConfig, new PhasedActionEventConsumer(failsafePhasedActionResultListener, payloadSerializer, listenerConfig.buildEventConsumer),
providerParameters, params);
} catch (BuildExceptionVersion1 e) {

This comment has been minimized.

@oehme

oehme Apr 17, 2018

Member

⭕️ I think this logic should live in the phased build action runner (i.e. a try-catch around buildController.run())

This comment has been minimized.

@lsmaira

lsmaira Apr 17, 2018

Contributor

Definitely, much better, thank you! Moved there

/**
* Specifies the tasks to execute before executing the BuildFinishedAction and after the ProjectsEvaluatedAction.
*
* The graph task can be changed in model builders invoked. If not configured, null, or an empty array is passed, then no tasks will be executed unless one of the model builders configures it.

This comment has been minimized.

@oehme

oehme Apr 17, 2018

Member

This is not true. Tasks will always be run. I think we should remove this paragraph and instead explain task execution later when we add the "upToPhase" concept. Also let's not call out the possibility of mutating the start parameters in the model builder.

This comment has been minimized.

@lsmaira

lsmaira Apr 17, 2018

Contributor

Alright, removed the paragraph

@lsmaira

Thank you for the review @oehme ! I addressed your comments in new commits :)

/**
* Specifies the tasks to execute before executing the BuildFinishedAction and after the ProjectsEvaluatedAction.
*
* The graph task can be changed in model builders invoked. If not configured, null, or an empty array is passed, then no tasks will be executed unless one of the model builders configures it.

This comment has been minimized.

@lsmaira

lsmaira Apr 17, 2018

Contributor

Alright, removed the paragraph

try {
return run(action, cancellationToken, listenerConfig, new PhasedActionEventConsumer(failsafePhasedActionResultListener, payloadSerializer, listenerConfig.buildEventConsumer),
providerParameters, params);
} catch (BuildExceptionVersion1 e) {

This comment has been minimized.

@lsmaira

lsmaira Apr 17, 2018

Contributor

Definitely, much better, thank you! Moved there

@oehme

oehme approved these changes Apr 17, 2018

@oehme

This comment has been minimized.

Member

oehme commented Apr 17, 2018

Thanks @lsmaira, this looks very good. I'll take it from here.

@oehme

This comment has been minimized.

Member

oehme commented Apr 18, 2018

@lsmaira One more request: Could you rebase your branch on master?

@lsmaira

This comment has been minimized.

Contributor

lsmaira commented Apr 18, 2018

Sure! I'll run tests locally to make sure everything works and send it soon

@lsmaira

This comment has been minimized.

Contributor

lsmaira commented Apr 18, 2018

Sorry for the delay, @oehme, I had some meetings the entire day. I rebased on top of master and made sure that :toolingApi:check, :toolingApiBuilders:check, :launcher:check and :core:check passed.

@lsmaira

This comment has been minimized.

Contributor

lsmaira commented Apr 20, 2018

@oehme is this performance test failure related to the PR? Looking at the test history it looks like it passed by an instability period. I couldn't run the test locally

@oehme

This comment has been minimized.

Member

oehme commented Apr 20, 2018

Yeah I think you got a bad commit form master when you rebaselined, can you try again?

lsmaira and others added some commits Feb 27, 2018

Introduce support for running phased actions
This commit introduces the ability of running multiple build actions in
different phases of the build. These actions are passed by the client
through the tooling api.

With this commit, a single action can be added to each one of the
supporting phases (after projects are loaded, after projects are
evaluated and after tasks are run).

This feature allows improvements like running actions that call a model
builder modifying the graph tasks, and then it is possible to first
fetch a model and then execute tasks, in this order. e.g. Android Studio
sync + source generation.

Signed-off-by: Lucas Smaira <lsmaira@google.com>
Add @Incubating and @SInCE annotations
To MultipleBuildActionsException.

Signed-off-by: Lucas Smaira <lsmaira@google.com>
Remove bad import
In PhasedBuildActionCrossVersionSpec.

Signed-off-by: Lucas Smaira <lsmaira@google.com>
Modify expected tooling api jar size
In DistributionIntegrationSpec

Signed-off-by: Lucas Smaira <lsmaira@google.com>
Code and documentation ajustments to PR
This commit:
- Renames methods in PhasedBuildActionExecuter and corresponding uses
- Improves public java docs making them more precise
- Replaces mocks by stubs in unit tests when possible
- Makes action's handlers in phased actions not receiving failures (they
are send to build results)

Signed-off-by: Lucas Smaira <lsmaira@google.com>
Change target version from 4.7 to 4.8
Changing all uses of 4.7 in documentation and code.

Signed-off-by: Lucas Smaira <lsmaira@google.com>
Change exception handling when phased action fails
Exceptions in BuildActions of a PhasedAction are now immediately thrown
and the build imediatelly fails. This makes sure that if an action fails
the remaining steps of the build will not uselessly be executed.

Exceptions are unwrapped in ProviderConnection so the correct
information is sent back to the TAPI client.

This commit also addresses other review comments in the PR:
- Removes unnecessary @SInCE annotations in methods
- Adds a test (ignored for now) making sure that default tasks are not
run when no tasks are specified by the user

Signed-off-by: Lucas Smaira <lsmaira@google.com>
Fix documentation of PhasedBuildActionExecuter
Signed-off-by: Lucas Smaira <lsmaira@google.com>
Handle BA failures in phased action runner
Move exception handling in case of failures in BuildActions from
ProviderConnection to ClientProvidedPhasedActionRunner.

Signed-off-by: Lucas Smaira <lsmaira@google.com>

@oehme oehme merged commit 0630bef into gradle:master Apr 24, 2018

2 checks passed

DCO All commits have a DCO sign-off from the author
Details
WIP ready for review
Details
@oehme

This comment has been minimized.

Member

oehme commented Apr 24, 2018

Thank you again @lsmaira! Looking forward to the upTo(Stage) PR :)

@lsmaira lsmaira deleted the lsmaira:single-invocation-sync branch Apr 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment