Support `--dry-run` in rich console #2164

lacasseio · 2017-05-26T19:29:03Z

Context

See #2060.

Gradle Core Team Checklist

Verify design and implementation
Verify test coverage and CI build status
Verify documentation including proper use of @since and @Incubating annotations for all public APIs
Recognize contributor in release notes

This moves parsing of the --dry-run option to the logging module so that logging can be configured differently when attached to a terminal or using --console=rich. Issue: #2060

big-guy · 2017-05-26T19:35:33Z

...logging/src/integTest/groovy/org/gradle/internal/logging/DryRunLoggingIntegrationTest.groovy

+        succeeds('baz')
+
+        then:
+        result.output.contains(":foo SKIPPED${EOL}:bar SKIPPED${EOL}:baz SKIPPED")


couldn't this just use the built-in parsing we have in the result?

e.g., result.assertTasksSkipped(":foo", ":bar", ":baz")

Good point, done.

big-guy · 2017-05-26T19:35:55Z

...logging/src/integTest/groovy/org/gradle/internal/logging/DryRunLoggingIntegrationTest.groovy

+        succeeds('baz')
+
+        then:
+        output =~ /(?s).*> Task :foo.*> Task :bar.*> Task :baz.*/


could we use the grouped output fixture?

I think we could assert on getOutput() being empty for that task grouping.

The rich output isn't connected to the result from the plain console. So we can't use the normal fixture for asserting order. If we find them in the output, it also means the --dry-run is working as expected since they aren't printing any output.

Wouldn't we also want to assert the concrete status here? I'd expect SKIPPED.

big-guy · 2017-05-26T19:36:31Z

...logging/src/integTest/groovy/org/gradle/internal/logging/DryRunLoggingIntegrationTest.groovy

+        buildFile << """
+            task foo {}
+            task bar(dependsOn: foo) {}
+            task baz(dependsOn: bar) {}


We should give these tasks actions so they're not skipped by default.

+1 noticed that too.

big-guy · 2017-05-26T19:38:21Z

...rojects/logging/src/main/java/org/gradle/api/logging/configuration/LoggingConfiguration.java

+
+    /**
+     * Enables or disables dry-run (log but do not perform task actions) mode.
+     */


Is this necessary? It seems really weird to me that dry-run would be a logging option.

I'd agree that it wouldn't see this as being part of logging.

I think too and the execution goes through in WithLogging class. At this point. we aren't parsing the entire StartParameter. I don't know what would be the best choice for this. We could do like the parallel configuration and extra it to its own configuration class if you like.

Yes, I think the better option is to only have a new method setDryRun(boolean) on LoggingManagerInternal. If you go for the AbstractCommandLineConverter model then we wouldn't have to expose the new method to LoggingConfiguration which is preferable from my perspective.

big-guy · 2017-05-26T19:48:36Z

...ogging/src/main/java/org/gradle/internal/logging/sink/GroupingProgressLogEventGenerator.java

+            if (didHaveOutput) {
+                spans.add(EOL);
+            }
+            spans.addAll(headerFormatter.format(loggingHeader, description, shortDescription, status));


I wonder if we need to be careful with the amount of garbage we may be creating now.

We create a 2 new lists here on every header and only sometimes need the second one.

Good point, I will rewrite the logic.

big-guy · 2017-05-26T19:49:54Z

...ogging/src/main/java/org/gradle/internal/logging/sink/GroupingProgressLogEventGenerator.java

                    listener.onOutput(header());
                }

+                didHaveOutput = bufferedLogs.size() > 0;


I think it might be easier to follow this if we marked didHaveOutput when we buffer? WDYT?

We want to know if the last forwarded output had any output (except for the header). I don't think it would give the same result.

bmuschko · 2017-05-26T19:53:40Z

...logging/src/integTest/groovy/org/gradle/internal/logging/DryRunLoggingIntegrationTest.groovy

+        buildFile << """
+            task foo {}
+            task bar(dependsOn: foo) {}
+            task baz(dependsOn: bar) {}


+1 noticed that too.

bmuschko · 2017-05-26T19:54:34Z

...logging/src/integTest/groovy/org/gradle/internal/logging/DryRunLoggingIntegrationTest.groovy

+        succeeds('baz')
+
+        then:
+        output =~ /(?s).*> Task :foo.*> Task :bar.*> Task :baz.*/


I think we could assert on getOutput() being empty for that task grouping.

bmuschko · 2017-05-26T19:57:30Z

...rojects/logging/src/main/java/org/gradle/api/logging/configuration/LoggingConfiguration.java

+
+    /**
+     * Enables or disables dry-run (log but do not perform task actions) mode.
+     */


I'd agree that it wouldn't see this as being part of logging.

bmuschko · 2017-05-26T20:07:45Z

subprojects/logging/src/main/java/org/gradle/internal/logging/events/DryRunChangeEvent.java

+import org.gradle.api.logging.LogLevel;
+
+public class DryRunChangeEvent extends OutputEvent {
+    private boolean dryRunEnabled;


Should become a immutable final field that is being passed in through the constructor.

adammurdoch

The 'dry-run' setting shouldn't be part of the logging configuration. Instead, the task execution events that the renderers receives should indicate whether the task was executed or not.

Alternatively, we should make --dry-run a report and not even pretend to run the tasks.

lacasseio · 2017-05-30T11:52:57Z

Using PR #2178 instead.

eriwen and others added 2 commits May 26, 2017 14:51

Print all tasks when dry run is enabled

b3a8505

This moves parsing of the --dry-run option to the logging module so that logging can be configured differently when attached to a terminal or using --console=rich. Issue: #2060

Polish for code review

17706b7

lacasseio added a:feature A new functionality from:member in:logging labels May 26, 2017

lacasseio added this to the 4.0 RC1 milestone May 26, 2017

lacasseio self-assigned this May 26, 2017

bmuschko self-requested a review May 26, 2017 19:46

big-guy requested changes May 26, 2017

View reviewed changes

Address code review change

2285514

bmuschko suggested changes May 26, 2017

View reviewed changes

Daniel Lacasse added 4 commits May 26, 2017 16:13

Fix windows build

36f53b2

Remove unused field

78b46fb

Address more code review comment

e26b3d3

Make checkstyle happy

7a9f15c

adammurdoch suggested changes May 28, 2017

View reviewed changes

lacasseio closed this May 30, 2017

lacasseio deleted the dl/rich-dry-run branch May 31, 2017 22:13

lacasseio mentioned this pull request Aug 21, 2017

Skipped tasks are not part of the BuildResult outcome #2732

Closed

lacasseio mentioned this pull request Sep 18, 2017

Add Verbose console type #2933

Merged

ov7a removed this from the 4.0 RC1 milestone Mar 28, 2024

Support --dry-run in rich console #2164

Support --dry-run in rich console #2164

Uh oh!

Conversation

lacasseio commented May 26, 2017

Context

Gradle Core Team Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmuschko May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adammurdoch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lacasseio commented May 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Support `--dry-run` in rich console #2164

Support `--dry-run` in rich console #2164

bmuschko May 26, 2017 •

edited

Loading

adammurdoch left a comment •

edited

Loading