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.

Sign up for GitHub

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

[Build] make sure there are no duplicate classes in third party audit #34213

Merged
merged 3 commits into from
Oct 5, 2018
Merged

[Build] make sure there are no duplicate classes in third party audit #34213

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9c7c09b
make sure there are no duplicate classes in third party audit
alpar-t Oct 2, 2018
a109e79
Explain the jar extraction better
alpar-t Oct 4, 2018
09dd69d
Merge branch 'master' into thirdparty-audit-multi-release-jars-33759
alpar-t Oct 4, 2018
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
2 changes: 1 addition & 1 deletion buildSrc/src/main/java/org/elasticsearch/gradle/JdkJarHellCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ private void scanForJDKJarHell(Path root) throws IOException {
@Override
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
String entry = root.relativize(file).toString().replace('\\', '/');
if (entry.endsWith(".class")) {
if (entry.endsWith(".class") && entry.endsWith("module-info.class") == false) {
if (ext.getResource(entry) != null) {
detected.add(
entry
Expand Down
38 changes: 25 additions & 13 deletions buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ThirdPartyAuditTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@

import org.apache.commons.io.output.NullOutputStream;
import org.elasticsearch.gradle.JdkJarHellCheck;
import org.elasticsearch.test.NamingConventionsCheck;
import org.gradle.api.DefaultTask;
import org.gradle.api.GradleException;
import org.gradle.api.JavaVersion;
import org.gradle.api.artifacts.Configuration;
import org.gradle.api.file.FileCollection;
import org.gradle.api.file.FileTree;
import org.gradle.api.tasks.Input;
import org.gradle.api.tasks.InputFile;
import org.gradle.api.tasks.InputFiles;
Expand All @@ -47,6 +47,7 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

public class ThirdPartyAuditTask extends DefaultTask {

Expand Down Expand Up @@ -171,19 +172,30 @@ private void extractJars(FileCollection jars) {
File jarExpandDir = getJarExpandDir();
// We need to clean up to make sure old dependencies don't linger
getProject().delete(jarExpandDir);
jars.forEach(jar ->

jars.forEach(jar -> {
alpar-t marked this conversation as resolved.
Show resolved Hide resolved
FileTree jarFiles = getProject().zipTree(jar);
getProject().copy(spec -> {
spec.from(jarFiles);
spec.into(jarExpandDir);
// exclude classes from multi release jars
spec.exclude("META-INF/versions/**");
});
// The order is important, we iterate here so we don't depend on the order in which Gradle executes the spec
IntStream.rangeClosed(
Integer.parseInt(JavaVersion.VERSION_1_9.getMajorVersion()),
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be the compiler version? For example, while we don't currently, we could add java 10 or 11 specific classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

targetCompatability is being set to runtime, since we cycle trough the supported run-times in CI we will eventually test each. The goal is to extract everything from 9 ( the first to support multi release jars ) to the current runtime version, whith each extract overwriting the previously existing classes. This means that the result will be the exact classes the loader would pick when running on a particular java version.

i will also add a comment in the code to clarify this.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I see it now. Thanks for the explanation.

Integer.parseInt(targetCompatibility.getMajorVersion())
).forEach(majorVersion -> getProject().copy(spec -> {
spec.from(getProject().zipTree(jar));
spec.into(jarExpandDir);
// Exclude classes for multi release jars above target
for (int i = Integer.parseInt(targetCompatibility.getMajorVersion()) + 1;
i <= Integer.parseInt(JavaVersion.VERSION_HIGHER.getMajorVersion());
i++
) {
spec.exclude("META-INF/versions/" + i + "/**");
}
})
);
// exclude classes from multi release jars
String metaInfPrefix = "META-INF/versions/" + majorVersion;
spec.include(metaInfPrefix + "/**");
// Drop the version specific prefix
spec.eachFile(details -> details.setPath(details.getPath().replace(metaInfPrefix, "")));
spec.setIncludeEmptyDirs(false);
}));
});
}

private void assertNoJarHell(Set<String> jdkJarHellClasses) {
Expand Down Expand Up @@ -276,9 +288,9 @@ private String formatClassList(Set<String> classList) {
private Set<String> runJdkJarHellCheck() throws IOException {
ByteArrayOutputStream standardOut = new ByteArrayOutputStream();
ExecResult execResult = getProject().javaexec(spec -> {
URL location = NamingConventionsCheck.class.getProtectionDomain().getCodeSource().getLocation();
URL location = JdkJarHellCheck.class.getProtectionDomain().getCodeSource().getLocation();
if (location.getProtocol().equals("file") == false) {
throw new GradleException("Unexpected location for NamingConventionCheck class: " + location);
throw new GradleException("Unexpected location for JdkJarHellCheck class: " + location);
}
try {
spec.classpath(
Expand Down