Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add delete-file and copy-file mutators #570

Merged
merged 15 commits into from
Sep 3, 2024
Merged

Add delete-file and copy-file mutators #570

merged 15 commits into from
Sep 3, 2024

Conversation

bdemers
Copy link
Member

@bdemers bdemers commented Aug 30, 2024

  • Add delete-file and copy-file mutators
  • Add system property support for MavenScenarioInvoker

@bdemers
Add delete-file and copy-file mutators
6da57e8 
Individual files can be copied or file/directories deleted.

I needed to get benchmarks running for a Maven build, and replace the .mvn/extensions.xml and related configuration files
@bdemers
Add system property support for MavenScenarioInvoker
fb4aa82
@lptr lptr self-assigned this Sep 2, 2024
lptr
lptr reviewed Sep 2, 2024
View reviewed changes
src/main/java/org/gradle/profiler/mutations/CopyFileMutator.java Outdated Show resolved Hide resolved
src/main/java/org/gradle/profiler/mutations/CopyFileMutator.java Show resolved Hide resolved
src/main/java/org/gradle/profiler/mutations/CopyFileMutator.java Outdated Show resolved Hide resolved
src/main/java/org/gradle/profiler/mutations/CopyFileMutator.java Outdated Show resolved Hide resolved
src/main/java/org/gradle/profiler/mutations/DeleteFileMutator.java Outdated Show resolved Hide resolved
src/main/java/org/gradle/profiler/ScenarioLoader.java
Comment on lines +96 to +97
private static final String DELETE_FILE = "delete-file";
private static final String COPY_FILE = "copy-file";
Copy link
Member

Choose a reason for hiding this comment

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

Considering that these should be able to take directories, too, I think delete and copy would be better names, WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I've been rethinking this, and I think it's okay to include -file; it's good to disambiguate that this is about the file system.

@lptr lptr mentioned this pull request Sep 2, 2024
Merged
@lptr
Allow copy-file to copy to non-existent directory
5bed2b9 
Also resolve target directory based on the project directory.
@lptr lptr requested a review from asodja September 2, 2024 19:30
asodja
asodja reviewed Sep 3, 2024
View reviewed changes
Copy link
Member

@asodja asodja left a comment

Choose a reason for hiding this comment

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

LGTM overall. It would be good we add some documentation for this

src/main/java/org/gradle/profiler/ScenarioLoader.java
@@ -91,6 +94,8 @@ class ScenarioLoader {
private static final String ANDROID_STUDIO_JVM_ARGS = "studio-jvm-args";
private static final String ANDROID_STUDIO_IDEA_PROPERTIES = "idea-properties";
private static final String JVM_ARGS = "jvm-args";
private static final String DELETE_FILE = "delete-file";
Copy link
Member

Choose a reason for hiding this comment

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

❓ Do we plan to add a documentation in README.md in a separate PR? If not it would be good we add it now

Copy link
Member

Choose a reason for hiding this comment

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

Good point.

src/main/java/org/gradle/profiler/mutations/CopyFileMutator.java
this.target = target;
}

@SuppressWarnings("UnstableApiUsage")
Copy link
Member

Choose a reason for hiding this comment

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

❔ Which API is unstable, maybe there is a stable replacement?

Copy link
Member

Choose a reason for hiding this comment

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

It's Guava. I think upgrading Guava is the right solution here. Let me take a look.

src/main/java/org/gradle/profiler/mutations/CopyFileMutator.java Outdated
}
Files.copy(source, target);
} catch (IOException e) {
throw new IllegalStateException("Failed to copy '" + source.getAbsolutePath() + "' to '" + target.getAbsolutePath() + "'", e);
Copy link
Member

Choose a reason for hiding this comment

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

❔ Should we throw UncheckedIOException here, instead of IllegalStateException?

Copy link
Member

Choose a reason for hiding this comment

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

You're right.

src/test/groovy/org/gradle/profiler/mutations/CopyFileMutatorTest.groovy
def target = new File(testDir, "nested/target.txt")

def spec = mockConfigSpec("""{
copy-file {
Copy link
Member

Choose a reason for hiding this comment

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

❔ Can we copy also multiple files or we can copy just one?

Copy link
Member

Choose a reason for hiding this comment

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

We can copy one source (a file or a directory) to a target location.

@lptr
Copy link
Member

lptr commented Sep 3, 2024

Build: https://builds.gradle.org/buildConfiguration/GradleProfiler_GradleProfilerTestTrigger/88291113

asodja
asodja approved these changes Sep 3, 2024
View reviewed changes
Copy link
Member

@asodja asodja left a comment

Choose a reason for hiding this comment

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

LGTM overall

README.md Outdated
- `clear-project-cache-before`: Deletes the contents of the `.gradle` and `buildSrc/.gradle` project cache directories before the scenario is executed (`SCENARIO`), before cleanup (`CLEANUP`) or before the build is executed (`BUILD`).
- `clear-transform-cache-before`: Deletes the contents of the transform cache before the scenario is executed (`SCENARIO`), before cleanup (`CLEANUP`) or before the build is executed (`BUILD`).
- `clear-jars-cache-before`: Deletes the contents of the instrumented jars cache before the scenario is executed (`SCENARIO`), before cleanup (`CLEANUP`) or before the build is executed (`BUILD`).
- `clear-android-studio-cache-before`: Invalidates the Android Studio caches before the scenario is executed (`SCENARIO`) or before the build is executed (`BUILD`). Due to Android Studio client specifics before cleanup (`CLEANUP`) is not supported. Note: cleaning the Android Studio caches is run only when Android Studio sync (`android-studio-sync`) is used.
- `copy-file`: Copies files from one location to another. Has to specify a `source` and a `target` path, both are resolved against the project directory.
Copy link
Member

@asodja asodja Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

❓ Should we mention that copy-file and delete-file works also with directories and not just files?

Also should we mention that relative paths are resolved agains project directories, but absolute paths can also be specified?

Copy link
Member

Choose a reason for hiding this comment

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

I've added some language to clarify.

@lptr lptr merged commit fcc1ecc into gradle:master Sep 3, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lptr lptr lptr left review comments

@asodja asodja asodja approved these changes

Assignees

@lptr lptr

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bdemers @lptr @asodja