Make LoggedExec gradle task configuration cache compatible #87621

breskeby · 2022-06-13T20:56:24Z

This changes the LoggedExec task to be configuration cache compatible. We changed the implementation
to use ExecOperations instead of extending Exec task. As double checked with the Gradle team this task
is not planned to be made configuration cache compatible out of the box anytime soon.

This is part of the effort on #57918

elasticmachine · 2022-07-06T13:22:31Z

Pinging @elastic/es-delivery (Team:Delivery)

build-tools/src/main/java/org/elasticsearch/gradle/LoggedExec.java

pugnascotia · 2022-07-06T13:35:45Z

build-tools/src/main/java/org/elasticsearch/gradle/LoggedExec.java

+            output = byteStreamToString(out);
+        }
+        if (getLogger().isInfoEnabled() == false) {
+            // We use an anonymous inner class here because Gradle cannot properly snapshot this input for the purposes of


I don't see an anonymous inner class being used here?

pugnascotia · 2022-07-06T13:37:27Z

build-tools/src/main/java/org/elasticsearch/gradle/LoggedExec.java

+        if (args.size() == 0) {
+            throw new IllegalArgumentException("Cannot set commandline with of entry size 0");
+        }


Would it be clearer like:

Suggested change

if (args.size() == 0) {

throw new IllegalArgumentException("Cannot set commandline with of entry size 0");

}

if (args.isEmpty()) {

throw new IllegalArgumentException("Cannot set commandLine with empty list");

}

mark-vieira · 2022-07-06T19:48:07Z

build-tools/src/main/java/org/elasticsearch/gradle/LoggedExec.java

+    @TaskAction
+    public void run() {
+        Consumer<Logger> outputLogger;
+        OutputStream out = getCaptureOutput().get() ? new ByteArrayOutputStream() : System.out;


I don't think this is correct. We don't ever want to output directly to standard out. Instead we want to capture output and then only log it in the case of a failure. Essentially, we always need to capture the output, because we hold on to it only to log to the console only in the case of a non-zero exit code.

This also throws an exception as is when the command fails and captureOutput == false since we try to cast System.out to a ByteArrayOutputStream in byteStreamToString() which of course will fail.

mark-vieira · 2022-07-06T19:50:02Z

build-tools/src/main/java/org/elasticsearch/gradle/LoggedExec.java

-            File spoolFile = new File(getProject().getBuildDir() + "/buffered-output/" + this.getName());
-            out = new LazyFileOutputStream(spoolFile);
+            File spoolFile = new File(projectLayout.getBuildDirectory().dir("buffered-output").get().getAsFile(), this.getName());
+            effectiveOutputStream = new TeeOutputStream(out, new LazyFileOutputStream(spoolFile));


Given my comment above, I don't think we'll want to tee the output here if out is always a ByteArrayOutputStream since of course it defeats the purpose of spooling to disk if we are also going to keep the output in memory. I think we can just simplify the whole thing by just setting out above to either a ByteArrayOutputStream or LazyFileOutputSream based on what spoolOutput is set to.

@mark-vieira I've reworked this with two implicit changes:

we now do not support capturing output now for later evaluation if spooling is enabled. I could be achieved by reading from the spool file but we currently have no use case for this

When indenting output is used (e.g. for our nested gradle builds) we do not log out to console by default anymore as this logger used to do so. This should be fine as it still logs to console in failure case.

As an interesting side effect spooling the output for the nested gradle builds works correctly now. Before we configured spooling to be true, but then reset the output stream to the indenting output stream which delegated to console instead of using the earlier configured LazyOutputstream to the spooling file.

I added further integration tests for these scenarios and that the task failure case is handling correctly.

we now do not support capturing output now for later evaluation if spooling is enabled. I could be achieved by reading from the spool file but we currently have no use case for this

I think that's fine.

When indenting output is used (e.g. for our nested gradle builds) we do not log out to console by default anymore as this logger used to do so. This should be fine as it still logs to console in failure case.

I'm on the fence about this. This is primiarily for building BWC distributions and these tasks can be long (over 1m). Having no console output makes it difficult to tell if something is hung or going wrong. My preference would be to keep the existing behavior of teeing to stdout in this scenario. Basically, the whole reason to indent to begin with is so you can differentiate the nested build from the outer one. If we're just spooling to a file and printing on error, the indenting wouldn't be necessary.

As an interesting side effect spooling the output for the nested gradle builds works correctly now. Before we configured spooling to be true, but then reset the output stream to the indenting output stream which delegated to console instead of using the earlier configured LazyOutputstream to the spooling file.

It would seem to me that forwarding to stdout and spooling to disk are redundant. In a "normal" operation, we'd spool the output because we'd only display it in the case of a failure. If we are forwarding the output to the console, then why print it again in the case of a failure.

It seems to me the BWC building case could just be done with a regular Exec task configured with an intending output stream. There's no need to otherwise "hold on to" the output and print it out later. LoggedExec should be used exclusively for shelling out where we don't need to see the output in the console, but we do want to keep it for reporting later.

I've reworked the LoggedExec to fix the indention for nested builds logging to stdout. Using Exec is actually what we do not want anymore as we want to have those tasks configuration cache compatible. Anyhow we should disable spooling for those nested builds

build-tools/src/main/java/org/elasticsearch/gradle/LoggedExec.java

mark-vieira · 2022-07-06T20:00:34Z

build-tools/src/main/java/org/elasticsearch/gradle/LoggedExec.java

@@ -93,11 +117,67 @@ public void setSpoolOutput(boolean spoolOutput) {
                }
            };
        } else {
-            out = new ByteArrayOutputStream();
-            outputLogger = logger -> { logger.error(((ByteArrayOutputStream) out).toString(StandardCharsets.UTF_8)); };
+            outputLogger = logger -> { logger.error(byteStreamToString(out)); };


nit: We can ditch the curly braces here.

mark-vieira · 2022-07-06T20:23:04Z

build-tools/src/main/java/org/elasticsearch/gradle/LoggedExec.java

+    abstract public MapProperty<String, String> getEnvironment();
+
+    @Input
+    abstract public Property<String> getExecutableProperty();


Can we just call this getExecutable? We don't seem to use a "property" suffix anywhere else.

I put an api convenience method of setExecutable(String) in the class. therefore the renaming was required to not break the dynamic method creation. anyhow. I'm ditching this as it was finally used only on a few plugin classes where convenience /readability is not that important

mark-vieira · 2022-07-06T20:29:58Z

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/InternalBwcGitPlugin.java

@@ -107,8 +105,8 @@ public void apply(Project project) {
            });
            fetchLatest.onlyIf(t -> isOffline == false && gitFetchLatest.get());
            fetchLatest.dependsOn(addRemoteTaskProvider);
-            fetchLatest.setWorkingDir(gitExtension.getCheckoutDir().get());
-            fetchLatest.setCommandLine(asList("git", "fetch", "--all"));
+            fetchLatest.getWorkingDir().set(gitExtension.getCheckoutDir().get());


Again, no need to call get().

mark-vieira · 2022-07-06T20:30:36Z

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/BwcSetupExtension.java

-                    loggedExec.environment("JAVA_HOME", getJavaHome(Integer.parseInt(minimumCompilerVersion)));
-                }
-            });
+            loggedExec.getWorkingDir().set(checkoutDir.get());


No need to call get() here on checkoutDir.

mark-vieira · 2022-07-06T20:35:19Z

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/BwcSetupExtension.java

-            });
+            loggedExec.getWorkingDir().set(checkoutDir.get());
+            // Execution time so that the checkouts are available
+            loggedExec.getEnvironment().put("JAVA_HOME", project.provider(() -> {


We can use the Provider APIs to simplify this a bit I think:

loggedExec.getEnvironment().put("JAVA_HOME", unreleasedVersionInfo.zip(checkoutDir, (version, checkoutDir) -> { String minimumCompilerVersion = readFromFile(new File(checkoutDir, minimumCompilerVersionPath(version.version()))); return getJavaHome(Integer.parseInt(minimumCompilerVersion)); }));

mark-vieira · 2022-07-06T20:35:25Z

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/BwcSetupExtension.java

-                }
-            });
+            loggedExec.getWorkingDir().set(checkoutDir.get());
+            // Execution time so that the checkouts are available


This comment is no longer relevant.

mark-vieira · 2022-07-06T20:37:59Z

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/BwcSetupExtension.java

@@ -110,8 +103,7 @@ public void execute(Task t) {
            if (project.getGradle().getStartParameter().isParallelProjectExecutionEnabled()) {
                loggedExec.args("--parallel");
            }
-            loggedExec.setStandardOutput(new IndentingOutputStream(System.out, unreleasedVersionInfo.get().version()));
-            loggedExec.setErrorOutput(new IndentingOutputStream(System.err, unreleasedVersionInfo.get().version()));
+            loggedExec.getOutputIndenting().set(unreleasedVersionInfo.get().version().toString());


We can be more idomatically lazy here and do:

loggedExec.getOutputIndenting().set(unreleasedVersionInfo.map(v -> v.version().toString()));

Exec is fundamentally incompatible with configuration cache. After having a conversation with the gradle team this is not about to change so we came up with our own rework to match configuration cache requirements

…java Co-authored-by: Rory Hunter <pugnascotia@users.noreply.github.com>

mark-vieira

Couple minor things but I think this looks good.

mark-vieira · 2022-07-08T17:06:59Z

build-tools/src/main/java/org/elasticsearch/gradle/LoggedExec.java

+        if (spoolOutput && getCaptureOutput().get()) {
+            throw new GradleException("Capturing output is not supported when spoolOutput is true.");
+        }
+        if (getCaptureOutput().getOrElse(false) && getIndentingConsoleOutput().isPresent()) {


Why do we need getOrElse(false) if we set the convention to false already?

mark-vieira · 2022-07-08T17:08:26Z

build-tools/src/main/java/org/elasticsearch/gradle/LoggedExec.java

+    protected FileSystemOperations fileSystemOperations;
+    private ProjectLayout projectLayout;
+    private ExecOperations execOperations;
+    private boolean spoolOutput;


Why aren't we using a Property for this?

mark-vieira · 2022-07-08T17:11:27Z

build-tools/src/main/java/org/elasticsearch/gradle/LoggedExec.java

+        if (getLogger().isInfoEnabled() == false) {
+            if (exitValue != 0) {
+                try {
+                    getLogger().error("Output for " + getExecutable().get() + ":");


I think we want to omit this is indenting output is enabled so we don't print Output for foo: and then nothing else.

* upstream/master: Pass IndexMetadata to AllocationDecider.can_remain (elastic#88453) [TSDB] Cache rollup bucket timestamp to reduce rounding cost (elastic#88420) Correct some typos/mistakes in comments/docs (elastic#88446) Make ClusterInfo use immutable maps in all cases (elastic#88447) Reduce map lookups (elastic#88418) Don't index geo_shape field in AbstractBuilderTestCase (elastic#88437) Remove usages of TestGeoShapeFieldMapperPlugin from enrich module (elastic#88440) Fix test memory leak (elastic#88362) Improve error when sorting on incompatible types (elastic#88399) Remove usages of BucketCollector#getLeafCollector(LeafReaderContext) (elastic#88414) Mute ReactiveStorageIT::testScaleWhileShrinking (elastic#88431) Clarify snapshot docs on archive indices (elastic#88417) [Stack Monitoring] Switch cgroup memory fields to keyword (elastic#88260) Fix RealmIdentifier XContent parser (elastic#88410) Make LoggedExec gradle task configuration cache compatible (elastic#87621) Update CorruptedFileIT so that it passes with new allocation strategy (elastic#88314) Update RareClusterStateIT to work with the new shards allocator (elastic#87922) Ensure CreateApiKey always creates a new document (elastic#88413) # Conflicts: # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/v2/RollupShardIndexer.java

breskeby self-assigned this Jun 13, 2022

breskeby added :Delivery/Build Build or test infrastructure >enhancement Team:Delivery Meta label for Delivery team v7.17.5 labels Jun 13, 2022

elasticsearchmachine added the v8.4.0 label Jun 13, 2022

breskeby force-pushed the make-logged-exec-cc-compatible branch from 46cb06f to 172b201 Compare July 4, 2022 17:56

breskeby removed the v7.17.5 label Jul 6, 2022

breskeby marked this pull request as ready for review July 6, 2022 13:22

pugnascotia reviewed Jul 6, 2022

View reviewed changes

build-tools/src/main/java/org/elasticsearch/gradle/LoggedExec.java Outdated Show resolved Hide resolved

pugnascotia reviewed Jul 6, 2022

View reviewed changes

pugnascotia approved these changes Jul 6, 2022

View reviewed changes

mark-vieira requested changes Jul 6, 2022

View reviewed changes

breskeby requested review from mark-vieira and pugnascotia July 7, 2022 11:21

breskeby and others added 13 commits July 8, 2022 18:36

Make LoggedExec gradle task configuration cache compatible

4a3e0fd

WIP

ade700a

Rework LoggedExec task to not extend Exec

7b303eb

Exec is fundamentally incompatible with configuration cache. After having a conversation with the gradle team this is not about to change so we came up with our own rework to match configuration cache requirements

Apply changed LoggedExec api

4c8b35c

Fix LoggedExec api usage in scripts

fdf1310

Fix environment nesting in LoggedExec task

95c157f

Api Polishing on LoggedExec task

112b818

Fix AntFixture rework

7988923

Tweak some api and test coverage

6606874

Simplify output handling

0cfdb49

Some more cleanup

72549d5

Update AbstractGradleFuncTest.groovy

4ec6fdd

Update build-tools/src/main/java/org/elasticsearch/gradle/LoggedExec.…

c234915

…java Co-authored-by: Rory Hunter <pugnascotia@users.noreply.github.com>

breskeby added 5 commits July 8, 2022 18:36

Apply review feedback and add test coverage

c23f96e

Fix gradle plugin tests

ad69568

Fix tests on linux

92108e3

Apply review feedback

35569b7

Apply review feedback

b7b42d1

breskeby force-pushed the make-logged-exec-cc-compatible branch from 3327395 to b7b42d1 Compare July 8, 2022 16:39

mark-vieira approved these changes Jul 8, 2022

View reviewed changes

Fix tests after reworking indented output handling

50367f7

breskeby merged commit dbf3974 into elastic:master Jul 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make LoggedExec gradle task configuration cache compatible #87621

Make LoggedExec gradle task configuration cache compatible #87621

breskeby commented Jun 13, 2022 •

edited

Loading

elasticmachine commented Jul 6, 2022

pugnascotia Jul 6, 2022

breskeby Jul 7, 2022

pugnascotia Jul 6, 2022

breskeby Jul 7, 2022

mark-vieira Jul 6, 2022

mark-vieira Jul 6, 2022

breskeby Jul 7, 2022

mark-vieira Jul 7, 2022

breskeby Jul 8, 2022

mark-vieira Jul 6, 2022

breskeby Jul 8, 2022

mark-vieira Jul 6, 2022

breskeby Jul 8, 2022

mark-vieira Jul 6, 2022

mark-vieira Jul 6, 2022

breskeby Jul 8, 2022

mark-vieira Jul 6, 2022

breskeby Jul 8, 2022

mark-vieira Jul 6, 2022

breskeby Jul 11, 2022

mark-vieira Jul 6, 2022

breskeby Jul 8, 2022

mark-vieira left a comment

mark-vieira Jul 8, 2022

mark-vieira Jul 8, 2022

mark-vieira Jul 8, 2022

Make LoggedExec gradle task configuration cache compatible #87621

Make LoggedExec gradle task configuration cache compatible #87621

Conversation

breskeby commented Jun 13, 2022 • edited Loading

elasticmachine commented Jul 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-vieira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breskeby commented Jun 13, 2022 •

edited

Loading