Skip to content

Conversation

ldaley
Copy link
Contributor

@ldaley ldaley commented Feb 2, 2021

This is a strawman for discussing options for improving the user feedback when unsafe/incomplete implementation or configuration of a work unit causes “optimizations” to be disabled.

Tests have not been updated to reflect the proposal.

For the common case of not using --info, before:

Before:

Property 'myPrivateInput' is private and annotated with @Input. This behaviour has been deprecated and is scheduled to be removed in Gradle 7.0. Execution optimizations are disabled due to the failed validation. See https://docs.gradle.org/7.0-20210201140000+0000/userguide/more_about_tasks.html#sec:up_to_date_checks for more details.
> Task :myTask
Validation failed for task ':myTask', disabling optimizations:
  - Property 'myPrivateInput' is private and annotated with @Input.
Execution optimizations have been disabled for 1 invalid unit(s) of work during the build. Consult deprecation warnings for more information.
BUILD SUCCESSFUL in 60ms
1 actionable task: 1 executed

After:

Property 'myPrivateInput' is private and annotated with @Input. This behaviour has been deprecated and is scheduled to be removed in Gradle 7.0. See https://docs.gradle.org/7.0-20210201140000+0000/userguide/more_about_tasks.html#sec:up_to_date_checks for more details.

> Task :myTask
Avoidance and parallel execution have been disabled for task ':myTask' in order to ensure correctness, due to the following reasons:
  - Property 'myPrivateInput' is private and annotated with @Input.

Avoidance and parallel execution were disabled for 1 unit(s) of work during this build in order to ensure correctness, which potentially increased the build time.
Please consult the build log and deprecation warnings for more details.

Both are problematic, with the most problematic probably being that the given documentation link will do very little to help any non-expert comprehend what is going on here.

Regardless, I propose that the after is better in that:

  • It does not using potentially confusing concept of “validation”, which makes sense to us but not to users
  • It avoids the ambiguous (for a user) concept of “disabling optimizations”
  • It gives some indication that this is actually benefiting a user
  • It makes it clearer that this has a negative build time impact and is different in that manner
  • It is generally more human oriented

@ldaley ldaley requested review from lptr and wolfs and removed request for lptr February 2, 2021 03:18
@pioterj
Copy link
Member

pioterj commented Feb 2, 2021

Nice improvement.

The only issue I see is that the "avoidance" part may not be clear. People may think of "task configuration avoidance" or "compile avoidance".

What exactly are we referring to avoidance here? Up-to-date checking and build caching?

@melix
Copy link
Contributor

melix commented Feb 2, 2021

I very much agree that the new message is more user-friendly. In general we assume too much about the user understanding what we mean, so more context is better.

Avoidance and parallel execution were disabled for 1 unit(s) of work during this build in order to ensure correctness, which potentially increased the build time.
Please consult the build log and deprecation warnings for more details.

"correctness, which potentially increased" reads a bit strange. I'd split that:

****** and parallel execution were disabled for 1 unit(s) of work during this build in order to ensure correctness.
This potentially increased the build time.
Please consult the build log and deprecation warnings for more details.

I don't know what to use for "avoidance" because I'm unsure myself what it refers to, but it should be explicit.

@ldaley
Copy link
Contributor Author

ldaley commented Feb 2, 2021

AFAIK, the three optimizations we are talking about are “Up-to-date checking”, “build caching” and “parallel execution”. These messages are already lengthy, so I tried using “Avoidance” to cover “Up-to-date checks” and “build caching”. I agree that it is awkward. I think the verbosity is better than the ambiguity when all things are considered. I'd update that.

@ldaley
Copy link
Contributor Author

ldaley commented Feb 2, 2021

"correctness, which potentially increased" reads a bit strange. I'd split that:

+1 on that suggestion.

@lptr
Copy link
Member

lptr commented Feb 2, 2021

AFAIK, the three optimizations we are talking about are “Up-to-date checking”, “build caching” and “parallel execution”. These messages are already lengthy, so I tried using “Avoidance” to cover “Up-to-date checks” and “build caching”. I agree that it is awkward. I think the verbosity is better than the ambiguity when all things are considered. I'd update that.

I think we should use the term "execution optimizations." This is a term that both sufficiently covers the things that are in question today and in the future, and it also captures what's important for the user (=they care that their build is executed as optimal as possible).

Note that "avoidance" rhymes with "compilation avoidance" which is not affected by validation problems.

Also note that in some cases we can't disable parallel execution (e.g. because we detect the problem too late). We could say here that "certain execution optimizations are disabled." I think it's sufficient to clarify the actual optimizations being disabled in the linked documentation.

@lptr
Copy link
Member

lptr commented Feb 2, 2021

Another thing we should probably do is to emphasize that the user can get more information (including the documentation link) for deprecation warnings by supplying --warning-mode=all.

@lptr
Copy link
Member

lptr commented Feb 2, 2021

BTW, yet another optimization that is disabled in incremental execution. We might have other things disabled in the future, too.

@ldaley
Copy link
Contributor Author

ldaley commented Feb 2, 2021

I’ll leave it to the execution team to resolve from here as desired.

@ldaley ldaley closed this Feb 2, 2021
@ldaley ldaley deleted the ldaley/more-meaningful-terminology-for-illegal-work-unit-impl-or-config branch February 2, 2021 12:16
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.

4 participants