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

Run forbidden api checks with runtimeJavaVersion #32947

Merged
merged 8 commits into from
Aug 22, 2018
Merged
Show file tree
Hide file tree
Changes from 4 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 @@ -18,17 +18,12 @@
*/
package org.elasticsearch.gradle.precommit

import de.thetaphi.forbiddenapis.gradle.CheckForbiddenApis
import de.thetaphi.forbiddenapis.gradle.ForbiddenApisPlugin
import org.elasticsearch.gradle.ExportElasticsearchBuildResourcesTask
import org.gradle.api.JavaVersion
import org.gradle.api.Project
import org.gradle.api.Task
import org.gradle.api.file.FileCollection
import org.gradle.api.plugins.JavaBasePlugin
import org.gradle.api.plugins.quality.Checkstyle
import org.gradle.api.tasks.JavaExec
import org.gradle.api.tasks.StopExecutionException

/**
* Validation tasks which should be run before committing. These run before tests.
*/
Expand All @@ -37,8 +32,8 @@ class PrecommitTasks {
/** Adds a precommit task, which depends on non-test verification tasks. */
public static Task create(Project project, boolean includeDependencyLicenses) {
List<Task> precommitTasks = [
configureForbiddenApis(project),
configureCheckstyle(project),
configureForbiddenApisCli(project),
configureNamingConventions(project),
project.tasks.create('forbiddenPatterns', ForbiddenPatternsTask.class),
project.tasks.create('licenseHeaders', LicenseHeadersTask.class),
Expand All @@ -47,9 +42,6 @@ class PrecommitTasks {
project.tasks.create('thirdPartyAudit', ThirdPartyAuditTask.class)
]

// Configure it but don't add it as a dependency yet
configureForbiddenApisCli(project)

// tasks with just tests don't need dependency licenses, so this flag makes adding
// the task optional
if (includeDependencyLicenses) {
Expand Down Expand Up @@ -83,72 +75,46 @@ class PrecommitTasks {
return project.tasks.create(precommitOptions)
}

private static Task configureForbiddenApis(Project project) {
project.pluginManager.apply(ForbiddenApisPlugin.class)
project.forbiddenApis {
failOnUnsupportedJava = false
bundledSignatures = ['jdk-unsafe', 'jdk-deprecated', 'jdk-non-portable', 'jdk-system-out']
signaturesURLs = [getClass().getResource('/forbidden/jdk-signatures.txt'),
getClass().getResource('/forbidden/es-all-signatures.txt')]
suppressAnnotations = ['**.SuppressForbidden']
}
project.tasks.withType(CheckForbiddenApis) {
// we do not use the += operator to add signatures, as conventionMappings of Gradle do not work when it's configured using withType:
if (name.endsWith('Test')) {
signaturesURLs = project.forbiddenApis.signaturesURLs +
[ getClass().getResource('/forbidden/es-test-signatures.txt'), getClass().getResource('/forbidden/http-signatures.txt') ]
} else {
signaturesURLs = project.forbiddenApis.signaturesURLs +
[ getClass().getResource('/forbidden/es-server-signatures.txt') ]
}
}
Task forbiddenApis = project.tasks.findByName('forbiddenApis')
forbiddenApis.group = "" // clear group, so this does not show up under verification tasks

return forbiddenApis
}

private static Task configureForbiddenApisCli(Project project) {
project.configurations.create("forbiddenApisCliJar")
project.dependencies {
forbiddenApisCliJar 'de.thetaphi:forbiddenapis:2.5'
}
Task forbiddenApisCli = project.tasks.create('forbiddenApisCli')

Task forbiddenApisCli = project.tasks.create('forbiddenApis')
project.sourceSets.forEach { sourceSet ->
forbiddenApisCli.dependsOn(
project.tasks.create(sourceSet.getTaskName('forbiddenApisCli', null), JavaExec) {
project.tasks.create(sourceSet.getTaskName('forbiddenApis', null), ForbiddenApisCliTask) {
ExportElasticsearchBuildResourcesTask buildResources = project.tasks.getByName('buildResources')
dependsOn(buildResources)
classpath = project.files(
project.configurations.forbiddenApisCliJar,
sourceSet.compileClasspath,
sourceSet.runtimeClasspath
execAction = { spec ->
spec.classpath(sourceSet.compileClasspath)
spec.classpath(sourceSet.runtimeClasspath)
spec.executable = "${project.runtimeJavaHome}/bin/java"
}
targetCompatibility = JavaVersion.VERSION_1_8 // FIXME: change to min(compilerVersion, 10)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will wait for the PR fixing the violations to be merged before changing this and merging this PR.

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 this should always be compiler version? That way if eg we use java 11, we will detect problems ahead of time, before we switch to using java 11 all the time?

bundledSignatures = [
"jdk-unsafe", "jdk-deprecated", "jdk-non-portable", "jdk-system-out"
]
signaturesFiles = project.files(
buildResources.copy("forbidden/jdk-signatures.txt"),
buildResources.copy("forbidden/es-all-signatures.txt")
)
main = 'de.thetaphi.forbiddenapis.cli.CliMain'
executable = "${project.runtimeJavaHome}/bin/java"
args "-b", 'jdk-unsafe-1.8'
args "-b", 'jdk-deprecated-1.8'
args "-b", 'jdk-non-portable'
args "-b", 'jdk-system-out'
args "-f", buildResources.copy("forbidden/jdk-signatures.txt")
args "-f", buildResources.copy("forbidden/es-all-signatures.txt")
args "--suppressannotation", '**.SuppressForbidden'
suppressAnnotations = ['**.SuppressForbidden']
if (sourceSet.name == 'test') {
args "-f", buildResources.copy("forbidden/es-test-signatures.txt")
args "-f", buildResources.copy("forbidden/http-signatures.txt")
signaturesFiles += project.files(
buildResources.copy("forbidden/es-test-signatures.txt"),
buildResources.copy("forbidden/http-signatures.txt")
)
} else {
args "-f", buildResources.copy("forbidden/es-server-signatures.txt")
signaturesFiles += project.files(buildResources.copy("forbidden/es-server-signatures.txt"))
}
dependsOn sourceSet.classesTaskName
doFirst {
// Forbidden APIs expects only existing dirs, and requires at least one
FileCollection existingOutputs = sourceSet.output.classesDirs
.filter { it.exists() }
if (existingOutputs.isEmpty()) {
throw new StopExecutionException("${sourceSet.name} has no outputs")
}
existingOutputs.forEach { args "-d", it }
classesDirs = sourceSet.output.classesDirs
ext.replaceSignatureFiles = { String... names ->
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 stick with the dsl of forbidden apis, instead of adding these helpers? I would like us to be able to switch back if/when it supports forking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It this is the only concern, we can add these helpers with any implementation of the task.
I don't have a strong preference, but do like how these make the build scripts shorter, without involving build resources.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, that is fine. Can we please name it set instead of replace then, to match common naming with other gradle dsl elements?

Copy link
Contributor Author

@alpar-t alpar-t Aug 21, 2018

Choose a reason for hiding this comment

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

@rjernst set will not work, as the task already has that method, so the extension will never be fetched and called, but even with something like setSignatures it would be confusing as I don't think signatures = ... would work, as it won't call the closure on task extensions but look for a set method on the task instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to merge and submit a PR for the third party audit as well.
Let me know and I'll make changes to the name if needed,

signaturesFiles = project.files(
names.collect { buildResources.copy("forbidden/${it}.txt") }
)
}
ext.addSignatureFiles = { String... names ->
signaturesFiles += project.files(
names.collect { buildResources.copy("forbidden/${it}.txt") }
)
}
}
)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you 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 org.elasticsearch.gradle;

import java.net.URISyntaxException;
import java.net.URL;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;

public class ClassPathUtils {

public static Path getJar(Class<?> theClass) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this hack, and especially not making it easier to use. Can it be isolated, or at least given large warning comments to not use this?

URL location = theClass.getProtectionDomain().getCodeSource().getLocation();
if (location.getProtocol().equals("file") == false) {
throw new IllegalArgumentException(
"Unexpected location for " + theClass.getName() + ": "+ location
);
}
final Path path;
try {
path = Paths.get(location.toURI());
} catch (URISyntaxException e) {
throw new AssertionError(e);
}
if (Files.exists(path) == false) {
throw new AssertionError("Bath to class source does not exist: " + path);
Copy link
Member

Choose a reason for hiding this comment

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

typo: bath -> path

}
return path;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;
Expand Down Expand Up @@ -105,7 +106,7 @@ public void doExport() {
if (is == null) {
throw new GradleException("Can't export `" + resourcePath + "` from build-tools: not found");
}
Files.copy(is, destination);
Files.copy(is, destination, StandardCopyOption.REPLACE_EXISTING);
} catch (IOException e) {
throw new GradleException("Can't write resource `" + resourcePath + "` to " + destination, e);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you 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 org.elasticsearch.gradle.precommit;

import de.thetaphi.forbiddenapis.cli.CliMain;
import org.elasticsearch.gradle.ClassPathUtils;
import org.gradle.api.Action;
import org.gradle.api.DefaultTask;
import org.gradle.api.JavaVersion;
import org.gradle.api.file.FileCollection;
import org.gradle.api.tasks.Input;
import org.gradle.api.tasks.InputFiles;
import org.gradle.api.tasks.OutputFile;
import org.gradle.api.tasks.SkipWhenEmpty;
import org.gradle.api.tasks.TaskAction;
import org.gradle.process.JavaExecSpec;

import java.io.File;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Collections;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;

public class ForbiddenApisCliTask extends DefaultTask {

private FileCollection signaturesFiles;
private List<String> signatures = new ArrayList<>();
private Set<String> bundledSignatures = new LinkedHashSet<>();
private Set<String> suppressAnnotations = new LinkedHashSet<>();
private JavaVersion targetCompatibility;
private FileCollection classesDirs;
private Action<JavaExecSpec> execAction;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This avoids inheriting from JavaExec. I prefer that because this way the task can control when to do the exec and we can do the required setup in the task action.
Gradle runs the task action of the inherited task first ( I couldn't find a documented order ).


public JavaVersion getTargetCompatibility() {
return targetCompatibility;
}

public void setTargetCompatibility(JavaVersion targetCompatibility) {
this.targetCompatibility = targetCompatibility;
}

public Action<JavaExecSpec> getExecAction() {
return execAction;
}

public void setExecAction(Action<JavaExecSpec> execAction) {
this.execAction = execAction;
}

@OutputFile
public File getMarkerFile() {
return new File(
new File(getProject().getBuildDir(), this.getClass().getSimpleName()),
getName()
);
}

@InputFiles
@SkipWhenEmpty
public FileCollection getClassesDirs() {
return classesDirs.filter(File::exists);
}

public void setClassesDirs(FileCollection classesDirs) {
this.classesDirs = classesDirs;
}

@InputFiles
public FileCollection getSignaturesFiles() {
return signaturesFiles;
}

public void setSignaturesFiles(FileCollection signaturesFiles) {
this.signaturesFiles = signaturesFiles;
}

@Input
public List<String> getSignatures() {
return signatures;
}

public void setSignatures(List<String> signatures) {
this.signatures = signatures;
}

@Input
public Set<String> getBundledSignatures() {
return bundledSignatures;
}

public void setBundledSignatures(Set<String> bundledSignatures) {
this.bundledSignatures = bundledSignatures;
}

@Input
public Set<String> getSuppressAnnotations() {
return suppressAnnotations;
}

public void setSuppressAnnotations(Set<String> suppressAnnotations) {
this.suppressAnnotations = suppressAnnotations;
}

@TaskAction
public void runCheck() throws IOException {
Path inlineSignatures = Paths.get(
getProject().getBuildDir().getAbsolutePath(),
this.getClass().getSimpleName(),
getName() + ".inline.sig"
);
Files.write(
inlineSignatures,
signatures.stream().collect(Collectors.joining("\n")).getBytes(StandardCharsets.UTF_8)
);

getProject().javaexec((JavaExecSpec spec) -> {
execAction.execute(spec);
// This works because forbidden apis has no transitive dependencies.
spec.classpath(ClassPathUtils.getJar(CliMain.class));
Copy link
Member

Choose a reason for hiding this comment

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

While I know we still need forbidden on the gradle classpath because we invoke it through ant for third-party-audit, I think once we move that to use cli, we should change this to have forbidden api in a separate configuration and pull it there, instead of this hack on finding where a class we have already loaded is located.

spec.setMain(CliMain.class.getName());
// build the command line
getSignaturesFiles().forEach(file -> spec.args("-f", file.getAbsolutePath()));
spec.args("-f", inlineSignatures.toAbsolutePath());
getSuppressAnnotations().forEach(annotation -> spec.args("--suppressannotation", annotation));
getBundledSignatures().forEach(bundled -> {
// there's no option for target compatibility so we have to interpret it
final String prefix;
if (bundled.equals("jdk-system-out") ||
bundled.equals("jdk-reflection") ||
bundled.equals("jdk-non-portable")) {
prefix = "";
} else {
prefix = "-" + (
getTargetCompatibility().compareTo(JavaVersion.VERSION_1_9) >= 0 ?
getTargetCompatibility().getMajorVersion() :
"1." + getTargetCompatibility().getMajorVersion())
;
}
spec.args("-b", bundled + prefix);
}
);
getClassesDirs().forEach(dir ->
spec.args("-d", dir)
);
});
Files.write(getMarkerFile().toPath(), Collections.emptyList());
}

}
Loading