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

[8.1] Update gradle wrapper to 7.4 (#81963) #83883

Merged
merged 1 commit into from
Feb 14, 2022
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
Expand Up @@ -25,6 +25,7 @@
import org.gradle.api.file.FileCollection;
import org.gradle.api.provider.ListProperty;
import org.gradle.api.tasks.CacheableTask;
import org.gradle.api.tasks.IgnoreEmptyDirectories;
import org.gradle.api.tasks.Input;
import org.gradle.api.tasks.InputFiles;
import org.gradle.api.tasks.Internal;
Expand Down Expand Up @@ -68,6 +69,7 @@ public LicenseHeadersTask() {
* constructor can write to it.
*/
@InputFiles
@IgnoreEmptyDirectories
@SkipWhenEmpty
@PathSensitive(PathSensitivity.RELATIVE)
public List<FileCollection> getJavaFiles() {
Expand Down
4 changes: 2 additions & 2 deletions build-tools-internal/gradle/wrapper/gradle-wrapper.properties
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-7.3.3-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-7.4-all.zip
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionSha256Sum=c9490e938b221daf0094982288e4038deed954a3f12fb54cbf270ddf4e37d879
distributionSha256Sum=cd5c2958a107ee7f0722004a12d0f8559b4564c34daad7df06cffd4d12a426d0
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,9 @@ precommit_master {

single_project_branch {
title = "single project (@testGitCommit@)"
cleanup-tasks = [":server:clean"]
tasks = [":server:spotlessApply", ":server:precommit"]
tasks = [":server:precommit"]
gradle-args = ["--no-scan"]
apply-abi-change-to = "server/src/main/java/org/elasticsearch/Build.java"
apply-abi-change-to = "server/src/main/java/org/elasticsearch/bootstrap/BootstrapInfo.java"
run-using = cli // value can be "cli" or "tooling-api"
daemon = warm // value can be "warm", "cold", or "none"
warm-ups = 5
Expand All @@ -87,10 +86,9 @@ single_project_branch {

single_project_master {
title = "single project (master)"
cleanup-tasks = [":server:clean"]
tasks = [":server:spotlessApply", ":server:precommit"]
tasks = [":server:precommit"]
gradle-args = ["--no-scan"]
apply-abi-change-to = "server/src/main/java/org/elasticsearch/Build.java"
apply-abi-change-to = "server/src/main/java/org/elasticsearch/bootstrap/BootstrapInfo.java"
run-using = cli // value can be "cli" or "tooling-api"
daemon = warm // value can be "warm", "cold", or "none"
warm-ups = 5
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

package org.elasticsearch.gradle.internal.precommit;

import de.thetaphi.forbiddenapis.gradle.CheckForbiddenApis;

import org.gradle.api.file.FileTree;
import org.gradle.api.tasks.IgnoreEmptyDirectories;

/**
* This implementation is used to fix gradle 8 compatibility of
* the CheckForbiddenApis task which is built with gradle 4 support
* in mind.
* */
public class CheckForbiddenApisTask extends CheckForbiddenApis {
Copy link

Choose a reason for hiding this comment

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

In case it could be helpful, discussions to have a fix in the plugin policeman-tools/forbidden-apis#189

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the solutions discussed there don't sound feasible to me. Ideally there would be a forbidden api version that doesn't require compatibility with old gradle versions. Discussed that before. My ideal plan is to move away from this plugin in the long run as its a burden to maintain more and more tbh

Copy link
Contributor

@uschindler uschindler Mar 23, 2022

Choose a reason for hiding this comment

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

Hi,
sorry for all this. I agree to update gradle minimum version, IF it would not be 6.8. Many projects that are using the plugin look like still on Gradle 6. Indeed the problem is also the minimum Java version while compiling, but this will change soon.

In the meantime I am about to release forbiddenapis 3.3 soon.

I know that many developers of Elasticsearch want to use forbiddenapis, but because of behavior of Gradle community and people (not willing to help) and with their shitload of changes (partly ununderstandable), this leads to issues that make plugin maintenance harder and harder. Because of this, I will in the future refuse to accept issues / PRs regarding Gradle. I mentioned this on Twitter already: https://twitter.com/thetaph1/status/1498631483887689732

@breskeby As you were working for Gradle before, maybe take your contacts and ask for including the plugin (which is only a few lines of Gradle-specific code) to the distribution and let people only configure the backend forbiddenapis JAR file (like this is done with other checkers like PMD or checkstyle which have plugin stubs in source code of Gradle and seem to be configurable to use a specific checkstyle/PMD backend). I will then only maintain the actual detection code, but I don't need to maintain the plugin in this ever-changing Gradle world which is a pain in the ass.

I may have a solution for the current problem:

  • simply skip task execution on my own and remove the @SkipIfEmpty annotation. @reta already tried this out also in a first attempt to fix many other plugins for OpenSearch. From my checks this brings no performance problems, because you just move the check into the code if the FileTree is empty or not. Only downside is that it won't print "NO-SOURCE".
  • Do the same like done here in forbiddenapi's plugin-init.groovy by subclassing and use the subclassed Task class when Gradle 6.8 is detected.

The first fix would be easiest and works also with custom Task setup like done in Elasticsearch (looks like you have your own plugin code to setup, because you have many more sourcesets and also use forbiddenapis to scan JAR files going back to Robert Muir's code.

If you want to have the "NO-SOURCE" message back, you can do the same like done here: subclass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@uschindler thanks for reaching out. really appreciated.

Your first suggested solution sounds like a good compromise on keeping version compatibility and fix this problem.

I totally agree that supporting plugins over a wider range of gradle versions is not straight forward and I totally understand your pain. I did that before and it requires also proper test coverage and stepping through quite some loops. On the other hand the "aggressive" way of @gradle moving forward allows quicker innovation IMO. If having the bandwidth to stay up-to-date with gradle versions, this is a huge plus and at elastic we benefit from this a lot imo.

I don't think having the forbidden apis plugin as part of the gradle distribution is something the @gradle team would support, as the general strategy is more of making less plugins part of the distribution instead of more.

Personally, I usually just mimic the gradle version scheme for plugins, meaning that if there is a breaking change in 7.0 that affects my plugin, the major release of my plugin would support that 7.0 version and break older versions. People running with older gradle versions, usually also don't mind running on older plugin versions.

I understand that as an individual contributor doing all this in your spare time is/can be too much to ask and I understand your decision and reasoning, though I have a different perspective on certain things (the gradle community, the cache support etc.)

Declaring no further support for gradle plugin requests from your side would mean for us effectively, creating/forking our own forbidden gradle plugin, that stays up-to-date with latest gradle versions and that would have a different deprecation/support lifecycle. Ideally we make this a public stand alone plugin available via the plugin portal, but to be honest that has not the highest priority for us.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I found an elegant solution: policeman-tools/forbidden-apis#189 (comment)

I can subclass SourceTask which inherits the annotation :-)

Copy link
Contributor

@uschindler uschindler Mar 23, 2022

Choose a reason for hiding this comment

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

Declaring no further support for gradle plugin requests from your side would mean for us effectively, creating/forking our own forbidden gradle plugin, that stays up-to-date with latest gradle versions and that would have a different deprecation/support lifecycle. Ideally we make this a public stand alone plugin available via the plugin portal, but to be honest that has not the highest priority for us.

I will of course make the plugin in a way that it works. I just won't support any crazy logic to support yet again new cache types invented all the time. Gradle is a cache-desaster (listen to complaints by Lucene people especially Robert Muir and Dawid Weiss). Gradle feels like blowing up at some point because it caches also the brain of developers! :-) Instead of caching everythingand spawning daemons all the time, there should be work on removing slow Groovy support (EA versions of Java users will thank you) and redesigning the DSL.

A standalone plugin that just depends on the forbiddenapis core JAR file would be another options, but for maintenance reasons this was abandoned, but I can reinvestiagte. It would be a simple plgin jar only containing the plugin, but no code. It would just depend on forbiddenapis main JAR file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @breskeby: I found a solution for this issue, also done by a lot of other plugin authors - so I am not the first one for this specific issue. Works perfectly, no messages about deprecated features with 7.4.1: policeman-tools/forbidden-apis#193

I also fixed an issue with configuration cache (access of Task#getProject() during task execution caused by stupid Groovy variable/field access scopes) described in policeman-tools/forbidden-apis#179.

Copy link
Contributor

Choose a reason for hiding this comment

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

New version with JDK 18 support will come out soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

@breskeby: Version 3.3 was release a minute ago. This contains the missing annotation and on top also works with configuration cache. You may update it in Elasticsearch and remove the subclassing introduced here.


/**
* Add additional annotation to make this input gradle 8 compliant.
* Otherwise we see a deprecation warning here starting with gradle 7.4
* */
@Override
@IgnoreEmptyDirectories
public FileTree getClassFiles() {
return super.getClassFiles();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.gradle.api.file.FileTree;
import org.gradle.api.file.ProjectLayout;
import org.gradle.api.provider.ListProperty;
import org.gradle.api.tasks.IgnoreEmptyDirectories;
import org.gradle.api.tasks.InputFiles;
import org.gradle.api.tasks.Internal;
import org.gradle.api.tasks.OutputFile;
Expand Down Expand Up @@ -75,6 +76,7 @@ private static boolean isExecutableFile(File file) {
* Returns the files this task will check
*/
@InputFiles
@IgnoreEmptyDirectories
@SkipWhenEmpty
public FileCollection getFiles() {
return getSources().get()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,19 @@

package org.elasticsearch.gradle.internal.precommit;

import de.thetaphi.forbiddenapis.gradle.CheckForbiddenApis;
import de.thetaphi.forbiddenapis.gradle.ForbiddenApisPlugin;
import de.thetaphi.forbiddenapis.gradle.CheckForbiddenApisExtension;
import groovy.lang.Closure;

import org.elasticsearch.gradle.internal.ExportElasticsearchBuildResourcesTask;
import org.elasticsearch.gradle.internal.InternalPlugin;
import org.elasticsearch.gradle.internal.conventions.precommit.PrecommitPlugin;
import org.elasticsearch.gradle.internal.info.BuildParams;
import org.elasticsearch.gradle.util.GradleUtils;
import org.gradle.api.Project;
import org.gradle.api.Task;
import org.gradle.api.plugins.ExtraPropertiesExtension;
import org.gradle.api.tasks.SourceSet;
import org.gradle.api.plugins.JavaBasePlugin;
import org.gradle.api.plugins.JavaPluginExtension;
import org.gradle.api.specs.Specs;
import org.gradle.api.tasks.SourceSetContainer;
import org.gradle.api.tasks.TaskProvider;

Expand All @@ -29,10 +29,24 @@
import java.util.List;
import java.util.Set;

import static de.thetaphi.forbiddenapis.gradle.ForbiddenApisPlugin.FORBIDDEN_APIS_EXTENSION_NAME;
import static de.thetaphi.forbiddenapis.gradle.ForbiddenApisPlugin.FORBIDDEN_APIS_TASK_NAME;

public class ForbiddenApisPrecommitPlugin extends PrecommitPlugin implements InternalPlugin {
@Override
public TaskProvider<? extends Task> createTask(Project project) {
project.getPluginManager().apply(ForbiddenApisPlugin.class);
project.getPluginManager().apply(JavaBasePlugin.class);

// create Extension for defaults:
var checkForbiddenApisExtension = project.getExtensions()
.create(FORBIDDEN_APIS_EXTENSION_NAME, CheckForbiddenApisExtension.class, project);

// Create a convenience task for all checks (this does not conflict with extension, as it has higher priority in DSL):
var forbiddenTask = project.getTasks()
.register(FORBIDDEN_APIS_TASK_NAME, task -> { task.setDescription("Runs forbidden-apis checks."); });

JavaPluginExtension javaPluginExtension = project.getExtensions().getByType(JavaPluginExtension.class);
// Define our tasks (one for each SourceSet):

TaskProvider<ExportElasticsearchBuildResourcesTask> resourcesTask = project.getTasks()
.register("forbiddenApisResources", ExportElasticsearchBuildResourcesTask.class);
Expand All @@ -47,76 +61,69 @@ public TaskProvider<? extends Task> createTask(Project project) {
t.copy("forbidden/es-server-signatures.txt");
t.copy("forbidden/snakeyaml-signatures.txt");
});
project.getTasks().withType(CheckForbiddenApis.class).configureEach(t -> {
t.dependsOn(resourcesTask);

assert t.getName().startsWith(ForbiddenApisPlugin.FORBIDDEN_APIS_TASK_NAME);
String sourceSetName;
if (ForbiddenApisPlugin.FORBIDDEN_APIS_TASK_NAME.equals(t.getName())) {
sourceSetName = "main";
} else {
// parse out the sourceSetName
char[] chars = t.getName().substring(ForbiddenApisPlugin.FORBIDDEN_APIS_TASK_NAME.length()).toCharArray();
chars[0] = Character.toLowerCase(chars[0]);
sourceSetName = new String(chars);
}

SourceSetContainer sourceSets = GradleUtils.getJavaSourceSets(project);
SourceSet sourceSet = sourceSets.getByName(sourceSetName);
t.setClasspath(project.files(sourceSet.getRuntimeClasspath()).plus(sourceSet.getCompileClasspath()));

t.setTargetCompatibility(BuildParams.getMinimumRuntimeVersion().getMajorVersion());
t.setBundledSignatures(Set.of("jdk-unsafe", "jdk-non-portable", "jdk-system-out"));
t.setSignaturesFiles(
project.files(
resourcesDir.resolve("forbidden/jdk-signatures.txt"),
resourcesDir.resolve("forbidden/es-all-signatures.txt"),
resourcesDir.resolve("forbidden/jdk-deprecated.txt")
)
);
t.setSuppressAnnotations(Set.of("**.SuppressForbidden"));
if (t.getName().endsWith("Test")) {
project.getExtensions().getByType(SourceSetContainer.class).configureEach(sourceSet -> {
String sourceSetTaskName = sourceSet.getTaskName(FORBIDDEN_APIS_TASK_NAME, null);
var sourceSetTask = project.getTasks().register(sourceSetTaskName, CheckForbiddenApisTask.class, t -> {
t.setDescription("Runs forbidden-apis checks on '${sourceSet.name}' classes.");
t.dependsOn(sourceSet.getOutput());
t.getOutputs().upToDateWhen(Specs.SATISFIES_ALL);
t.setClassesDirs(sourceSet.getOutput().getClassesDirs());
t.dependsOn(resourcesTask);
t.setClasspath(project.files(sourceSet.getRuntimeClasspath()).plus(sourceSet.getCompileClasspath()));
t.setTargetCompatibility(BuildParams.getMinimumRuntimeVersion().getMajorVersion());
t.setBundledSignatures(Set.of("jdk-unsafe", "jdk-non-portable", "jdk-system-out"));
t.setSignaturesFiles(
t.getSignaturesFiles()
.plus(
project.files(
resourcesDir.resolve("forbidden/es-test-signatures.txt"),
resourcesDir.resolve("forbidden/http-signatures.txt")
)
)
project.files(
resourcesDir.resolve("forbidden/jdk-signatures.txt"),
resourcesDir.resolve("forbidden/es-all-signatures.txt"),
resourcesDir.resolve("forbidden/jdk-deprecated.txt")
)
);
} else {
t.setSignaturesFiles(
t.getSignaturesFiles().plus(project.files(resourcesDir.resolve("forbidden/es-server-signatures.txt")))
);
}
ExtraPropertiesExtension ext = t.getExtensions().getExtraProperties();
ext.set("replaceSignatureFiles", new Closure<Void>(t) {
@Override
public Void call(Object... names) {
List<Path> resources = new ArrayList<>(names.length);
for (Object name : names) {
resources.add(resourcesDir.resolve("forbidden/" + name + ".txt"));
}
t.setSignaturesFiles(project.files(resources));
return null;
t.setSuppressAnnotations(Set.of("**.SuppressForbidden"));
if (t.getName().endsWith("Test")) {
t.setSignaturesFiles(
t.getSignaturesFiles()
.plus(
project.files(
resourcesDir.resolve("forbidden/es-test-signatures.txt"),
resourcesDir.resolve("forbidden/http-signatures.txt")
)
)
);
} else {
t.setSignaturesFiles(
t.getSignaturesFiles().plus(project.files(resourcesDir.resolve("forbidden/es-server-signatures.txt")))
);
}
ExtraPropertiesExtension ext = t.getExtensions().getExtraProperties();
ext.set("replaceSignatureFiles", new Closure<Void>(t) {
@Override
public Void call(Object... names) {
List<Path> resources = new ArrayList<>(names.length);
for (Object name : names) {
resources.add(resourcesDir.resolve("forbidden/" + name + ".txt"));
}
t.setSignaturesFiles(project.files(resources));
return null;
}

});
ext.set("addSignatureFiles", new Closure<Void>(t) {
@Override
public Void call(Object... names) {
List<Path> resources = new ArrayList<>(names.length);
for (Object name : names) {
resources.add(resourcesDir.resolve("forbidden/" + name + ".txt"));
});
ext.set("addSignatureFiles", new Closure<Void>(t) {
@Override
public Void call(Object... names) {
List<Path> resources = new ArrayList<>(names.length);
for (Object name : names) {
resources.add(resourcesDir.resolve("forbidden/" + name + ".txt"));
}
t.setSignaturesFiles(t.getSignaturesFiles().plus(project.files(resources)));
return null;
}
t.setSignaturesFiles(t.getSignaturesFiles().plus(project.files(resources)));
return null;
}
});

});
forbiddenTask.configure(t -> t.dependsOn(sourceSetTask));
});
TaskProvider<Task> forbiddenApis = project.getTasks().named("forbiddenApis");
forbiddenApis.configure(t -> t.setGroup(""));
return forbiddenApis;
return forbiddenTask;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.gradle.api.provider.ListProperty;
import org.gradle.api.provider.Property;
import org.gradle.api.provider.Provider;
import org.gradle.api.tasks.IgnoreEmptyDirectories;
import org.gradle.api.tasks.Input;
import org.gradle.api.tasks.InputFiles;
import org.gradle.api.tasks.Internal;
Expand Down Expand Up @@ -90,6 +91,7 @@ public ForbiddenPatternsTask(ProjectLayout projectLayout) {
}

@InputFiles
@IgnoreEmptyDirectories
@PathSensitive(PathSensitivity.RELATIVE)
@SkipWhenEmpty
public FileCollection getFiles() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.gradle.api.file.FileSystemOperations;
import org.gradle.api.file.FileTree;
import org.gradle.api.model.ObjectFactory;
import org.gradle.api.tasks.IgnoreEmptyDirectories;
import org.gradle.api.tasks.Input;
import org.gradle.api.tasks.InputFiles;
import org.gradle.api.tasks.Internal;
Expand Down Expand Up @@ -417,6 +418,7 @@ public DirectoryProperty getOutputDirectory() {
}

@SkipWhenEmpty
@IgnoreEmptyDirectories
@InputFiles
public FileTree getTestFiles() {
return sourceDirectory.getAsFileTree().matching(testPatternSet);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.gradle.api.file.ProjectLayout;
import org.gradle.api.model.ObjectFactory;
import org.gradle.api.provider.ListProperty;
import org.gradle.api.tasks.IgnoreEmptyDirectories;
import org.gradle.api.tasks.Input;
import org.gradle.api.tasks.InputFiles;
import org.gradle.api.tasks.Internal;
Expand Down Expand Up @@ -86,6 +87,7 @@ public boolean isSkipHasRestTestCheck() {
}

@SkipWhenEmpty
@IgnoreEmptyDirectories
@InputFiles
public FileTree getInputDir() {
FileTree coreFileTree = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.gradle.api.file.ProjectLayout;
import org.gradle.api.model.ObjectFactory;
import org.gradle.api.provider.ListProperty;
import org.gradle.api.tasks.IgnoreEmptyDirectories;
import org.gradle.api.tasks.Input;
import org.gradle.api.tasks.InputFiles;
import org.gradle.api.tasks.Optional;
Expand Down Expand Up @@ -98,6 +99,7 @@ public Map<String, String> getSubstitutions() {
}

@SkipWhenEmpty
@IgnoreEmptyDirectories
@InputFiles
public FileTree getInputDir() {
FileTree coreFileTree = null;
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
7.3.3
7.4
Binary file modified gradle/wrapper/gradle-wrapper.jar
Binary file not shown.
4 changes: 2 additions & 2 deletions gradle/wrapper/gradle-wrapper.properties
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-7.3.3-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-7.4-all.zip
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionSha256Sum=c9490e938b221daf0094982288e4038deed954a3f12fb54cbf270ddf4e37d879
distributionSha256Sum=cd5c2958a107ee7f0722004a12d0f8559b4564c34daad7df06cffd4d12a426d0
4 changes: 2 additions & 2 deletions plugins/examples/gradle/wrapper/gradle-wrapper.properties
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-7.3.3-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-7.4-all.zip
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionSha256Sum=c9490e938b221daf0094982288e4038deed954a3f12fb54cbf270ddf4e37d879
distributionSha256Sum=cd5c2958a107ee7f0722004a12d0f8559b4564c34daad7df06cffd4d12a426d0