Skip to content

Detect and prevent configuration that triggers a Gradle bug #31912

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

Merged
merged 4 commits into from
Jul 19, 2018
Merged

Detect and prevent configuration that triggers a Gradle bug #31912

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5540af5
Detect and prevent configuration that triggers a Gradle bug
alpar-t Jul 10, 2018
4e7dfd6
PR review
alpar-t Jul 17, 2018
d1bc8b9
Merge remote-tracking branch 'origin/master' into prevent_gradle_bug
alpar-t Jul 18, 2018
39d1034
PR review
alpar-t Jul 18, 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
18 changes: 17 additions & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ subprojects {
"org.elasticsearch.gradle:build-tools:${version}": ':build-tools',
"org.elasticsearch:rest-api-spec:${version}": ':rest-api-spec',
"org.elasticsearch:elasticsearch:${version}": ':server',
"org.elasticsearch:elasticsearch-cli:${version}": ':libs:cli',
"org.elasticsearch:elasticsearch-cli:${version}": ':libs:elasticsearch-cli',
"org.elasticsearch:elasticsearch-core:${version}": ':libs:core',
"org.elasticsearch:elasticsearch-nio:${version}": ':libs:nio',
"org.elasticsearch:elasticsearch-x-content:${version}": ':libs:x-content',
Expand Down Expand Up @@ -579,6 +579,22 @@ gradle.projectsEvaluated {
}
}
}
project.allprojects.forEach { outer ->
project.allprojects.forEach { inner ->
Copy link
Member

Choose a reason for hiding this comment

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

This is O(N^2)....Instead we can have one loop over the projects and build a map with the group/name as the key. Also, I think this needs to be in a projectsEvaluated or it won't capture when the group is changed during configuration, since this code is running during root configuration?

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 is already in a projectsEvaluated, but I added it to an existing one thus it doesn't show in the diff.
We wrap this individually for better visibility.

I'm not convinces the impact in performance will be noticeable, as the number of projects is not arbitrarily large nor can it grow significantly. I think the gain in readability is worth it.

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 think this is more readable. I had to stare at this a while to understand what was going on. A single loop over all the projects, collecting them by their group/name seems much more transparent to me:

Map coordsToProject = []
project.allprojects.forEach { p ->
  String coords = "${p.group}:${p.name}"
  if (false == coords.putIfAbsent(coords, p)) {
    throw new GradleException(a message)
  }
}

I would also add a comment explaining the problem and linking to the gradle bug before the loop.

if (inner.path == outer.path) {
// same project, do nothing
return
}
if (inner.name == outer.name && inner.group == outer.group) {
throw new GradleException(
"Detected that two projects: ${inner.path} and ${outer.path} " +
"have the same name and group: ${inner.group}:${inner.name}. " +
"This doesn't currently work correctly in Gradle, see: " +
"https://github.com/gradle/gradle/issues/847"
)
}
}
}
}

if (System.properties.get("build.compare") != null) {
Expand Down
2 changes: 2 additions & 0 deletions client/test/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ apply plugin: 'elasticsearch.build'
targetCompatibility = JavaVersion.VERSION_1_7
sourceCompatibility = JavaVersion.VERSION_1_7

group = "${group}.client.test"

dependencies {
compile "org.apache.httpcomponents:httpcore:${versions.httpcore}"
compile "com.carrotsearch.randomizedtesting:randomizedtesting-runner:${versions.randomizedrunner}"
Expand Down
10 changes: 0 additions & 10 deletions libs/cli/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,6 @@ apply plugin: 'nebula.optional-base'
apply plugin: 'nebula.maven-base-publish'
apply plugin: 'nebula.maven-scm'

publishing {
publications {
nebula {
artifactId 'elasticsearch-cli'
}
}
}

archivesBaseName = 'elasticsearch-cli'

dependencies {
compile 'net.sf.jopt-simple:jopt-simple:5.0.2'
compile "org.elasticsearch:elasticsearch-core:${version}"
Expand Down
1 change: 1 addition & 0 deletions plugins/discovery-ec2/qa/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
group = "${group}.plugins.discovery-ec2.qa"
1 change: 1 addition & 0 deletions plugins/repository-azure/qa/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
group = "${group}.plugins.repository-azure.qa"
1 change: 1 addition & 0 deletions plugins/repository-gcs/qa/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
group = "${group}.plugins.repository-gcs.qa"
2 changes: 2 additions & 0 deletions settings.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,3 +131,5 @@ if (extraProjects.exists()) {

// enable in preparation for Gradle 5.0
enableFeaturePreview('STABLE_PUBLISHING')

project(":libs:cli").name = 'elasticsearch-cli'
1 change: 1 addition & 0 deletions x-pack/qa/rolling-upgrade/with-system-key/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
group = "${group}.x-pack.qa.rolling-upgrade.with-system-key"
1 change: 1 addition & 0 deletions x-pack/qa/rolling-upgrade/without-system-key/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
group = "${group}.x-pack.qa.rolling-upgrade.without-system-key"
2 changes: 2 additions & 0 deletions x-pack/qa/sql/security/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ dependencies {

Project mainProject = project

group = "${group}.x-pack.qa.sql.security"

subprojects {
// Use resources from the parent project in subprojects
sourceSets {
Expand Down