Skip to content

Commit f58ebe5

Browse files
breskebybreskeby
authored
Use services for archive and file operations in tasks (#62968) (#63201)
Referencing a project instance during task execution is discouraged by Gradle and should be avoided. E.g. It is incompatible with Gradles incubating configuration cache. Instead there are services available to handle archive and filesystem operations in task actions. Brings us one step closer to #57918
1 parent 1e63313 commit f58ebe5

File tree

23 files changed

+305
-52
lines changed

23 files changed

+305
-52
lines changed

‎buildSrc/src/main/groovy/org/elasticsearch/gradle/AntTask.groovy‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,11 @@ import org.apache.tools.ant.DefaultLogger
2525
import org.apache.tools.ant.Project
2626
import org.gradle.api.DefaultTask
2727
import org.gradle.api.GradleException
28+
import org.gradle.api.file.FileSystemOperations
2829
import org.gradle.api.tasks.Input
2930
import org.gradle.api.tasks.TaskAction
3031

32+
import javax.inject.Inject
3133
import java.nio.charset.Charset
3234

3335
/**
@@ -43,6 +45,11 @@ public abstract class AntTask extends DefaultTask {
4345
*/
4446
public final ByteArrayOutputStream outputBuffer = new ByteArrayOutputStream()
4547

48+
@Inject
49+
protected FileSystemOperations getFileSystemOperations() {
50+
throw new UnsupportedOperationException();
51+
}
52+
4653
@TaskAction
4754
final void executeTask() {
4855
AntBuilder ant = new AntBuilder()

‎buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/LicenseHeadersTask.groovy‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ import java.nio.file.Files
3535
* <p>
3636
* This is a port of the apache lucene check
3737
*/
38-
public class LicenseHeadersTask extends AntTask {
38+
class LicenseHeadersTask extends AntTask {
3939

4040
@OutputFile
4141
File reportFile = new File(project.buildDir, 'reports/licenseHeaders/rat.log')

‎buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ class PrecommitTasks {
2828

2929
/** Adds a precommit task, which depends on non-test verification tasks. */
3030

31-
public static void create(Project project, boolean includeDependencyLicenses) {
31+
static void create(Project project, boolean includeDependencyLicenses) {
3232

3333
project.pluginManager.apply(CheckstylePrecommitPlugin)
3434
project.pluginManager.apply(ForbiddenApisPrecommitPlugin)

‎buildSrc/src/main/groovy/org/elasticsearch/gradle/test/AntFixture.groovy‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,10 @@ class AntFixture extends AntTask implements Fixture {
9696

9797
@Override
9898
protected void runAnt(AntBuilder ant) {
99-
project.delete(baseDir) // reset everything
99+
// reset everything
100+
getFileSystemOperations().delete {
101+
it.delete(baseDir)
102+
}
100103
cwd.mkdirs()
101104
final String realExecutable
102105
final List<Object> realArgs = new ArrayList<>()
@@ -242,7 +245,9 @@ class AntFixture extends AntTask implements Fixture {
242245
args('-9', pid)
243246
}
244247
doLast {
245-
project.delete(fixture.pidFile)
248+
getFileSystemOperations().delete {
249+
it.delete(fixture.pidFile)
250+
}
246251
}
247252

248253
}

‎buildSrc/src/main/java/org/elasticsearch/gradle/ElasticsearchTestBasePlugin.java‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import java.io.File;
3939
import java.util.Map;
4040

41+
import static org.elasticsearch.gradle.util.FileUtils.mkdirs;
4142
import static org.elasticsearch.gradle.util.GradleUtils.maybeConfigure;
4243

4344
/**
@@ -84,10 +85,10 @@ public void apply(Project project) {
8485
test.doFirst(new Action<>() {
8586
@Override
8687
public void execute(Task t) {
87-
project.mkdir(testOutputDir);
88-
project.mkdir(heapdumpDir);
89-
project.mkdir(test.getWorkingDir());
90-
project.mkdir(test.getWorkingDir().toPath().resolve("temp"));
88+
mkdirs(testOutputDir);
89+
mkdirs(heapdumpDir);
90+
mkdirs(test.getWorkingDir());
91+
mkdirs(test.getWorkingDir().toPath().resolve("temp").toFile());
9192

9293
// TODO remove once jvm.options are added to test system properties
9394
if (BuildParams.getRuntimeJavaVersion() == JavaVersion.VERSION_1_8) {

‎buildSrc/src/main/java/org/elasticsearch/gradle/FileSystemOperationsAware.java‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/*
2+
* Licensed to Elasticsearch under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.elasticsearch.gradle;
21+
22+
import org.gradle.api.tasks.WorkResult;
23+
24+
/**
25+
* An interface for tasks that support basic file operations.
26+
* Methods will be added as needed.
27+
*/
28+
public interface FileSystemOperationsAware {
29+
WorkResult delete(Object... objects);
30+
}

‎buildSrc/src/main/java/org/elasticsearch/gradle/LoggedExec.java‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,15 +22,18 @@
2222
import org.gradle.api.GradleException;
2323
import org.gradle.api.Project;
2424
import org.gradle.api.Task;
25+
import org.gradle.api.file.FileSystemOperations;
2526
import org.gradle.api.logging.Logger;
2627
import org.gradle.api.logging.Logging;
2728
import org.gradle.api.tasks.Exec;
29+
import org.gradle.api.tasks.WorkResult;
2830
import org.gradle.process.BaseExecSpec;
2931
import org.gradle.process.ExecOperations;
3032
import org.gradle.process.ExecResult;
3133
import org.gradle.process.ExecSpec;
3234
import org.gradle.process.JavaExecSpec;
3335

36+
import javax.inject.Inject;
3437
import java.io.ByteArrayOutputStream;
3538
import java.io.File;
3639
import java.io.IOException;
@@ -47,13 +50,15 @@
4750
* A wrapper around gradle's Exec task to capture output and log on error.
4851
*/
4952
@SuppressWarnings("unchecked")
50-
public class LoggedExec extends Exec {
53+
public class LoggedExec extends Exec implements FileSystemOperationsAware {
5154

5255
private static final Logger LOGGER = Logging.getLogger(LoggedExec.class);
5356
private Consumer<Logger> outputLogger;
57+
private FileSystemOperations fileSystemOperations;
5458

55-
public LoggedExec() {
56-
59+
@Inject
60+
public LoggedExec(FileSystemOperations fileSystemOperations) {
61+
this.fileSystemOperations = fileSystemOperations;
5762
if (getLogger().isInfoEnabled() == false) {
5863
setIgnoreExitValue(true);
5964
setSpoolOutput(false);
@@ -154,4 +159,9 @@ private static <T extends BaseExecSpec> ExecResult genericExec(Function<Action<T
154159
throw e;
155160
}
156161
}
162+
163+
@Override
164+
public WorkResult delete(Object... objects) {
165+
return fileSystemOperations.delete(d -> d.delete(objects));
166+
}
157167
}

‎buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/CopyRestApiTask.java‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,9 @@
2424
import org.gradle.api.DefaultTask;
2525
import org.gradle.api.Project;
2626
import org.gradle.api.artifacts.Configuration;
27+
import org.gradle.api.file.ArchiveOperations;
2728
import org.gradle.api.file.ConfigurableFileCollection;
29+
import org.gradle.api.file.FileSystemOperations;
2830
import org.gradle.api.file.FileTree;
2931
import org.gradle.api.plugins.JavaPluginConvention;
3032
import org.gradle.api.provider.ListProperty;
@@ -47,6 +49,8 @@
4749
import java.util.Set;
4850
import java.util.stream.Collectors;
4951

52+
import static org.elasticsearch.gradle.util.GradleUtils.getProjectPathFromTask;
53+
5054
/**
5155
* Copies the files needed for the Rest YAML specs to the current projects test resources output directory.
5256
* This is intended to be be used from {@link RestResourcesPlugin} since the plugin wires up the needed
@@ -76,6 +80,16 @@ protected Factory<PatternSet> getPatternSetFactory() {
7680
throw new UnsupportedOperationException();
7781
}
7882

83+
@Inject
84+
protected FileSystemOperations getFileSystemOperations() {
85+
throw new UnsupportedOperationException();
86+
}
87+
88+
@Inject
89+
protected ArchiveOperations getArchiveOperations() {
90+
throw new UnsupportedOperationException();
91+
}
92+
7993
@Input
8094
public ListProperty<String> getIncludeCore() {
8195
return includeCore;
@@ -137,23 +151,23 @@ public File getOutputDir() {
137151

138152
@TaskAction
139153
void copy() {
140-
Project project = getProject();
141154
// always copy the core specs if the task executes
155+
String projectPath = getProjectPathFromTask(getPath());
142156
if (BuildParams.isInternal()) {
143-
getLogger().debug("Rest specs for project [{}] will be copied to the test resources.", project.getPath());
144-
project.copy(c -> {
157+
getLogger().debug("Rest specs for project [{}] will be copied to the test resources.", projectPath);
158+
getFileSystemOperations().copy(c -> {
145159
c.from(coreConfig.getAsFileTree());
146160
c.into(getOutputDir());
147161
c.include(corePatternSet.getIncludes());
148162
});
149163
} else {
150164
getLogger().debug(
151165
"Rest specs for project [{}] will be copied to the test resources from the published jar (version: [{}]).",
152-
project.getPath(),
166+
projectPath,
153167
VersionProperties.getElasticsearch()
154168
);
155-
project.copy(c -> {
156-
c.from(project.zipTree(coreConfig.getSingleFile()));
169+
getFileSystemOperations().copy(c -> {
170+
c.from(getArchiveOperations().zipTree(coreConfig.getSingleFile()));
157171
// this ends up as the same dir as outputDir
158172
c.into(Objects.requireNonNull(getSourceSet().orElseThrow().getOutput().getResourcesDir()));
159173
if (includeCore.get().isEmpty()) {
@@ -167,8 +181,8 @@ void copy() {
167181
}
168182
// only copy x-pack specs if explicitly instructed
169183
if (includeXpack.get().isEmpty() == false) {
170-
getLogger().debug("X-pack rest specs for project [{}] will be copied to the test resources.", project.getPath());
171-
project.copy(c -> {
184+
getLogger().debug("X-pack rest specs for project [{}] will be copied to the test resources.", projectPath);
185+
getFileSystemOperations().copy(c -> {
172186
c.from(xpackConfig.getSingleFile());
173187
c.into(getOutputDir());
174188
c.include(xpackPatternSet.getIncludes());
@@ -177,7 +191,7 @@ void copy() {
177191
// TODO: once https://github.com/elastic/elasticsearch/pull/62968 lands ensure that this uses `getFileSystemOperations()`
178192
// copy any additional config
179193
if (additionalConfig != null) {
180-
project.copy(c -> {
194+
getFileSystemOperations().copy(c -> {
181195
c.from(additionalConfig.getAsFileTree());
182196
c.into(getOutputDir());
183197
});

‎buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/CopyRestTestsTask.java‎

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,9 @@
2424
import org.gradle.api.DefaultTask;
2525
import org.gradle.api.Project;
2626
import org.gradle.api.artifacts.Configuration;
27+
import org.gradle.api.file.ArchiveOperations;
2728
import org.gradle.api.file.ConfigurableFileCollection;
29+
import org.gradle.api.file.FileSystemOperations;
2830
import org.gradle.api.file.FileTree;
2931
import org.gradle.api.plugins.JavaPluginConvention;
3032
import org.gradle.api.provider.ListProperty;
@@ -44,6 +46,8 @@
4446
import java.util.Optional;
4547
import java.util.stream.Collectors;
4648

49+
import static org.elasticsearch.gradle.util.GradleUtils.getProjectPathFromTask;
50+
4751
/**
4852
* Copies the Rest YAML test to the current projects test resources output directory.
4953
* This is intended to be be used from {@link RestResourcesPlugin} since the plugin wires up the needed
@@ -73,6 +77,16 @@ protected Factory<PatternSet> getPatternSetFactory() {
7377
throw new UnsupportedOperationException();
7478
}
7579

80+
@Inject
81+
protected FileSystemOperations getFileSystemOperations() {
82+
throw new UnsupportedOperationException();
83+
}
84+
85+
@Inject
86+
protected ArchiveOperations getArchiveOperations() {
87+
throw new UnsupportedOperationException();
88+
}
89+
7690
@Input
7791
public ListProperty<String> getIncludeCore() {
7892
return includeCore;
@@ -127,25 +141,24 @@ public File getOutputDir() {
127141

128142
@TaskAction
129143
void copy() {
130-
Project project = getProject();
144+
String projectPath = getProjectPathFromTask(getPath());
131145
// only copy core tests if explicitly instructed
132146
if (includeCore.get().isEmpty() == false) {
133147
if (BuildParams.isInternal()) {
134-
getLogger().debug("Rest tests for project [{}] will be copied to the test resources.", project.getPath());
135-
project.copy(c -> {
148+
getLogger().debug("Rest tests for project [{}] will be copied to the test resources.", projectPath);
149+
getFileSystemOperations().copy(c -> {
136150
c.from(coreConfig.getAsFileTree());
137151
c.into(getOutputDir());
138152
c.include(corePatternSet.getIncludes());
139153
});
140-
141154
} else {
142155
getLogger().debug(
143156
"Rest tests for project [{}] will be copied to the test resources from the published jar (version: [{}]).",
144-
project.getPath(),
157+
projectPath,
145158
VersionProperties.getElasticsearch()
146159
);
147-
project.copy(c -> {
148-
c.from(project.zipTree(coreConfig.getSingleFile()));
160+
getFileSystemOperations().copy(c -> {
161+
c.from(getArchiveOperations().zipTree(coreConfig.getSingleFile()));
149162
// this ends up as the same dir as outputDir
150163
c.into(Objects.requireNonNull(getSourceSet().orElseThrow().getOutput().getResourcesDir()));
151164
c.include(
@@ -156,17 +169,16 @@ void copy() {
156169
}
157170
// only copy x-pack tests if explicitly instructed
158171
if (includeXpack.get().isEmpty() == false) {
159-
getLogger().debug("X-pack rest tests for project [{}] will be copied to the test resources.", project.getPath());
160-
project.copy(c -> {
172+
getLogger().debug("X-pack rest tests for project [{}] will be copied to the test resources.", projectPath);
173+
getFileSystemOperations().copy(c -> {
161174
c.from(xpackConfig.getAsFileTree());
162175
c.into(getOutputDir());
163176
c.include(xpackPatternSet.getIncludes());
164177
});
165178
}
166-
// TODO: once https://github.com/elastic/elasticsearch/pull/62968 lands ensure that this uses `getFileSystemOperations()`
167179
// copy any additional config
168180
if (additionalConfig != null) {
169-
project.copy(c -> {
181+
getFileSystemOperations().copy(c -> {
170182
c.from(additionalConfig.getAsFileTree());
171183
c.into(getOutputDir());
172184
});

0 commit comments

Comments
 (0)