Use services for archive and file operations in tasks

[breskeby]

• 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 Elasticsearch build and Gradle plugins should support Gradle configuration cache #57918

[elasticmachine]

Pinging @elastic/es-core-infra (:Core/Infra/Build)

[mark-vieira]

Few comments.

[breskeby]

My initial thought here was that we should use method injector in favorite of constructor injection as this would have a slight benefit in performance as this service would not have been resolved if it’s not user. But thinking again since you asked we probably can move to the more idiomatic constructor injection as 1. with task avoidance api those tasks should only be created if needed, 2. when the service is used in configuration time it doesn’t matter as it’s always used 3. those services are created already anyhow as they’re likely used within gradle core already. I‘m moving to constructor invocation.

…

On 30. Sep 2020, at 00:57, Mark Vieira ***@***.***> wrote:  @mark-vieira commented on this pull request. Few comments. In buildSrc/src/main/java/org/elasticsearch/gradle/ElasticsearchTestBasePlugin.java: > @@ -38,6 +38,7 @@ import java.util.Map; import static org.elasticsearch.gradle.util.GradleUtils.maybeConfigure; +import static org.gradle.util.GFileUtils.mkdirs; This is psudo-internal. Can we rely on this? In buildSrc/src/main/java/org/elasticsearch/gradle/FileSystemOperationsAware.java: > + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.gradle; + +import org.gradle.api.tasks.WorkResult; + +/** + * A marker interface for tasks signaling that they support basic file operations. + * Methods will be added as needed. + */ + +public interface FileSystemOperationsAware { + WorkResult delete(Object... objects); Not really a "marker" interface if it defines methods. I'm not sure this provides a lot of benefit, as is. Perhaps we can have the interface define the injected method to be implemented by supporting types then provide default method impls that delegate to getFileSystemOperations()? In buildSrc/src/main/java/org/elasticsearch/gradle/LoggedExec.java: > private static final Logger LOGGER = Logging.getLogger(LoggedExec.class); private Consumer<Logger> outputLogger; + @Inject + protected FileSystemOperations getFileSystemOperations() { + throw new UnsupportedOperationException(); Can't we inject this into the constructor as well? If so, doesn't that avoid having this weird exception throwing method implementation? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe.

[breskeby]

@elasticmachine test this please

[mark-vieira]

I‘m moving to constructor invocation.

👍