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.

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

fix gradle custom step with closure and configuration cache #2376

Merged
merged 12 commits into from
Jan 6, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016-2023 DiffPlug
* Copyright 2016-2025 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -193,6 +193,8 @@ static class RuntimeInit {
/////////////////////////////////
// USER AND SYSTEM-WIDE VALUES //
/////////////////////////////////
FS.DETECTED.setGitSystemConfig(new File("no-global-git-config-for-spotless")); // this fixes a problem
// that was only occurring on Java 11. If we remove support for Java 11, we could probably remove it.
systemConfig = SystemReader.getInstance().openSystemConfig(null, FS.DETECTED);
Errors.log().run(systemConfig::load);
userConfig = SystemReader.getInstance().openUserConfig(systemConfig, FS.DETECTED);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import java.util.List;
import java.util.Objects;

import com.diffplug.spotless.yaml.SerializeToByteArrayHack;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

/**
Expand Down Expand Up @@ -51,27 +53,43 @@
* to make Spotless work with all of Gradle's cache systems at once.
*/
public class ConfigurationCacheHackList implements java.io.Serializable {
private static final long serialVersionUID = 1L;
private static final long serialVersionUID = 6914178791997323870L;

private boolean optimizeForEquality;
private ArrayList<Object> backingList = new ArrayList<>();

private boolean shouldWeSerializeToByteArrayFirst() {
return backingList.stream().anyMatch(step -> step instanceof SerializeToByteArrayHack);
}

private void writeObject(java.io.ObjectOutputStream out) throws IOException {
boolean serializeToByteArrayFirst = shouldWeSerializeToByteArrayFirst();
out.writeBoolean(serializeToByteArrayFirst);
out.writeBoolean(optimizeForEquality);
out.writeInt(backingList.size());
for (Object obj : backingList) {
// if write out the list on its own, we'll get java's non-deterministic object-graph serialization
// by writing each object to raw bytes independently, we avoid this
out.writeObject(LazyForwardingEquality.toBytes((Serializable) obj));
if (serializeToByteArrayFirst) {
out.writeObject(LazyForwardingEquality.toBytes((Serializable) obj));
} else {
out.writeObject(obj);
}
}
}

@SuppressFBWarnings("MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT")
private void readObject(java.io.ObjectInputStream in) throws IOException, ClassNotFoundException {
boolean serializeToByteArrayFirst = in.readBoolean();
optimizeForEquality = in.readBoolean();
backingList = new ArrayList<>();
int size = in.readInt();
for (int i = 0; i < size; i++) {
backingList.add(LazyForwardingEquality.fromBytes((byte[]) in.readObject()));
if (serializeToByteArrayFirst) {
backingList.add(LazyForwardingEquality.fromBytes((byte[]) in.readObject()));
} else {
backingList.add(in.readObject());
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright 2025 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.diffplug.spotless.yaml;

import java.io.File;

import javax.annotation.Nullable;

import com.diffplug.spotless.FormatterStep;

/**
* This step is a flag which marks that `ConfigurationCacheHackList` should
* serialize each item individually into `byte[]` array, rather than using normal
* serialization.
*
* The reason to use this is if you are using `toggleOffOn` *and* two kinds of
* google-java-format (e.g. one for format and the other for imports), then
* problems with Java's handling of object graphs will cause your up-to-date checks
* to always fail. `CombinedJavaFormatStepTest` recreates this situation. By adding
* this step, it will trigger this workaround which fixes the up-to-dateness bug.
*
* But, turning it on will break all `custom` steps that use Groovy closures. So
* by default you get regular serialization. If you're using `toggleOffOn` and having
* problems with up-to-dateness, then adding this step can be a workaround.
*/
public class SerializeToByteArrayHack implements FormatterStep {
private static final long serialVersionUID = 8071047581828362545L;

@Override
public String getName() {
return "hack to force serializing objects to byte array";
}

@Nullable
@Override
public String format(String rawUnix, File file) throws Exception {
return null;
}

@Override
public void close() throws Exception {

}
}
4 changes: 4 additions & 0 deletions plugin-gradle/CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
* Bump minimum `eclipse-cdt` version to `11.0` (removed support for `10.7`). ([#2373](https://github.com/diffplug/spotless/pull/2373))
### Fixed
* `toggleOffOn` now works with the configuration cache. ([#2378](https://github.com/diffplug/spotless/pull/2378) fixes [#2317](https://github.com/diffplug/spotless/issues/2317))
* Using `custom` with a Groovy closure now works with and without configuration cache. ([#2376](https://github.com/diffplug/spotless/pull/2376))
* Minimum required Gradle version for this to work has bumped from `8.0` to `8.4`.
* The global git system config is now ignored for line-ending purposes.
* Added `SerializeToByteArrayHack` as a flag for a limitation at the intersection of `toggleOffOn` and `custom`.
* You can now use `removeUnusedImports` and `googleJavaFormat` at the same time again. (fixes [#2159](https://github.com/diffplug/spotless/issues/2159))
* The default list of type annotations used by `formatAnnotations` now includes Jakarta Validation's `Valid` and constraints validations (fixes [#2334](https://github.com/diffplug/spotless/issues/2334))
* `indentWith[Spaces|Tabs]` has been deprecated in favor of `leadingTabsToSpaces` and `leadingSpacesToTabs`. ([#2350](https://github.com/diffplug/spotless/pull/2350) fixes [#794](https://github.com/diffplug/spotless/issues/794))
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016-2024 DiffPlug
* Copyright 2016-2025 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -457,12 +457,11 @@ protected Integer calculateState() throws Exception {
*/
public void custom(String name, Closure<String> formatter) {
requireNonNull(formatter, "formatter");
Closure<String> dehydrated = formatter.dehydrate();
custom(name, new ClosureFormatterFunc(dehydrated));
custom(name, new ClosureFormatterFunc(formatter));
}

static class ClosureFormatterFunc implements FormatterFunc, Serializable {
private final Closure<String> closure;
private Closure<String> closure;

ClosureFormatterFunc(Closure<String> closure) {
this.closure = closure;
Expand All @@ -472,6 +471,14 @@ static class ClosureFormatterFunc implements FormatterFunc, Serializable {
public String apply(String unixNewlines) {
return closure.call(unixNewlines);
}

private void writeObject(java.io.ObjectOutputStream stream) throws java.io.IOException {
stream.writeObject(closure.dehydrate());
}

private void readObject(java.io.ObjectInputStream stream) throws java.io.IOException, ClassNotFoundException {
this.closure = (Closure<String>) stream.readObject();
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016-2024 DiffPlug
* Copyright 2016-2025 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -29,7 +29,7 @@ public class SpotlessPlugin implements Plugin<Project> {
static final String SPOTLESS_MODERN = "spotlessModern";
static final String VER_GRADLE_min = "6.1.1";
static final String VER_GRADLE_javaPluginExtension = "7.1";
static final String VER_GRADLE_minVersionForCustom = "8.0";
static final String VER_GRADLE_minVersionForCustom = "8.4";
private static final int MINIMUM_JRE = 11;

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016-2024 DiffPlug
* Copyright 2016-2025 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -17,9 +17,39 @@

import java.io.IOException;

import org.gradle.testkit.runner.GradleRunner;
import org.junit.jupiter.api.Test;

class BumpThisNumberIfACustomStepChangesTest extends GradleIntegrationHarness {
abstract class BumpThisNumberIfACustomStepChangesTest extends GradleIntegrationHarness {
private boolean useConfigCache;

BumpThisNumberIfACustomStepChangesTest(boolean useConfigCache) {
this.useConfigCache = useConfigCache;
}

static class WithConfigCache extends BumpThisNumberIfACustomStepChangesTest {
WithConfigCache() {
super(true);
}
}

static class WithoutConfigCache extends BumpThisNumberIfACustomStepChangesTest {
WithoutConfigCache() {
super(false);
}
}

@Override
public GradleRunner gradleRunner() throws IOException {
if (useConfigCache) {
setFile("gradle.properties").toLines("org.gradle.unsafe.configuration-cache=true",
"org.gradle.configuration-cache=true");
return super.gradleRunner().withGradleVersion(GradleVersionSupport.CUSTOM_STEPS.version);
} else {
return super.gradleRunner();
}
}

private void writeBuildFile(String toInsert) throws IOException {
setFile("build.gradle").toLines(
"plugins {",
Expand Down Expand Up @@ -50,7 +80,12 @@ void customRuleNeverUpToDate() throws IOException {
writeContentWithBadFormatting();
applyIsUpToDate(false);
checkIsUpToDate(false);
checkIsUpToDate(false);
if (useConfigCache) {
// if the config cache is in-effect, then it's okay for custom rules to become "up-to-date"
checkIsUpToDate(true);
} else {
checkIsUpToDate(false);
}
}

@Test
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2023-2024 DiffPlug
* Copyright 2023-2025 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -31,6 +31,7 @@
import com.diffplug.spotless.java.GoogleJavaFormatStep;
import com.diffplug.spotless.java.ImportOrderStep;
import com.diffplug.spotless.java.RemoveUnusedImportsStep;
import com.diffplug.spotless.yaml.SerializeToByteArrayHack;

public class CombinedJavaFormatStepTest extends ResourceHarness {

Expand All @@ -45,6 +46,7 @@ void checkIssue1679() {
FenceStep toggleOffOnPair = FenceStep.named(FenceStep.defaultToggleName()).openClose("formatting:off", "formatting:on");
try (StepHarness formatter = StepHarness.forSteps(
toggleOffOnPair.preserveWithin(List.of(
new SerializeToByteArrayHack(),
gjf,
indentWithSpaces,
importOrder,
Expand Down
Loading