Skip to content
This repository has been archived by the owner on Feb 6, 2024. It is now read-only.

Remove usage of Project during task execution #455

Closed

Conversation

liutikas
Copy link

@liutikas liutikas commented Mar 1, 2023

  • Replace project with injected FileOperations object
  • Replace project with logger as that's the only thing used

Fixes part of GoogleCloudPlatform/appengine-plugins#997 issue

Test: existing tests pass

- Replace project with injected FileOperations object
- Replace project with logger as that's the only thing used

Fixes part of #454 issue

Test: existing tests pass
Copy link
Contributor

@vlsi vlsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please do not use .gradle..internal.. classes
  2. FileSystemOperations requires Gradle 6.0, so either the implementation should be adjusted to support Gradle 5, or someone should decide if bumping the minimal version to 6.0 was acceptable. If the minimal Gradle version is increased, then README should be updated as well to reflect the change.

public abstract class StageAppYamlTask extends DefaultTask {

@Inject
abstract public FileOperations getFileOperations();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FileSystemOperations requires Gradle 6.0+
Recently there was a commit comparing the version with 5.5 (see #451 ), so I'm not sure if bumping the minimal version to Gradle 6.0 is generally expected from an innocent change like this

Suggested change
abstract public FileOperations getFileOperations();
abstract public FileSystemOperations getFileSystemOperations();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't take my usage of 5.1 as a guide :) I used the lowest possible point where the new API is available. During maintenance this makes the condition disappear faster than if it's at the point of deprecation.

A more relevant discussion might be: #450

Copy link
Contributor

@TWiStErRob TWiStErRob Aug 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the minimum supported version is Gradle 4.0, which means this suggestion can't work.

I think the getFileOperations() needs a

// TODO use public org.gradle.api.file.FileSystemOperations once the minimum version is greater than Gradle 6.0.

but until then it's safe to use as the FileOperations API, however internal, was added in Gradle 0.9 and hasn't changed since then (for this use case).


Alternatively we could use some other library to do the same. e.g.

org.gradle.util.GFileUtils.deleteDirectory(appYamlExtension.getStagingDirectory());

or

org.apache.tools.ant.util.FileUtils.delete(appYamlExtension.getStagingDirectory());

or plain old Java 8 (assuming minimum supported version allows, what is the minimum Java for this plugin? Gradle 4.0 supports Java 7 minimum):

Files.walk(pathToBeDeleted)
      .sorted(Comparator.reverseOrder())
      .map(Path::toFile)
      .forEach(File::delete);

and since the .delete() just deleted the folder it's possible to just use plain appYamlExtension.getStagingDirectory().mkdir().

import org.gradle.api.DefaultTask;
import org.gradle.api.internal.file.FileOperations;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not import org.gradle.api.internal packages. Those are not meant for general use in plugin code

Suggested change
import org.gradle.api.internal.file.FileOperations;
import org.gradle.api.file.FileSystemOperations;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not possible to use the public API here, because it was added in Gradle 6.0 as you said.

Copy link
Contributor

@TWiStErRob TWiStErRob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is almost ready. I reviewed again deeper and it looks good, googleJavaFormat is failing the build with trivial problems. I commented what needs changing/deciding to get this merged.

import org.gradle.api.DefaultTask;
import org.gradle.api.internal.file.FileOperations;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not possible to use the public API here, because it was added in Gradle 6.0 as you said.

public abstract class StageAppYamlTask extends DefaultTask {

@Inject
abstract public FileOperations getFileOperations();
Copy link
Contributor

@TWiStErRob TWiStErRob Aug 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the minimum supported version is Gradle 4.0, which means this suggestion can't work.

I think the getFileOperations() needs a

// TODO use public org.gradle.api.file.FileSystemOperations once the minimum version is greater than Gradle 6.0.

but until then it's safe to use as the FileOperations API, however internal, was added in Gradle 0.9 and hasn't changed since then (for this use case).


Alternatively we could use some other library to do the same. e.g.

org.gradle.util.GFileUtils.deleteDirectory(appYamlExtension.getStagingDirectory());

or

org.apache.tools.ant.util.FileUtils.delete(appYamlExtension.getStagingDirectory());

or plain old Java 8 (assuming minimum supported version allows, what is the minimum Java for this plugin? Gradle 4.0 supports Java 7 minimum):

Files.walk(pathToBeDeleted)
      .sorted(Comparator.reverseOrder())
      .map(Path::toFile)
      .forEach(File::delete);

and since the .delete() just deleted the folder it's possible to just use plain appYamlExtension.getStagingDirectory().mkdir().

public abstract class StageAppYamlTask extends DefaultTask {

@Inject
abstract public FileOperations getFileOperations();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gradlew googleJavaFormat wants this modifier order:

Suggested change
abstract public FileOperations getFileOperations();
public abstract FileOperations getFileOperations();

@@ -20,12 +20,13 @@
import com.google.cloud.tools.managedcloudsdk.ConsoleListener;
import org.gradle.api.Project;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gradlew googleJavaFormat found an unused import here, which makes sense since you removed the field.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants