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

Improvement usage of gradle task avoidance api #56627

Merged
Merged
Show file tree
Hide file tree
Changes from 5 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
14 changes: 7 additions & 7 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ subprojects {
}
}

task updateCIBwcVersions() {
tasks.register("updateCIBwcVersions") {
doLast {
File yml = file(".ci/bwcVersions")
yml.text = ""
Expand Down Expand Up @@ -142,7 +142,7 @@ allprojects {
}
}

task verifyVersions {
tasks.register("verifyVersions") {
doLast {
if (gradle.startParameter.isOffline()) {
throw new GradleException("Must run in online mode to verify versions")
Expand Down Expand Up @@ -194,18 +194,18 @@ subprojects {
ext.bwc_tests_enabled = bwc_tests_enabled
}

task verifyBwcTestsEnabled {
tasks.register("verifyBwcTestsEnabled") {
doLast {
if (bwc_tests_enabled == false) {
throw new GradleException('Bwc tests are disabled. They must be re-enabled after completing backcompat behavior backporting.')
}
}
}

task branchConsistency {
tasks.register("branchConsistency") {
description 'Ensures this branch is internally consistent. For example, that versions constants match released versions.'
group 'Verification'
dependsOn verifyVersions, verifyBwcTestsEnabled
dependsOn ":verifyVersions", ":verifyBwcTestsEnabled"
}

allprojects {
Expand Down Expand Up @@ -406,7 +406,7 @@ class Run extends DefaultTask {
}
}

task run(type: Run) {
tasks.register("run", Run) {
dependsOn ':distribution:run'
description = 'Runs elasticsearch in the foreground'
group = 'Verification'
Expand Down Expand Up @@ -485,7 +485,7 @@ allprojects {
if (realTask == null) {
return
}
project.tasks.create(taskName) {
project.tasks.register(taskName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this strictly beneficial since this task rule should only run when it finds a task matching this naming convention, right? Granted, it doesn't hurt, just wondering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general I think we just should use register over create where possible. In a later step we need to revisit the actual behaviour and when they got materialised anyhow, but more on a per case fashion I think as thinks get complex there.

There are use cases where tasks in a rule would not be created but registered. You can depend on tasks that are created by a rule, e.g. you could have `myTask.dependsOn tasks.named("cleanCompileJava"). The rule would kick in so the task would be registered, but it doesn't need to be created.

doLast {
println("${realTask.path} dependencies:")
for (Task dep : realTask.getTaskDependencies().getDependencies(realTask)) {
Expand Down
6 changes: 3 additions & 3 deletions buildSrc/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ if (project == rootProject) {
Properties props = VersionPropertiesLoader.loadBuildSrcVersion(project.file('version.properties'))
version = props.getProperty("elasticsearch")

task generateVersionProperties(type: WriteProperties) {
def generateVersionProperties = tasks.register("generateVersionProperties", WriteProperties) {
outputFile = "${buildDir}/version.properties"
comment = 'Generated version properties'
properties(props)
Expand Down Expand Up @@ -234,13 +234,13 @@ if (project != rootProject) {
}
}

task integTest(type: Test) {
tasks.register("integTest", Test) {
inputs.dir(file("src/testKit")).withPropertyName("testkit dir").withPathSensitivity(PathSensitivity.RELATIVE)
systemProperty 'test.version_under_test', version
onlyIf { org.elasticsearch.gradle.info.BuildParams.inFipsJvm == false }
maxParallelForks = System.getProperty('tests.jvms', org.elasticsearch.gradle.info.BuildParams.defaultParallel.toString()) as Integer
}
check.dependsOn(integTest)
check.dependsOn("integTest")

// for now we hardcode the tests for our build to use the gradle jvm.
tasks.withType(Test).configureEach {
Expand Down
2 changes: 1 addition & 1 deletion buildSrc/reaper/build.gradle
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
jar {
archiveName = "${project.name}.jar"
archiveFileName = "${project.name}.jar"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix deprecation warning on the way

manifest {
attributes 'Main-Class': 'org.elasticsearch.gradle.reaper.Reaper'
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import org.gradle.api.GradleException
import org.gradle.api.Task
import org.gradle.api.tasks.Exec
import org.gradle.api.tasks.Internal
import org.gradle.api.tasks.TaskProvider

/**
* A fixture for integration tests which runs in a separate process launched by Ant.
*/
Expand Down Expand Up @@ -79,7 +81,7 @@ class AntFixture extends AntTask implements Fixture {
return tmpFile.exists()
}

private final Task stopTask
private final TaskProvider stopTask

AntFixture() {
stopTask = createStopTask()
Expand All @@ -88,7 +90,7 @@ class AntFixture extends AntTask implements Fixture {

@Override
@Internal
Task getStopTask() {
TaskProvider getStopTask() {
return stopTask
}

Expand Down Expand Up @@ -222,24 +224,29 @@ class AntFixture extends AntTask implements Fixture {
}

/** Adds a task to kill an elasticsearch node with the given pidfile */
private Task createStopTask() {
private TaskProvider createStopTask() {
final AntFixture fixture = this
final Object pid = "${ -> fixture.pid }"
Exec stop = project.tasks.create(name: "${name}#stop", type: LoggedExec)
stop.onlyIf { fixture.pidFile.exists() }
stop.doFirst {
logger.info("Shutting down ${fixture.name} with pid ${pid}")
}
if (Os.isFamily(Os.FAMILY_WINDOWS)) {
stop.executable = 'Taskkill'
stop.args('/PID', pid, '/F')
} else {
stop.executable = 'kill'
stop.args('-9', pid)
}
stop.doLast {
project.delete(fixture.pidFile)
TaskProvider<Exec> stop = project.tasks.register("${name}#stop", LoggedExec)
stop.configure{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing space stop.configure {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

onlyIf { fixture.pidFile.exists() }
doFirst {
logger.info("Shutting down ${fixture.name} with pid ${pid}")
}

if (Os.isFamily(Os.FAMILY_WINDOWS)) {
executable = 'Taskkill'
args('/PID', pid, '/F')
} else {
executable = 'kill'
args('-9', pid)
}
doLast {
project.delete(fixture.pidFile)
}

}

return stop
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,12 @@ class TestWithDependenciesPlugin implements Plugin<Project> {
String outputDir = "${project.buildDir}/generated-resources/${pluginProject.name}"
String camelName = pluginProject.name.replaceAll(/-(\w)/) { _, c -> c.toUpperCase(Locale.ROOT) }
String taskName = "copy" + camelName[0].toUpperCase(Locale.ROOT) + camelName.substring(1) + "Metadata"
Copy copyPluginMetadata = project.tasks.create(taskName, Copy.class)
copyPluginMetadata.into(outputDir)
copyPluginMetadata.from(pluginProject.tasks.pluginProperties)
copyPluginMetadata.from(pluginProject.file('src/main/plugin-metadata'))
project.tasks.register(taskName, Copy.class) {
into(outputDir)
from(pluginProject.tasks.pluginProperties)
from(pluginProject.file('src/main/plugin-metadata'))
}

project.sourceSets.test.output.dir(outputDir, builtBy: taskName)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,16 @@ public void testUpToDateWithSourcesConfigured() {
BuildResult result = getGradleRunner(PROJECT_NAME).withArguments("buildResources", "-s", "-i").build();
assertTaskSuccessful(result, ":buildResources");
assertBuildFileExists(result, PROJECT_NAME, "build-tools-exported/checkstyle.xml");
assertBuildFileExists(result, PROJECT_NAME, "build-tools-exported/checkstyle_suppressions.xml");

// using task avoidance api means the task configuration of the sample task is never triggered
assertBuildFileDoesNotExists(result, PROJECT_NAME, "build-tools-exported/checkstyle_suppressions.xml");

result = getGradleRunner(PROJECT_NAME).withArguments("buildResources", "-s", "-i").build();
assertTaskUpToDate(result, ":buildResources");
assertBuildFileExists(result, PROJECT_NAME, "build-tools-exported/checkstyle.xml");
assertBuildFileExists(result, PROJECT_NAME, "build-tools-exported/checkstyle_suppressions.xml");

// using task avoidance api means the task configuration of the sample task is never triggered
assertBuildFileDoesNotExists(result, PROJECT_NAME, "build-tools-exported/checkstyle_suppressions.xml");
}

public void testImplicitTaskDependencyCopy() {
Expand All @@ -46,8 +50,10 @@ public void testImplicitTaskDependencyCopy() {
assertTaskSuccessful(result, ":buildResources");
assertTaskSuccessful(result, ":sampleCopyAll");
assertBuildFileExists(result, PROJECT_NAME, "sampleCopyAll/checkstyle.xml");
// This is a side effect of compile time reference
assertBuildFileExists(result, PROJECT_NAME, "sampleCopyAll/checkstyle_suppressions.xml");

// using task avoidance api means the task configuration of the sample task is never triggered
// which means buildResource is not configured to copy this file
assertBuildFileDoesNotExists(result, PROJECT_NAME, "sampleCopyAll/checkstyle_suppressions.xml");
}

public void testImplicitTaskDependencyInputFileOfOther() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ public class ThirdPartyAuditTaskIT extends GradleIntegrationTestCase {
@Before
public void setUp() throws Exception {
// Build the sample jars
getGradleRunner("thirdPartyAudit").withArguments("build", "-s").build();
getGradleRunner("thirdPartyAudit").withArguments(":sample_jars:build", "-s").build();
}

public void testElasticsearchIgnored() {
BuildResult result = getGradleRunner("thirdPartyAudit").withArguments(
"clean",
"empty",
":clean",
":empty",
"-s",
"-PcompileOnlyGroup=elasticsearch.gradle:broken-log4j",
"-PcompileOnlyVersion=0.0.1",
Expand All @@ -44,9 +44,9 @@ public void testElasticsearchIgnored() {
}

public void testWithEmptyRules() {
BuildResult result = getGradleRunner("thirdPartyAudit").withArguments(
"clean",
"empty",
getGradleRunner("thirdPartyAudit").withArguments(
":clean",
":empty",
"-s",
"-PcompileOnlyGroup=other.gradle:broken-log4j",
"-PcompileOnlyVersion=0.0.1",
Expand All @@ -57,8 +57,8 @@ public void testWithEmptyRules() {

public void testViolationFoundAndCompileOnlyIgnored() {
BuildResult result = getGradleRunner("thirdPartyAudit").withArguments(
"clean",
"absurd",
":clean",
":absurd",
"-s",
"-PcompileOnlyGroup=other.gradle:broken-log4j",
"-PcompileOnlyVersion=0.0.1",
Expand All @@ -73,8 +73,8 @@ public void testViolationFoundAndCompileOnlyIgnored() {

public void testClassNotFoundAndCompileOnlyIgnored() {
BuildResult result = getGradleRunner("thirdPartyAudit").withArguments(
"clean",
"absurd",
":clean",
":absurd",
"-s",
"-PcompileGroup=other.gradle:broken-log4j",
"-PcompileVersion=0.0.1",
Expand All @@ -94,8 +94,8 @@ public void testClassNotFoundAndCompileOnlyIgnored() {

public void testJarHellWithJDK() {
BuildResult result = getGradleRunner("thirdPartyAudit").withArguments(
"clean",
"absurd",
":clean",
":absurd",
"-s",
"-PcompileGroup=other.gradle:jarhellJdk",
"-PcompileVersion=0.0.1",
Expand All @@ -115,8 +115,8 @@ public void testJarHellWithJDK() {

public void testElasticsearchIgnoredWithViolations() {
BuildResult result = getGradleRunner("thirdPartyAudit").withArguments(
"clean",
"absurd",
":clean",
":absurd",
"-s",
"-PcompileOnlyGroup=elasticsearch.gradle:broken-log4j",
"-PcompileOnlyVersion=0.0.1",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ if (distroConfig != null) {
// setup the test distribution as an artifact of this project
String distroType = System.getProperty('tests.distro.type')

Task buildDistro
def buildDistro
File buildFile
if (['rpm', 'deb'].contains(distroType)) {
buildDistro = project.tasks.create("build" + distroType.capitalize(), Copy) {
buildDistro = project.tasks.register("build" + distroType.capitalize(), Copy) {
from 'files'
into 'build/files'
include 'fake_elasticsearch.tar.gz'
Expand All @@ -21,7 +21,7 @@ if (distroConfig != null) {
extension = "zip"
}
// copy file as is
buildDistro = project.tasks.create("buildArchive", Copy) {
buildDistro = project.tasks.register("buildArchive", Copy) {
from 'files'
include "fake_elasticsearch.${extension}"
into 'build/files'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ elasticsearch_distributions {
}
}

task assertDistroFile {
tasks.register("assertDistroFile") {
dependsOn elasticsearch_distributions.test_distro
doLast {
File distroFile = new File(elasticsearch_distributions.test_distro.toString())
Expand All @@ -50,7 +50,7 @@ task assertDistroFile {
}

if (['rpm', 'deb'].contains(distroType) == false) {
task assertDistroExtracted {
tasks.register("assertDistroExtracted") {
dependsOn elasticsearch_distributions.test_distro.extracted, assertDistroFile
doLast {
File distroExtracted = new File(elasticsearch_distributions.test_distro.extracted.toString())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ buildResources {
copy 'checkstyle.xml'
}

task sampleCopyAll(type: Sync) {
tasks.register("sampleCopyAll", Sync) {
/** Note: no explicit dependency. This works with tasks that use the Provider API a.k.a "Lazy Configuration" **/
from buildResources
into "$buildDir/sampleCopyAll"
}

task sample {
tasks.register("sample") {
// This does not work, task dependencies can't be providers
// dependsOn buildResources.resource('minimumRuntimeVersion')
// Nor does this, despite https://github.com/gradle/gradle/issues/3811
Expand All @@ -29,7 +29,7 @@ task sample {
}
}

task noConfigAfterExecution {
tasks.register("noConfigAfterExecution") {
dependsOn buildResources
doLast {
println "This should cause an error because we are refferencing " +
Expand Down
2 changes: 1 addition & 1 deletion buildSrc/src/testKit/elasticsearch.build/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ loggerUsageCheck.enabled = false
// TODO: shouldn't be part of BuildPlugin, should be tested separately
validateNebulaPom.enabled = false

task hello {
tasks.register("hello") {
doFirst {
println "build plugin can be applied"
}
Expand Down
2 changes: 1 addition & 1 deletion buildSrc/src/testKit/jdk-download/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ project.gradle.projectsEvaluated {
repository.setUrl(fakeJdkRepo)
}

task numConfigurations {
tasks.register("numConfigurations") {
doLast {
println "NUM CONFIGS: ${project.configurations.size()}"
}
Expand Down
6 changes: 3 additions & 3 deletions buildSrc/src/testKit/jdk-download/subproj/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,21 @@ jdks {
}
}

task getLinuxJdk {
tasks.register("getLinuxJdk") {
dependsOn jdks.linux
doLast {
println "JDK HOME: " + jdks.linux
}
}

task getDarwinJdk {
tasks.register("getDarwinJdk") {
dependsOn jdks.darwin
doLast {
println "JDK HOME: " + jdks.darwin
}
}

task getWindowsJdk {
tasks.register("getWindowsJdk") {
dependsOn jdks.windows
doLast {
println "JDK HOME: " + jdks.windows
Expand Down
Loading