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

Dont break config avoidance for other tasks #463

Merged
merged 5 commits into from
Oct 6, 2019
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
3 changes: 3 additions & 0 deletions plugin-gradle/CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

### Version 3.25.0-SNAPSHOT - TBD ([javadoc](https://diffplug.github.io/spotless/javadoc/snapshot/), [snapshot](https://oss.sonatype.org/content/repositories/snapshots/com/diffplug/spotless/spotless-plugin-gradle/))

* Spotless no longer breaks configuration avoidance for other tasks (specifically the `check` task and all of its dependees) ([#463](https://github.com/diffplug/spotless/pull/463)).
* Important change: **Formerly, Spotless did not create its tasks until the `afterEvaluate` phase. Spotless now creates them as soon as the plugin is applied**, and it creates the format-specific tasks as soon as the formats are defined. There is no performance degradation associated with this change, and it makes configuring Spotless easier.

### Version 3.24.3 - September 23rd 2019 ([javadoc](https://diffplug.github.io/spotless/javadoc/spotless-plugin-gradle/3.24.3/), [jcenter](https://bintray.com/diffplug/opensource/spotless-plugin-gradle/3.24.3))

* Update jgit from `5.3.2.201906051522-r` to `5.5.0.201909110433-r`. ([#445](https://github.com/diffplug/spotless/pull/445))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,38 @@
import org.gradle.api.Action;
import org.gradle.api.GradleException;
import org.gradle.api.Project;
import org.gradle.api.Task;
import org.gradle.api.execution.TaskExecutionGraph;
import org.gradle.api.plugins.BasePlugin;

import com.diffplug.common.base.Errors;
import com.diffplug.spotless.LineEnding;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import groovy.lang.Closure;

public class SpotlessExtension {
final Project project;
final Task rootCheckTask, rootApplyTask;

static final String EXTENSION = "spotless";
static final String CHECK = "Check";
static final String APPLY = "Apply";

private static final String TASK_GROUP = "Verification";
private static final String CHECK_DESCRIPTION = "Checks that sourcecode satisfies formatting steps.";
private static final String APPLY_DESCRIPTION = "Applies code formatting steps to sourcecode in-place.";

private static final String FILES_PROPERTY = "spotlessFiles";

public SpotlessExtension(Project project) {
this.project = requireNonNull(project);
rootCheckTask = project.task(EXTENSION + CHECK);
rootCheckTask.setGroup(TASK_GROUP);
rootCheckTask.setDescription(CHECK_DESCRIPTION);
rootApplyTask = project.task(EXTENSION + APPLY);
rootApplyTask.setGroup(TASK_GROUP);
rootApplyTask.setDescription(APPLY_DESCRIPTION);
}

/** Line endings (if any). */
Expand Down Expand Up @@ -201,14 +224,66 @@ private <T extends FormatExtension> T maybeCreate(String name, Class<T> clazz) {
} else {
try {
Constructor<T> constructor = clazz.getConstructor(SpotlessExtension.class);
T newlyCreated = constructor.newInstance(this);
formats.put(name, newlyCreated);
return newlyCreated;
T formatExtension = constructor.newInstance(this);
formats.put(name, formatExtension);
createFormatTask(name, formatExtension);
return formatExtension;
} catch (NoSuchMethodException e) {
throw new GradleException("Must have a constructor " + clazz.getSimpleName() + "(SpotlessExtension root)", e);
} catch (Exception e) {
throw Errors.asRuntime(e);
}
}
}

@SuppressWarnings("rawtypes")
private void createFormatTask(String name, FormatExtension formatExtension) {
// create the SpotlessTask
String taskName = EXTENSION + SpotlessPlugin.capitalize(name);
SpotlessTask spotlessTask = project.getTasks().create(taskName, SpotlessTask.class);
project.afterEvaluate(unused -> formatExtension.setupTask(spotlessTask));

// clean removes the SpotlessCache, so we have to run after clean
Task clean = project.getTasks().getByName(BasePlugin.CLEAN_TASK_NAME);
spotlessTask.mustRunAfter(clean);

// create the check and apply control tasks
Task checkTask = project.getTasks().create(taskName + CHECK);
Task applyTask = project.getTasks().create(taskName + APPLY);

checkTask.dependsOn(spotlessTask);
applyTask.dependsOn(spotlessTask);
// when the task graph is ready, we'll configure the spotlessTask appropriately
project.getGradle().getTaskGraph().whenReady(new Closure(null) {
Copy link
Member

Choose a reason for hiding this comment

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

I think there's a whenReady that supports an Action now isn't there?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is, but not in Gradle 2.x, which we still support.

private static final long serialVersionUID = 1L;

// called by gradle
@SuppressFBWarnings("UMAC_UNCALLABLE_METHOD_OF_ANONYMOUS_CLASS")
public Object doCall(TaskExecutionGraph graph) {
if (graph.hasTask(checkTask)) {
spotlessTask.setCheck();
}
if (graph.hasTask(applyTask)) {
spotlessTask.setApply();
}
return Closure.DONE;
}
});

// set the filePatterns property
project.afterEvaluate(unused -> {
String filePatterns;
if (project.hasProperty(FILES_PROPERTY) && project.property(FILES_PROPERTY) instanceof String) {
filePatterns = (String) project.property(FILES_PROPERTY);
} else {
// needs to be non-null since it is an @Input property of the task
filePatterns = "";
}
spotlessTask.setFilePatterns(filePatterns);
});

// the root tasks depend on the control tasks
rootCheckTask.dependsOn(checkTask);
rootApplyTask.dependsOn(applyTask);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,112 +18,45 @@
import org.gradle.api.Plugin;
import org.gradle.api.Project;
import org.gradle.api.Task;
import org.gradle.api.execution.TaskExecutionGraph;
import org.gradle.api.plugins.BasePlugin;
import org.gradle.api.plugins.JavaBasePlugin;
import org.gradle.util.GradleVersion;

import com.diffplug.spotless.SpotlessCache;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import groovy.lang.Closure;

public class SpotlessPlugin implements Plugin<Project> {
SpotlessExtension spotlessExtension;

static final String EXTENSION = "spotless";
static final String CHECK = "Check";
static final String APPLY = "Apply";

private static final String TASK_GROUP = "Verification";
private static final String CHECK_DESCRIPTION = "Checks that sourcecode satisfies formatting steps.";
private static final String APPLY_DESCRIPTION = "Applies code formatting steps to sourcecode in-place.";
private static final String FILES_PROPERTY = "spotlessFiles";

@Override
public void apply(Project project) {
// make sure there's a `clean` task
project.getPlugins().apply(BasePlugin.class);

// setup the extension
spotlessExtension = project.getExtensions().create(EXTENSION, SpotlessExtension.class, project);
spotlessExtension = project.getExtensions().create(SpotlessExtension.EXTENSION, SpotlessExtension.class, project);

// clear spotless' cache when the user does a clean
Task clean = project.getTasks().getByName(BasePlugin.CLEAN_TASK_NAME);
clean.doLast(unused -> SpotlessCache.clear());

// after the project has been evaluated, configure the check and format tasks per source set
project.afterEvaluate(this::createTasks);
project.afterEvaluate(unused -> {
Copy link
Member

Choose a reason for hiding this comment

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

Is there value in doing this in an afterEvaluate?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so. People can have multiple spotless blocks (such as a default one in a subprojects block in the the root project, then making little changes in a specific subproject), so we have to wait until the script has finished running to know if enforceCheck is true or not. It's the same reason we do afterEvaluate here:

project.afterEvaluate(unused -> formatExtension.setupTask(spotlessTask));

We used to do all the config in afterEvaluate, the main change in the first commit is to create the unconfigured tasks immediately, because it makes it easier for users to connect them with other tasks to not have to do those connections in an afterEvaluate block.

// Add our check task as a dependency on the global check task
// getTasks() returns a "live" collection, so this works even if the
// task doesn't exist at the time this call is made
if (spotlessExtension.enforceCheck) {
if (GradleVersion.current().compareTo(SpotlessPluginLegacy.CONFIG_AVOIDANCE_INTRODUCED) >= 0) {
SpotlessPluginConfigAvoidance.enforceCheck(spotlessExtension, project);
} else {
SpotlessPluginLegacy.enforceCheck(spotlessExtension, project);
}
}
});
}

/** The extension for this plugin. */
public SpotlessExtension getExtension() {
return spotlessExtension;
}

@SuppressWarnings("rawtypes")
void createTasks(Project project) {
Task rootCheckTask = project.task(EXTENSION + CHECK);
rootCheckTask.setGroup(TASK_GROUP);
rootCheckTask.setDescription(CHECK_DESCRIPTION);
Task rootApplyTask = project.task(EXTENSION + APPLY);
rootApplyTask.setGroup(TASK_GROUP);
rootApplyTask.setDescription(APPLY_DESCRIPTION);
String filePatterns;
if (project.hasProperty(FILES_PROPERTY) && project.property(FILES_PROPERTY) instanceof String) {
filePatterns = (String) project.property(FILES_PROPERTY);
} else {
// needs to be non-null since it is an @Input property of the task
filePatterns = "";
}

spotlessExtension.formats.forEach((key, value) -> {
// create the task that does the work
String taskName = EXTENSION + capitalize(key);
SpotlessTask spotlessTask = project.getTasks().create(taskName, SpotlessTask.class);
value.setupTask(spotlessTask);
spotlessTask.setFilePatterns(filePatterns);

// create the check and apply control tasks
Task checkTask = project.getTasks().create(taskName + CHECK);
Task applyTask = project.getTasks().create(taskName + APPLY);
// the root tasks depend on them
rootCheckTask.dependsOn(checkTask);
rootApplyTask.dependsOn(applyTask);
// and they depend on the work task
checkTask.dependsOn(spotlessTask);
applyTask.dependsOn(spotlessTask);

// when the task graph is ready, we'll configure the spotlessTask appropriately
project.getGradle().getTaskGraph().whenReady(new Closure(null) {
private static final long serialVersionUID = 1L;

// called by gradle
@SuppressFBWarnings("UMAC_UNCALLABLE_METHOD_OF_ANONYMOUS_CLASS")
public Object doCall(TaskExecutionGraph graph) {
if (graph.hasTask(checkTask)) {
spotlessTask.setCheck();
}
if (graph.hasTask(applyTask)) {
spotlessTask.setApply();
}
return Closure.DONE;
}
});
});

// Add our check task as a dependency on the global check task
// getTasks() returns a "live" collection, so this works even if the
// task doesn't exist at the time this call is made
if (spotlessExtension.enforceCheck) {
project.getTasks()
.matching(task -> task.getName().equals(JavaBasePlugin.CHECK_TASK_NAME))
.all(task -> task.dependsOn(rootCheckTask));
}

// clear spotless' cache when the user does a clean, but only after any spotless tasks
Task clean = project.getTasks().getByName(BasePlugin.CLEAN_TASK_NAME);
clean.doLast(unused -> SpotlessCache.clear());
project.getTasks()
.withType(SpotlessTask.class)
.all(task -> task.mustRunAfter(clean));
}

static String capitalize(String input) {
return Character.toUpperCase(input.charAt(0)) + input.substring(1);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Copyright 2016 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.gradle.spotless;

import org.gradle.api.Project;
import org.gradle.api.Task;
import org.gradle.api.plugins.JavaBasePlugin;
import org.gradle.api.tasks.TaskProvider;

class SpotlessPluginConfigAvoidance {
static void enforceCheck(SpotlessExtension extension, Project project) {
TaskProvider<Task> check = project.getTasks().named(JavaBasePlugin.CHECK_TASK_NAME);
check.configure(task -> task.dependsOn(extension.rootCheckTask));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright 2016 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.gradle.spotless;

import org.gradle.api.Project;
import org.gradle.api.Task;
import org.gradle.api.plugins.JavaBasePlugin;
import org.gradle.util.GradleVersion;

class SpotlessPluginLegacy {
static final GradleVersion CONFIG_AVOIDANCE_INTRODUCED = GradleVersion.version("4.9");

static void enforceCheck(SpotlessExtension extension, Project project) {
Task check = project.getTasks().getByName(JavaBasePlugin.CHECK_TASK_NAME);
check.dependsOn(extension.rootCheckTask);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,8 @@ public void setApply() {
/** Returns the name of this format. */
String formatName() {
String name = getName();
if (name.startsWith(SpotlessPlugin.EXTENSION)) {
return name.substring(SpotlessPlugin.EXTENSION.length()).toLowerCase(Locale.ROOT);
if (name.startsWith(SpotlessExtension.EXTENSION)) {
return name.substring(SpotlessExtension.EXTENSION.length()).toLowerCase(Locale.ROOT);
} else {
return name;
}
Expand All @@ -179,10 +179,10 @@ String formatName() {
@TaskAction
public void performAction(IncrementalTaskInputs inputs) throws Exception {
if (target == null) {
throw new GradleException("You must specify 'Iterable<File> toFormat'");
throw new GradleException("You must specify 'Iterable<File> target'");
}
if (!check && !apply) {
throw new GradleException("Don't call " + getName() + " directly, call " + getName() + SpotlessPlugin.CHECK + " or " + getName() + SpotlessPlugin.APPLY);
throw new GradleException("Don't call " + getName() + " directly, call " + getName() + SpotlessExtension.CHECK + " or " + getName() + SpotlessExtension.APPLY);
}

Predicate<File> shouldInclude;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Copyright 2016 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.gradle.spotless;

import java.io.IOException;

import org.assertj.core.api.Assertions;
import org.gradle.testkit.runner.GradleRunner;
import org.junit.Test;

public class ConfigAvoidanceTest extends GradleIntegrationTest {
protected final GradleRunner gradleRunnerConfigAvoidance() throws IOException {
return gradleRunner().withGradleVersion(SpotlessPluginLegacy.CONFIG_AVOIDANCE_INTRODUCED.getVersion());
}

@Test
public void noConfigOnHelp() throws IOException {
setFile("build.gradle").toLines(
"buildscript { repositories { mavenCentral() } }",
"plugins {",
" id 'com.diffplug.gradle.spotless'",
"}",
"apply plugin: 'java'",
"spotless {",
" java {",
" googleJavaFormat('1.2')",
" }",
"}",
"",
"class ConfigureCanary extends DefaultTask {",
" ConfigureCanary() {",
" println('Canary was configured')",
" }",
"",
" @TaskAction",
" def action() {",
" println('Canary ran')",
" }",
"}",
"def canary = tasks.register('canary', ConfigureCanary) {}",
"tasks.named('check').configure {",
" dependsOn(canary)",
"}");
setFile("src/main/java/test.java").toResource("java/googlejavaformat/JavaCodeUnformatted.test");

String help_4_9 = gradleRunnerConfigAvoidance().withArguments("help").build().getOutput();
Assertions.assertThat(help_4_9).doesNotContain("Canary was configured");
String check_4_9 = gradleRunnerConfigAvoidance().withArguments("check").buildAndFail().getOutput();
Assertions.assertThat(check_4_9).contains("Canary was configured", "Canary ran", "Execution failed for task ':spotlessJava'");
}
}
Loading