-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Build: Rework shadow plugin configuration #32409
Changes from 9 commits
cae6b5c
7f89181
831d6db
9d30c49
c2c64c5
293cc2a
59e263b
4ea85ac
1202dbe
10195a0
b8ad995
d5be929
e30b8a7
e6a4ea4
03f2f92
744e77f
dd1c1cc
fb0ba83
ea6f915
1d2d77d
8581519
d7479de
5dab259
c9e6230
f924f2c
1b6614d
b3a0cb7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ import org.elasticsearch.gradle.LoggedExec | |
import org.elasticsearch.gradle.Version | ||
import org.elasticsearch.gradle.VersionCollection | ||
import org.elasticsearch.gradle.VersionProperties | ||
import org.elasticsearch.gradle.plugin.PluginBuildPlugin | ||
import org.gradle.plugins.ide.eclipse.model.SourceFolder | ||
import org.gradle.util.GradleVersion | ||
import org.gradle.util.DistributionLocator | ||
|
@@ -296,7 +297,7 @@ subprojects { | |
// org.elasticsearch:elasticsearch must be the last one or all the links for the | ||
// other packages (e.g org.elasticsearch.client) will point to server rather than | ||
// their own artifacts. | ||
if (project.plugins.hasPlugin(BuildPlugin)) { | ||
if (project.plugins.hasPlugin(BuildPlugin) || project.plugins.hasPlugin(PluginBuildPlugin)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PluginBuildPlugin extends BuildPlugin, maybe it should apply it instead? Then we only need the one check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that'd be cleaner but I didn't want to do it while messing around with shadow stuff. |
||
String artifactsHost = VersionProperties.elasticsearch.isSnapshot() ? "https://snapshots.elastic.co" : "https://artifacts.elastic.co" | ||
Closure sortClosure = { a, b -> b.group <=> a.group } | ||
Closure depJavadocClosure = { shadowed, dep -> | ||
|
@@ -314,13 +315,6 @@ subprojects { | |
*/ | ||
project.evaluationDependsOn(upstreamProject.path) | ||
project.javadoc.source += upstreamProject.javadoc.source | ||
/* | ||
* Do not add those projects to the javadoc classpath because | ||
* we are going to resolve them with their source instead. | ||
*/ | ||
project.javadoc.classpath = project.javadoc.classpath.filter { f -> | ||
false == upstreamProject.configurations.archives.artifacts.files.files.contains(f) | ||
} | ||
/* | ||
* Instead we need the upstream project's javadoc classpath so | ||
* we don't barf on the classes that it references. | ||
|
@@ -337,16 +331,16 @@ subprojects { | |
project.configurations.compile.dependencies | ||
.findAll() | ||
.toSorted(sortClosure) | ||
.each({ c -> depJavadocClosure(hasShadow, c) }) | ||
.each({ c -> depJavadocClosure(false, c) }) | ||
project.configurations.compileOnly.dependencies | ||
.findAll() | ||
.toSorted(sortClosure) | ||
.each({ c -> depJavadocClosure(hasShadow, c) }) | ||
.each({ c -> depJavadocClosure(false, c) }) | ||
if (hasShadow) { | ||
project.configurations.shadow.dependencies | ||
project.configurations.bundle.dependencies | ||
.findAll() | ||
.toSorted(sortClosure) | ||
.each({ c -> depJavadocClosure(false, c) }) | ||
.each({ c -> depJavadocClosure(true, c) }) | ||
} | ||
} | ||
} | ||
|
@@ -525,15 +519,11 @@ allprojects { | |
* Without this they won't properly depend on the shadowed project. | ||
*/ | ||
if (isEclipse || isIdea) { | ||
configurations.all { Configuration configuration -> | ||
dependencies.all { Dependency dep -> | ||
if (dep instanceof ProjectDependency) { | ||
if (dep.getTargetConfiguration() == 'shadow') { | ||
configuration.dependencies.add(project.dependencies.project(path: dep.dependencyProject.path, configuration: 'runtime')) | ||
} | ||
} | ||
} | ||
} | ||
project.plugins.withType(ShadowPlugin).whenPluginAdded { | ||
project.afterEvaluate { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be done when the bundle configuration is added, in BuildPlugin, instead of in an afterEvaluate? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BuildPlugin doesn't have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Maybe it should? It seems odd to have to do this extra work, it should really be part of the base setup. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll use it if it is defined. |
||
project.configurations.compile.extendsFrom project.configurations.bundle | ||
} | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ | |
|
||
package org.elasticsearch.gradle.precommit | ||
|
||
import com.github.jengelman.gradle.plugins.shadow.ShadowPlugin | ||
import org.elasticsearch.gradle.LoggedExec | ||
import org.gradle.api.file.FileCollection | ||
import org.gradle.api.tasks.OutputFile | ||
|
@@ -39,6 +40,9 @@ public class JarHellTask extends LoggedExec { | |
public JarHellTask() { | ||
project.afterEvaluate { | ||
FileCollection classpath = project.sourceSets.test.runtimeClasspath | ||
if (project.plugins.hasPlugin(ShadowPlugin)) { | ||
classpath += project.configurations.bundle | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
inputs.files(classpath) | ||
dependsOn(classpath) | ||
description = "Runs CheckJarHell on ${classpath}" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,20 +37,26 @@ mainClassName = 'org.elasticsearch.client.benchmark.BenchmarkMain' | |
test.enabled = false | ||
|
||
dependencies { | ||
compile 'org.apache.commons:commons-math3:3.2' | ||
bundle 'org.apache.commons:commons-math3:3.2' | ||
|
||
compile("org.elasticsearch.client:elasticsearch-rest-client:${version}") | ||
bundle("org.elasticsearch.client:elasticsearch-rest-client:${version}") | ||
// bottleneck should be the client, not Elasticsearch | ||
compile project(path: ':client:client-benchmark-noop-api-plugin') | ||
bundle project(path: ':client:client-benchmark-noop-api-plugin') | ||
// for transport client | ||
compile("org.elasticsearch:elasticsearch:${version}") | ||
compile("org.elasticsearch.client:transport:${version}") | ||
compile project(path: ':modules:transport-netty4', configuration: 'runtime') | ||
compile project(path: ':modules:reindex', configuration: 'runtime') | ||
compile project(path: ':modules:lang-mustache', configuration: 'runtime') | ||
compile project(path: ':modules:percolator', configuration: 'runtime') | ||
bundle("org.elasticsearch:elasticsearch:${version}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, why do benchmark jars for testing need bundled deps? |
||
bundle("org.elasticsearch.client:transport:${version}") | ||
bundle project(path: ':modules:transport-netty4', configuration: 'runtime') | ||
bundle project(path: ':modules:reindex', configuration: 'runtime') | ||
bundle project(path: ':modules:lang-mustache', configuration: 'runtime') | ||
bundle project(path: ':modules:percolator', configuration: 'runtime') | ||
} | ||
|
||
// No licenses for our benchmark deps (we don't ship benchmarks) | ||
dependencyLicenses.enabled = false | ||
dependenciesInfo.enabled = false | ||
|
||
/* | ||
* Javadocs are a waste of time because we try to make them produce | ||
* nice output with shading. | ||
*/ | ||
javadoc.enabled = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we need to bundle anything in benchmarks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no clue. It has been shadowing all of its dependencies for ages and I was keeping it going. I could dig into it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I didn't realize that. Quite odd, maybe @danielmitterdorfer can elaborate on why we do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an area where we can improve a lot: commenting the non-obvious choices in our code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's for convenience, so that you can run the benchmarks as simply as
java -jar benchmarks.jar <class under benchmark>
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done a little digging and I also think it is for convenience. Every example I see of using JMH shades the dependencies but this says that it is not required.