You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/04/16 06:18:40 UTC

[GitHub] [geode] rhoughton-pivot opened a new pull request, #7600: Remove problematic buildSrc project, replacing with includeBuild projects

rhoughton-pivot opened a new pull request, #7600:
URL: https://github.com/apache/geode/pull/7600

   Gradle team recommends using includeBuilds for all build logic, instead of the special-case buildSrc project. To accomplish this, I also moved the `gradle` scripts from `<root>/gradle` into a `buildScripts` pre-compiled plugin project. 
   
   <!-- Thank you for submitting a contribution to Apache Geode. -->
   
   <!-- In order to streamline the review of the contribution we ask you
   to ensure the following steps have been taken: 
   -->
   
   ### For all changes:
   - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
   
   - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
   
   - [ ] Is your initial contribution a single, squashed commit?
   
   - [ ] Does `gradlew build` run cleanly?
   
   - [ ] Have you written or updated unit tests to verify your changes?
   
   - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
   
   <!-- Note:
   Please ensure that once the PR is submitted, check Concourse for build issues and
   submit an update to your PR as soon as possible. If you need help, please send an
   email to dev@geode.apache.org.
   -->
   


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

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

[GitHub] [geode] rhoughton-pivot commented on pull request #7600: Remove problematic buildSrc project, replacing with includeBuild projects

Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on PR #7600:
URL: https://github.com/apache/geode/pull/7600#issuecomment-1117852654

   UpgradeTest failure is due to the parallelism being too close to the memory ceiling of the test VM. The PR scales that number down 48->42, while we think on a better parallelism answer (3 hours is too long).


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

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

[GitHub] [geode] rhoughton-pivot commented on a diff in pull request #7600: Remove problematic buildSrc project, replacing with includeBuild projects

Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on code in PR #7600:
URL: https://github.com/apache/geode/pull/7600#discussion_r868260864


##########
build.gradle:
##########
@@ -29,16 +29,21 @@ plugins {
   id "org.ajoberstar.grgit" version "4.1.1" apply false
   id "org.nosphere.apache.rat" version "0.7.1" apply false
   id "org.sonarqube" version "3.3" apply false
-  id "me.champeau.gradle.japicmp" apply false // Version defined in buildSrc/build.gradle
   id 'me.champeau.gradle.jmh' version '0.5.3' apply false
   id "de.undercouch.download" version "5.0.1" apply false
+  id 'org.apache.geode.gradle.geode-dependency-constraints' apply false
+  id 'geode-publish-artifacts' apply false
+  id 'geode-publish-common' apply false
+  id 'geode-publish-java' apply false
+  id 'geode-publish-war' apply false
+//  id 'lint'

Review Comment:
   We can delete the linter line. I have hopes of the nebula-lint plugin working for Geode, and so left it here (transplanted from the old `apply` declaration below) in hopes of utilizing 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.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

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

[GitHub] [geode] mhansonp commented on a diff in pull request #7600: GEODE-10283: Remove problematic buildSrc project, replacing with includeBuild projects

Posted by GitBox <gi...@apache.org>.
mhansonp commented on code in PR #7600:
URL: https://github.com/apache/geode/pull/7600#discussion_r870509656


##########
build-tools/geode-testing-isolation/src/test/java/org/apache/geode/gradle/test/isolation/PortRangeTest.java:
##########
@@ -16,31 +16,28 @@
 package org.apache.geode.gradle.test.isolation;
 
 import static java.util.stream.Collectors.toList;
-import static org.junit.Assert.assertTrue;
 
 import java.util.List;
 import java.util.stream.IntStream;
 
-import org.junit.Test;
-
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-
 import org.apache.geode.gradle.testing.isolation.PortRange;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
 
 public class PortRangeTest {
   @Test
   public void firstPartitionHasSameLowerBoundAsFullRange() {
     PortRange fullRange = new PortRange(11111, 19994); // Arbitrary
     PortRange firstPartition = fullRange.partition(0, 24);
-    assertEquals(fullRange.lowerBound(), firstPartition.lowerBound());
+    Assertions.assertEquals(fullRange.lowerBound(), firstPartition.lowerBound());

Review Comment:
   Please use assertThat. We are no longer using assertEquals.



##########
build-tools/scripts/src/main/groovy/code-analysis.gradle:
##########
@@ -15,61 +15,56 @@
  * limitations under the License.
  */
 
-if (project.hasProperty("staticAnalysis")) {
-  apply plugin: 'checkstyle'
+plugins {
+  // Findbugs has been removed as of Gradle 6. Spotbugs is an alternative, if anyone wants to enable it.
+//  id 'com.github.spotbugs'
+  id 'jacoco'
+}
 
-  //Checkstyle configuration
-  configurations.checkstyle {
-    dependencies.all { dep ->
-      dep.transitive = true
+// JaCoCo configuration
+def jacocoOff = {
+  it.configure {
+    jacoco {
+      enabled = false
     }
   }
-
-  //Findbugs configuration
-  apply plugin: 'findbugs'

Review Comment:
   Does JaCoCo take care of the xml and html review now in a way it didn't before?



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

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

[GitHub] [geode] jdeppe-pivotal commented on a diff in pull request #7600: Remove problematic buildSrc project, replacing with includeBuild projects

Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on code in PR #7600:
URL: https://github.com/apache/geode/pull/7600#discussion_r868260710


##########
buildScripts/src/main/groovy/code-analysis.gradle:
##########
@@ -15,61 +15,85 @@
  * limitations under the License.
  */
 
-if (project.hasProperty("staticAnalysis")) {
-  apply plugin: 'checkstyle'
-
-  //Checkstyle configuration
-  configurations.checkstyle {
-    dependencies.all { dep ->
-      dep.transitive = true
-    }
-  }
-
-  //Findbugs configuration
-  apply plugin: 'findbugs'
+plugins {
+//  id 'com.github.spotbugs'
+  id 'jacoco'
+}
 
-  // Switch default Findbugs report to HTML for developers
-  def findbugsXmlEnabled = false
-  def findbugsHtmlEnabled = true
+if (project.hasProperty("staticAnalysis")) {
 
-  // Provide ability to change report type to XML for ingesting into other ap
-  if (project.hasProperty("findbugsXmlReport")) {
-    findbugsXmlEnabled = true
-    findbugsHtmlEnabled = false
-  }
+//  //Findbugs configuration
+//
+//  // Switch default Findbugs report to HTML for developers
+//  def findbugsXmlEnabled = false
+//  def findbugsHtmlEnabled = true
+//
+//  // Provide ability to change report type to XML for ingesting into other ap
+//  if (project.hasProperty("findbugsXmlReport")) {
+//    findbugsXmlEnabled = true
+//    findbugsHtmlEnabled = false
+//  }
+//
+//  configurations.findbugs {
+//    dependencies.all { dep ->
+//      dep.transitive = true
+//    }
+//    findbugs.effort = 'max'
+//    findbugs.reportLevel = 'low'
+//  }
+//
+//  tasks.withType(FindBugs) {
+//    reports {
+//      xml.enabled = findbugsXmlEnabled
+//      html.enabled = findbugsHtmlEnabled
+//    }
+//  }
+}

Review Comment:
   Can this block of comments be removed?



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

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

[GitHub] [geode] rhoughton-pivot merged pull request #7600: GEODE-10283: Remove problematic buildSrc project, replacing with includeBuild projects

Posted by GitBox <gi...@apache.org>.
rhoughton-pivot merged PR #7600:
URL: https://github.com/apache/geode/pull/7600


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

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

[GitHub] [geode] jdeppe-pivotal commented on a diff in pull request #7600: Remove problematic buildSrc project, replacing with includeBuild projects

Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on code in PR #7600:
URL: https://github.com/apache/geode/pull/7600#discussion_r868259385


##########
build.gradle:
##########
@@ -29,16 +29,21 @@ plugins {
   id "org.ajoberstar.grgit" version "4.1.1" apply false
   id "org.nosphere.apache.rat" version "0.7.1" apply false
   id "org.sonarqube" version "3.3" apply false
-  id "me.champeau.gradle.japicmp" apply false // Version defined in buildSrc/build.gradle
   id 'me.champeau.gradle.jmh' version '0.5.3' apply false
   id "de.undercouch.download" version "5.0.1" apply false
+  id 'org.apache.geode.gradle.geode-dependency-constraints' apply false
+  id 'geode-publish-artifacts' apply false
+  id 'geode-publish-common' apply false
+  id 'geode-publish-java' apply false
+  id 'geode-publish-war' apply false
+//  id 'lint'

Review Comment:
   Can this be deleted?



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

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