[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #5618: Improve project resolution of nested subprojects for jar manifests

[GitBox <gi...@apache.org>]

pivotal-jbarrett commented on a change in pull request #5618:
URL: https://github.com/apache/geode/pull/5618#discussion_r503669622



##########
File path: gradle/publish-java.gradle
##########
@@ -34,40 +34,46 @@ gradle.taskGraph.whenReady({ graph ->
     jar.doFirst {
       def projectDependencies = []
       def runtimeList = []
+      def allProjectDeps = []
 
-      // Iterate over runtime classpath dependencies and separate project dependencies from library
-      // dependencies.
-      configurations.runtimeClasspath
-              .collect { it.name - ".jar" }
-              .each { dependency ->
-                if (dependency.startsWith("geode-")) {
-                  projectDependencies.add(dependency)
-                } else {
-                  runtimeList.add(dependency+".jar")
-                }
-              }
+      ['api', 'implementation', 'runtimeOnly'].each { conf ->
+        allProjectDeps.addAll(project.configurations.getByName(conf).getDependencies())
+      }
+
+//      // Iterate over runtime classpath dependencies and separate project dependencies from library
+//      // dependencies.
+      allProjectDeps.each { dependency ->
+        if ( dependency instanceof ProjectDependency )  {
+          projectDependencies.add(dependency)
+        } else {
+          project.configurations.runtimeClasspath.files(dependency).each { depJar ->
+            runtimeList.add(depJar.name)
+          }
+        }
+      }
 
       // Iterate over project (parent) dependencies and remove its runtime library dependencies from
       // the current project's runtime library dependencies.
       // Also removes all parent project's runtime project dependencies from the current project.
       // This returns a unique set of parent project and library dependencies that are not found
       // within it's parent's project dependencies.
       projectDependencies.clone().each { projectDependency ->
-        def geodeProject = projectDependency - "-${version}.jar"
+        def geodeProject = projectDependency.getDependencyProject()
         if (projectDependencies.contains(geodeProject)) {
           try {
-            def parentProject = project(":$geodeProject" - "-$version")
+            def parentProject = geodeProject
             def collect = parentProject.configurations.runtimeClasspath.collect { it.name }
             runtimeList.removeAll(collect)

Review comment:
       I really think we don't want to be removing these dependencies. If we discovered them in the block above as direct dependencies they should be included. Because we aren't iterating over transitive dependencies we shouldn't have any to filter out.

##########
File path: gradle/publish-java.gradle
##########
@@ -34,40 +34,46 @@ gradle.taskGraph.whenReady({ graph ->
     jar.doFirst {
       def projectDependencies = []
       def runtimeList = []
+      def allProjectDeps = []
 
-      // Iterate over runtime classpath dependencies and separate project dependencies from library
-      // dependencies.
-      configurations.runtimeClasspath
-              .collect { it.name - ".jar" }
-              .each { dependency ->
-                if (dependency.startsWith("geode-")) {
-                  projectDependencies.add(dependency)
-                } else {
-                  runtimeList.add(dependency+".jar")
-                }
-              }
+      ['api', 'implementation', 'runtimeOnly'].each { conf ->
+        allProjectDeps.addAll(project.configurations.getByName(conf).getDependencies())
+      }
+
+//      // Iterate over runtime classpath dependencies and separate project dependencies from library
+//      // dependencies.
+      allProjectDeps.each { dependency ->
+        if ( dependency instanceof ProjectDependency )  {
+          projectDependencies.add(dependency)
+        } else {
+          project.configurations.runtimeClasspath.files(dependency).each { depJar ->
+            runtimeList.add(depJar.name)
+          }
+        }
+      }
 
       // Iterate over project (parent) dependencies and remove its runtime library dependencies from
       // the current project's runtime library dependencies.
       // Also removes all parent project's runtime project dependencies from the current project.
       // This returns a unique set of parent project and library dependencies that are not found
       // within it's parent's project dependencies.
       projectDependencies.clone().each { projectDependency ->
-        def geodeProject = projectDependency - "-${version}.jar"
+        def geodeProject = projectDependency.getDependencyProject()
         if (projectDependencies.contains(geodeProject)) {
           try {
-            def parentProject = project(":$geodeProject" - "-$version")
+            def parentProject = geodeProject
             def collect = parentProject.configurations.runtimeClasspath.collect { it.name }
             runtimeList.removeAll(collect)
-            projectDependencies.removeAll(collect.collect {it-".jar"})
+//            projectDependencies.removeAll(collect.collect {it-".jar"})
           } catch (UnknownProjectException ignore) {
+            throw ignore

Review comment:
       I think that exception isn't possible there anyway. Pull it.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org