You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2021/03/04 01:43:49 UTC
[GitHub] [bookkeeper] pkumar-singh opened a new pull request #2636: Gradle: Build bookkeeper-server with gradle and run unit tests
pkumar-singh opened a new pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636
### Motivation
Migrate bookkeeper to gradle.
### Changes
Minimum gradle files and setup to build bookeeper:server and its dependencies and run unit tests.
### How to build
From bookkeep root directory
Just to clean build the source
`/gradlew clean build -x test`
Clean Build and run unit tests
`./gradlew clean build`
Incremental build and run unit tests
`./gradlew build`
Build bookkeeper:server and run tests
`./gradlew :bookkeeper-server:test `
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r595500829
##########
File path: gradle/wrapper/gradle-wrapper.properties
##########
@@ -0,0 +1,26 @@
+#
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+#
+
+distributionBase=GRADLE_USER_HOME
+distributionPath=wrapper/dists
+distributionUrl=https\://services.gradle.org/distributions/gradle-6.0.1-all.zip
Review comment:
Please upgrade to a recent version of gradle. The distributionUrl should refer the -bin.zip to minimize download size in CI.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r595504061
##########
File path: cpu-affinity/build.gradle
##########
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+plugins {
+ id 'java'
+}
+
+dependencies {
+ compileOnly depLibs.lombok
+ compileOnly depLibs.spotbugsAnnotations
+ implementation depLibs.commonsLang3
+ implementation depLibs.guava
+ implementation depLibs.slf4j
+ testImplementation depLibs.junit
+
+ annotationProcessor depLibs.lombok
+}
+
+task generateJniHeaders(type:Exec) {
+ ext {
+ javahOutputDir = "$buildDir/javahGenerated"
+ }
+ dependsOn classes
+ def classpath = sourceSets.main.output.classesDirs.join(":")
Review comment:
This happens during configuration phase
##########
File path: circe-checksum/build.gradle
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+plugins {
+ id 'java'
+}
+
+dependencies {
+ implementation depLibs.guava
+ implementation depLibs.nettyBuffer
+ implementation depLibs.slf4j
+ testImplementation depLibs.junit
+ testImplementation depLibs.mockito
+}
+
+task generateJniHeaders(type:Exec) {
+ ext {
+ javahOutputDir = "$buildDir/javahGenerated"
+ }
+ dependsOn classes
+ def classpath = sourceSets.main.output.classesDirs.join(":")
Review comment:
This is problematic since it is done during Gradle's configuration phase.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596427162
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
Review comment:
Abosolutely.!
----------------------------------------------------------------
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
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r597073342
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
+ }
+}
+
+allprojects {
+ apply from: "$rootDir/dependencies.gradle"
+
Review comment:
@lhotari Your comments were invaluable. And I really appreciate that. Keep them coming. This will be the first commit for gradle and it's important that we start with the right set or rules and practices. Moreover, I had already updated the PR as per you suggestion.
--
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
[GitHub] [bookkeeper] aahmed-se commented on pull request #2636: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
aahmed-se commented on pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#issuecomment-790229746
Add github workflow to test gradle build.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r595860229
##########
File path: bookkeeper-common/build.gradle
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+plugins {
+ id 'java'
Review comment:
it would be good to use the java-library plugin for library projects. There's more work to use it but there will be benefits in using it, explained in https://docs.gradle.org/current/userguide/java_library_plugin.html .
----------------------------------------------------------------
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
[GitHub] [bookkeeper] eolivelli commented on pull request #2636: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#issuecomment-790447902
I forgot to add that I tested the build and it works well
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596887952
##########
File path: bookkeeper-common/build.gradle
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+plugins {
+ id 'java'
+}
+
+dependencies {
+ implementation project(":bookkeeper-stats")
+ implementation project(":cpu-affinity")
+
+ compileOnly depLibs.errorprone
+ compileOnly depLibs.jsr305
+ compileOnly depLibs.lombok
+ compileOnly depLibs.spotbugsAnnotations
+ implementation depLibs.commonsConfiguration
+ implementation depLibs.guava
+ implementation depLibs.jacksonAnnotations
+ implementation depLibs.jacksonCore
+ implementation depLibs.jacksonDatabind
+ implementation depLibs.jctools
+ implementation depLibs.nettyCommon
+ implementation depLibs.slf4j
+ testCompileOnly depLibs.lombok
+ testImplementation depLibs.commonsLang3
+ testImplementation depLibs.hamcrest
+ testImplementation depLibs.junit
+ testImplementation depLibs.log4j
+ testImplementation depLibs.mockito
+ testImplementation depLibs.slf4jLog4j
+
+ annotationProcessor depLibs.lombok
+ testAnnotationProcessor depLibs.lombok
+}
+
+configurations {
+ testArtifacts.extendsFrom testRuntime
+}
+
+task testJar(type: Jar) {
+ archiveClassifier.set('test')
+ from sourceSets.test.output
+}
+
+artifacts {
+ testArtifacts testJar
+}
Review comment:
yes, there's also some other work required when using the test fixtures plugin since I don't think that it's recommended to mix the actual tests with the fixtures. It's fine to go forward with the original solution.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r595599017
##########
File path: circe-checksum/build.gradle
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+plugins {
+ id 'java'
+}
+
+dependencies {
+ implementation depLibs.guava
+ implementation depLibs.nettyBuffer
+ implementation depLibs.slf4j
+ testImplementation depLibs.junit
+ testImplementation depLibs.mockito
+}
+
+task generateJniHeaders(type:Exec) {
+ ext {
+ javahOutputDir = "$buildDir/javahGenerated"
+ }
+ dependsOn classes
+ def classpath = sourceSets.main.output.classesDirs.join(":")
Review comment:
I am not sure I understood this, as what you mean by gradle's configuration phase, and why it is problematic.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r595889765
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
+ }
+}
+
+allprojects {
+ apply from: "$rootDir/dependencies.gradle"
+
Review comment:
The build doesn't currently specify Java source compatibility and target compatibility, which is necessary.
Specifying the common settings for all Java projects could be done here, for example:
```
pluginManager.withPlugin('java') {
sourceCompatibility = targetCompatibility = 8
}
```
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596885159
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
+ }
+}
+
+allprojects {
+ apply from: "$rootDir/dependencies.gradle"
+
Review comment:
the comments I made in the review weren't meant to block the work, but provide some feedback on what common Gradle build best practices are. Feel free to ignore my comments. :)
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#issuecomment-802226134
> It feels like a good starting point and can evolve. One general question is that I'm not seeing plugin configurations. At least In Pravega, we have them in a `gradle/` folder, see here if it helps:
>
> https://github.com/pravega/pravega/blob/master/build.gradle#L73
>
> If you could clarify what the plan is plugin configurations, I'd appreciate.
Good point.
Gradle's current recommendation is no more to use "script plugins" such as `apply from: "$rootDir/gradle/java.gradle"` to share build logic. `gradle init` in recent Gradle versions will generate examples of the current recommendation where the shared build logic is put under `buildSrc/src/main/groovy/*-conventions.gradle` files.
This is documented in https://docs.gradle.org/current/userguide/sharing_build_logic_between_subprojects.html and https://docs.gradle.org/current/samples/sample_convention_plugins.html in the Gradle manual.
An example build using Gradle Groovy DSL is in https://github.com/gradle/gradle/tree/master/subprojects/docs/src/samples/build-organization/multi-project-with-convention-plugins/groovy .
When it comes to finding a way how to define plugin configurations, I recommend using the new approach.
--
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596278983
##########
File path: settings.gradle
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+rootProject.name = 'bookkeeper'
+
+include('circe-checksum', 'circe-checksum:src:main:circe',
Review comment:
one possibility is this ([from docs](https://docs.gradle.org/current/javadoc/org/gradle/api/initialization/Settings.html#include-java.lang.String...-)):
```
include('circe-checksum-jni')
project(':circe-checksum-jni').projectDir='circe-checksum/src/main/circe'
```
However, it seems wrong to make 'circe-checksum/src/main/circe' the project directory.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] pkumar-singh commented on pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#issuecomment-800676291
> Thanks for this work !
> a few points:
>
> * The file gradle/wrapper/gradle-wrapper.jar is empty. can you remove it ?
> * There are not license file headers in the new files, can you fix the Maven build regarding the apache-rat plugin ? you have to add the license headers and/or ignore the files for which you cannot add them (like "gradlew")
- Added license headers.
- gradle/wrapper/gradle-wrapper.jar is non empty and its needed.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596455532
##########
File path: settings.gradle
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+rootProject.name = 'bookkeeper'
+
+include('circe-checksum', 'circe-checksum:src:main:circe',
Review comment:
I think we can revisit this later. Because I am sure we will encounter some more cases like that down the line.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] hsaputra commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
hsaputra commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r597288374
##########
File path: bookkeeper-stats-providers/prometheus-metrics-provider/src/test/java/org/apache/bookkeeper/stats/prometheus/TestPrometheusMetricsProvider.java
##########
@@ -70,9 +69,13 @@ public void testStartNoHttpWhenBkHttpEnabled() {
PropertiesConfiguration config = new PropertiesConfiguration();
config.setProperty(PrometheusMetricsProvider.PROMETHEUS_STATS_HTTP_ENABLE, true);
config.setProperty("httpServerEnabled", true);
- @Cleanup("stop") PrometheusMetricsProvider provider = new PrometheusMetricsProvider();
- provider.start(config);
- assertNull(provider.server);
+ PrometheusMetricsProvider provider = new PrometheusMetricsProvider();
Review comment:
@fpj - wondering if we could follow up for Gradle plugin configurations in a follow up PR?
--
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r595857408
##########
File path: settings.gradle
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+rootProject.name = 'bookkeeper'
+
+include('circe-checksum', 'circe-checksum:src:main:circe',
Review comment:
the modules `circe-checksum:src:main:circe` and `cpu-affinity:src:main:affinity` look like an odd solution. There must be cleaner ways to define these.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r595879496
##########
File path: .gitignore
##########
@@ -41,3 +41,5 @@ tools/all/src/main/resources
node_modules
package-lock.json
+# gradle
+build/
Review comment:
`.gradle/` should be added to `.gitignore`
----------------------------------------------------------------
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
[GitHub] [bookkeeper] fpj commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
fpj commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596652257
##########
File path: bookkeeper-stats-providers/prometheus-metrics-provider/src/test/java/org/apache/bookkeeper/stats/prometheus/TestPrometheusMetricsProvider.java
##########
@@ -70,9 +69,13 @@ public void testStartNoHttpWhenBkHttpEnabled() {
PropertiesConfiguration config = new PropertiesConfiguration();
config.setProperty(PrometheusMetricsProvider.PROMETHEUS_STATS_HTTP_ENABLE, true);
config.setProperty("httpServerEnabled", true);
- @Cleanup("stop") PrometheusMetricsProvider provider = new PrometheusMetricsProvider();
- provider.start(config);
- assertNull(provider.server);
+ PrometheusMetricsProvider provider = new PrometheusMetricsProvider();
Review comment:
Is this change unrelated to this PR?
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r597542119
##########
File path: cpu-affinity/build.gradle
##########
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+plugins {
+ id 'java'
+}
+
+dependencies {
+ compileOnly depLibs.lombok
+ compileOnly depLibs.spotbugsAnnotations
+ implementation depLibs.commonsLang3
+ implementation depLibs.guava
+ implementation depLibs.slf4j
+ testImplementation depLibs.junit
+
+ annotationProcessor depLibs.lombok
+}
+
+task generateJniHeaders(type:Exec) {
+ ext {
+ javahOutputDir = "$buildDir/javahGenerated"
+ }
+ dependsOn classes
+ def classpath = sourceSets.main.output.classesDirs.join(":")
Review comment:
`.join(":")` -> `.getAsPath()`
--
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
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596449418
##########
File path: settings.gradle
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+rootProject.name = 'bookkeeper'
+
+include('circe-checksum', 'circe-checksum:src:main:circe',
Review comment:
This does not work.
```
-include('circe-checksum', 'circe-checksum:src:main:circe',
+include('circe-checksum',
'cpu-affinity', 'cpu-affinity:src:main:affinity', 'bookkeeper-common',
'bookkeeper-http:http-server', 'bookkeeper-server',
'bookkeeper-common-allocator', 'bookkeeper-proto', 'bookkeeper-stats',
'bookkeeper-stats-providers:prometheus-metrics-provider',
'tools:framework')
+include(':circe-checksum-jni')
+project(':circe-checksum-jni').projectDir=file('circe-checksum/src/main/circe')
```
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r595852123
##########
File path: circe-checksum/build.gradle
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+plugins {
+ id 'java'
+}
+
+dependencies {
+ implementation depLibs.guava
+ implementation depLibs.nettyBuffer
+ implementation depLibs.slf4j
+ testImplementation depLibs.junit
+ testImplementation depLibs.mockito
+}
+
+task generateJniHeaders(type:Exec) {
+ ext {
+ javahOutputDir = "$buildDir/javahGenerated"
+ }
+ dependsOn classes
+ def classpath = sourceSets.main.output.classesDirs.join(":")
Review comment:
The way to investigate possible configuration time issues is to do a build scan for the "help" task on 2. execution:
```
./gradlew help
./gradlew help --scan
```
I created a build scan for this PR (rev a5f28e1) here: https://gradle.com/s/m7ae4q3ulill4 .
The performance->configuration shows the break down for configuration.The https://scans.gradle.com/s/m7ae4q3ulill4/performance/dependency-resolution shows that there isn't any dependency resolution happening during configuration time, so from that perspective it's fine to access `sourceSets.main.out.classesDirs` during configuration time.
The remaining issue is the possible cacheability and correctness of the build. In Gradle, it's considered a bug in the build or one of the plugins if you have to use "clean" to get correct results. The task inputs and outputs should be properly declared primarily for the correctness & reproduceability of the build, but also to be able to get more advantages such as cacheability.
##########
File path: cpu-affinity/build.gradle
##########
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+plugins {
+ id 'java'
+}
+
+dependencies {
+ compileOnly depLibs.lombok
+ compileOnly depLibs.spotbugsAnnotations
+ implementation depLibs.commonsLang3
+ implementation depLibs.guava
+ implementation depLibs.slf4j
+ testImplementation depLibs.junit
+
+ annotationProcessor depLibs.lombok
+}
+
+task generateJniHeaders(type:Exec) {
+ ext {
+ javahOutputDir = "$buildDir/javahGenerated"
+ }
+ dependsOn classes
+ def classpath = sourceSets.main.output.classesDirs.join(":")
Review comment:
This happens during configuration phase
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r595864182
##########
File path: bookkeeper-common/build.gradle
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+plugins {
+ id 'java'
+}
+
+dependencies {
+ implementation project(":bookkeeper-stats")
+ implementation project(":cpu-affinity")
+
+ compileOnly depLibs.errorprone
+ compileOnly depLibs.jsr305
+ compileOnly depLibs.lombok
+ compileOnly depLibs.spotbugsAnnotations
+ implementation depLibs.commonsConfiguration
+ implementation depLibs.guava
+ implementation depLibs.jacksonAnnotations
+ implementation depLibs.jacksonCore
+ implementation depLibs.jacksonDatabind
+ implementation depLibs.jctools
+ implementation depLibs.nettyCommon
+ implementation depLibs.slf4j
+ testCompileOnly depLibs.lombok
+ testImplementation depLibs.commonsLang3
+ testImplementation depLibs.hamcrest
+ testImplementation depLibs.junit
+ testImplementation depLibs.log4j
+ testImplementation depLibs.mockito
+ testImplementation depLibs.slf4jLog4j
+
+ annotationProcessor depLibs.lombok
+ testAnnotationProcessor depLibs.lombok
+}
+
+configurations {
+ testArtifacts.extendsFrom testRuntime
+}
+
+task testJar(type: Jar) {
+ archiveClassifier.set('test')
+ from sourceSets.test.output
+}
+
+artifacts {
+ testArtifacts testJar
+}
Review comment:
Since Gradle 5.6, there has been java-test-fixtures plugin to achieve this, explained in https://stackoverflow.com/a/60138176 and https://docs.gradle.org/current/userguide/java_testing.html#sec:java_test_fixtures . It requires the usage of "java-library" plugin too.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r595854563
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
Review comment:
0.8.15 is the most recent version of this plugin. I'd assume that it would be good to start with the most recent version.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r597541188
##########
File path: circe-checksum/build.gradle
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+plugins {
+ id 'java'
+}
+
+dependencies {
+ implementation depLibs.guava
+ implementation depLibs.nettyBuffer
+ implementation depLibs.slf4j
+ testImplementation depLibs.junit
+ testImplementation depLibs.mockito
+}
+
+task generateJniHeaders(type:Exec) {
+ ext {
+ javahOutputDir = "$buildDir/javahGenerated"
+ }
+ dependsOn classes
+ def classpath = sourceSets.main.output.classesDirs.join(":")
Review comment:
does this work on Windows OS? `sourceSets.main.output.classesDirs.getAsPath()` would use `File.pathSeparator` and work for Windows.
--
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r597546860
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
+ }
+}
+
+allprojects {
+ apply from: "$rootDir/dependencies.gradle"
+
Review comment:
yes it's fine to get this PR merged and gradually improve the Gradle build. The current build does have issues with up-to-date checks since all tasks don't properly declare inputs and outputs. This is the reason why "gradle clean" will be occasionally needed. In that sense this build has bugs. Once the build is merged, it's easier for me to contribute fixes for this.
--
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r597546860
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
+ }
+}
+
+allprojects {
+ apply from: "$rootDir/dependencies.gradle"
+
Review comment:
yes it's fine to get this PR merged and gradually improve the Gradle build. The current build does have issues with up-to-date checks since all tasks don't properly declare inputs and outputs. This is the reason why "gradle clean" will be occasionally needed. In that sense this build has bugs. In Gradle, it's always a bug in the build or a plugin if there's a need to use "clean". I believe this is the same for other incremental build systems. Once the build is merged, it's easier for me to contribute fixes for this.
--
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
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596453166
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
+ }
+}
+
+allprojects {
+ apply from: "$rootDir/dependencies.gradle"
+
Review comment:
If that's the conventional wisdom then I will accept this.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596290833
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
+ }
+}
+
+allprojects {
+ apply from: "$rootDir/dependencies.gradle"
+
Review comment:
That's not a very common solution to let it float. Usually a project sets the sourceCompatibilty and targetCompatibility to a specific version. In a OSS library project like BookKeeper, I don't think it's a good idea to not specify it.
Even if you set sourceCompatibility and targetCompatibility to Java 8, there will be a benefit in using Java 11.
Since Gradle 6.6 there is also support for passing the "release" flag to the compiler, docs in https://docs.gradle.org/6.6/release-notes.html#javacompile-release .
Using the release flag is necessary when building on newer JVMs and targeting Java 8 for preventing issues such as https://github.com/apache/pulsar/issues/8445 (`NoSuchMethodError: java.nio.ByteBuffer.rewind()Ljava/nio/ByteBuffer;`).
If there's a need to easily change the version for custom forks etc, the logic could be made configurable/conditional with a `gradle.properties` property.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r595497191
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
+ }
+}
+
+allprojects {
+ apply from: "$rootDir/dependencies.gradle"
+
+ repositories {
+ jcenter()
Review comment:
jcenter is shutting down. It shouldn't be used anymore. https://blog.gradle.org/jcenter-shutdown for more info
----------------------------------------------------------------
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
[GitHub] [bookkeeper] hsaputra commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
hsaputra commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r597298213
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
+ }
+}
+
+allprojects {
+ apply from: "$rootDir/dependencies.gradle"
+
Review comment:
Thanks, @lhotari , and as @pkumar-singh said, there can and will be follow ups to make the Gradle solution better.
This PR will be the "bootstrapping" effort for us to add the improvements to keep us moving forward.
--
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
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r597840972
##########
File path: circe-checksum/build.gradle
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+plugins {
+ id 'java'
+}
+
+dependencies {
+ implementation depLibs.guava
+ implementation depLibs.nettyBuffer
+ implementation depLibs.slf4j
+ testImplementation depLibs.junit
+ testImplementation depLibs.mockito
+}
+
+task generateJniHeaders(type:Exec) {
+ ext {
+ javahOutputDir = "$buildDir/javahGenerated"
+ }
+ dependsOn classes
+ def classpath = sourceSets.main.output.classesDirs.join(":")
Review comment:
Sure I will update that.
I think it still work on windows for the fact that there is only one dir.
--
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
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596449418
##########
File path: settings.gradle
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+rootProject.name = 'bookkeeper'
+
+include('circe-checksum', 'circe-checksum:src:main:circe',
Review comment:
This does not work. -include('circe-checksum', 'circe-checksum:src:main:circe',
+include('circe-checksum',
'cpu-affinity', 'cpu-affinity:src:main:affinity', 'bookkeeper-common',
'bookkeeper-http:http-server', 'bookkeeper-server',
'bookkeeper-common-allocator', 'bookkeeper-proto', 'bookkeeper-stats',
'bookkeeper-stats-providers:prometheus-metrics-provider',
'tools:framework')
+include(':circe-checksum-jni')
+project(':circe-checksum-jni').projectDir=file('circe-checksum/src/main/circe')
----------------------------------------------------------------
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
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596255397
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
+ }
+}
+
+allprojects {
+ apply from: "$rootDir/dependencies.gradle"
+
Review comment:
I would probably not vote for tying to any version and use as default. Which essentially means
- sourceCompatibility will be the version of JVM.
- targetCompatibility will be same as sourceCompatibility
Therefore If anybody wants certain build and source version adjust the JVM version.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r597121919
##########
File path: bookkeeper-stats-providers/prometheus-metrics-provider/src/test/java/org/apache/bookkeeper/stats/prometheus/TestPrometheusMetricsProvider.java
##########
@@ -70,9 +69,13 @@ public void testStartNoHttpWhenBkHttpEnabled() {
PropertiesConfiguration config = new PropertiesConfiguration();
config.setProperty(PrometheusMetricsProvider.PROMETHEUS_STATS_HTTP_ENABLE, true);
config.setProperty("httpServerEnabled", true);
- @Cleanup("stop") PrometheusMetricsProvider provider = new PrometheusMetricsProvider();
- provider.start(config);
- assertNull(provider.server);
+ PrometheusMetricsProvider provider = new PrometheusMetricsProvider();
Review comment:
So far pulgin configuration is not so evolved. If we need to configure plugin we can do similar to what is being done in Pravega.
--
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r595867340
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
Review comment:
In most recent Gradle guidelines, it's recommended to use the "Plugin Version Management" feature to centrally manage the plugin versions, please check https://docs.gradle.org/current/userguide/plugins.html#sec:plugin_version_management .
This configuration goes to `settings.gradle` file.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r597073342
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
+ }
+}
+
+allprojects {
+ apply from: "$rootDir/dependencies.gradle"
+
Review comment:
@lhotari Your comments were invaluable. And I really appreciate that. Keep them coming. This will be the first commit for gradle and it's important that we start with the right set or rules and practices.
--
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
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r597092854
##########
File path: bookkeeper-stats-providers/prometheus-metrics-provider/src/test/java/org/apache/bookkeeper/stats/prometheus/TestPrometheusMetricsProvider.java
##########
@@ -70,9 +69,13 @@ public void testStartNoHttpWhenBkHttpEnabled() {
PropertiesConfiguration config = new PropertiesConfiguration();
config.setProperty(PrometheusMetricsProvider.PROMETHEUS_STATS_HTTP_ENABLE, true);
config.setProperty("httpServerEnabled", true);
- @Cleanup("stop") PrometheusMetricsProvider provider = new PrometheusMetricsProvider();
- provider.start(config);
- assertNull(provider.server);
+ PrometheusMetricsProvider provider = new PrometheusMetricsProvider();
Review comment:
Well, It was actually related to PR. Because lombok was being used just once in the entire test module so initial thought was instead of pulling lombok as test dependency, test can be modified not to use it. But I think that was a bad idea. I am going to undo this change and pull lombok in the test dependency.
--
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596290833
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
+ }
+}
+
+allprojects {
+ apply from: "$rootDir/dependencies.gradle"
+
Review comment:
That's not a very common solution to let it float. Usually a project sets the sourceCompatibilty and targetCompatibility to a specific version. In a OSS library project like BookKeeper, I don't think it's a good idea to not specify it.
Even if you set sourceCompatibility and targetCompatibility to Java 8, there will be a benefit in using Java 11.
Since Gradle 6.6 there is also support for passing the "release" flag to the compiler, docs in https://docs.gradle.org/6.6/release-notes.html#javacompile-release .
Using the release flag is necessary when building on newer JVMs and targeting Java 8 for preventing issues such as https://github.com/apache/pulsar/issues/8445 .
If there's a need to easily change the version for custom forks etc, the logic could be made configurable/conditional with a `gradle.properties` property.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596881452
##########
File path: settings.gradle
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+rootProject.name = 'bookkeeper'
+
+include('circe-checksum', 'circe-checksum:src:main:circe',
Review comment:
yes, it can be revisited later.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596278983
##########
File path: settings.gradle
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+rootProject.name = 'bookkeeper'
+
+include('circe-checksum', 'circe-checksum:src:main:circe',
Review comment:
one possibility is this:
```
include('circe-checksum-jni')
project('circe-checksum-jni').projectDir='circe-checksum/src/main/circe'
```
However, it seems wrong to make 'circe-checksum/src/main/circe' the project directory.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596255397
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
+ }
+}
+
+allprojects {
+ apply from: "$rootDir/dependencies.gradle"
+
Review comment:
I would probably not vote for tying to any version and rather use as default. Which essentially means
- sourceCompatibility will be the version of JVM.
- targetCompatibility will be same as sourceCompatibility
Therefore If anybody wants certain build and source version adjust the JVM version.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] merlimat merged pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636
--
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r597546860
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
+ }
+}
+
+allprojects {
+ apply from: "$rootDir/dependencies.gradle"
+
Review comment:
yes it's fine to get this PR merged and gradually improve the Gradle build. The current build does have issues with up-to-date checks since all tasks don't properly declare inputs and outputs. This is the reason why "gradle clean" will be occasionally needed. In that sense this build has bugs. In Gradle, it's always a bug in the build or a plugin if there's a need to use "clean". Once the build is merged, it's easier for me to contribute fixes for this.
--
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
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r595604537
##########
File path: build.gradle
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+buildscript {
+ repositories {
+ mavenCentral()
+ }
+ dependencies {
+ classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.10'
+ }
+}
+
+allprojects {
+ apply from: "$rootDir/dependencies.gradle"
+
+ repositories {
+ jcenter()
Review comment:
Done.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] fpj commented on pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
fpj commented on pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#issuecomment-802762304
Thanks for the pointers, @lhotari. I agree that following the current guidelines makes sense.
--
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
[GitHub] [bookkeeper] pkumar-singh commented on pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#issuecomment-801258994
> LGTM
>
> I have checked other Apache projects, like Calcite, and they are storing gradle-wapper.jar as well
>
> so I am +1 with keeping it.
Yeah. Same reason I also arrived at.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] lhotari commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r595842848
##########
File path: circe-checksum/build.gradle
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+plugins {
+ id 'java'
+}
+
+dependencies {
+ implementation depLibs.guava
+ implementation depLibs.nettyBuffer
+ implementation depLibs.slf4j
+ testImplementation depLibs.junit
+ testImplementation depLibs.mockito
+}
+
+task generateJniHeaders(type:Exec) {
+ ext {
+ javahOutputDir = "$buildDir/javahGenerated"
+ }
+ dependsOn classes
+ def classpath = sourceSets.main.output.classesDirs.join(":")
Review comment:
It mainly impacts the performance of the build, explained in https://docs.gradle.org/current/userguide/performance.html#configuration . However it's possible that in case there isn't a real performance implication when accessing `sourceSets.main.out.classesDirs`. If it triggers dependency resolution, it would be an actual problem.
Another performance related reason for specifying the Gradle tasks in a certain way is to make the build compatible with Gradle Build Cache. That aspect is covered in https://docs.gradle.org/current/userguide/build_cache.html#sec:task_output_caching_inputs . Using Gradle Build Cache doesn't require Gradle Enterprise. For GitHub Actions CI builds, It's possible to use the Gradle remote build cache with the GitHub Actions Cache as the cache backend with https://github.com/cirruslabs/http-cache-action .
----------------------------------------------------------------
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
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2636: ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2636:
URL: https://github.com/apache/bookkeeper/pull/2636#discussion_r596305007
##########
File path: bookkeeper-common/build.gradle
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+plugins {
+ id 'java'
+}
+
+dependencies {
+ implementation project(":bookkeeper-stats")
+ implementation project(":cpu-affinity")
+
+ compileOnly depLibs.errorprone
+ compileOnly depLibs.jsr305
+ compileOnly depLibs.lombok
+ compileOnly depLibs.spotbugsAnnotations
+ implementation depLibs.commonsConfiguration
+ implementation depLibs.guava
+ implementation depLibs.jacksonAnnotations
+ implementation depLibs.jacksonCore
+ implementation depLibs.jacksonDatabind
+ implementation depLibs.jctools
+ implementation depLibs.nettyCommon
+ implementation depLibs.slf4j
+ testCompileOnly depLibs.lombok
+ testImplementation depLibs.commonsLang3
+ testImplementation depLibs.hamcrest
+ testImplementation depLibs.junit
+ testImplementation depLibs.log4j
+ testImplementation depLibs.mockito
+ testImplementation depLibs.slf4jLog4j
+
+ annotationProcessor depLibs.lombok
+ testAnnotationProcessor depLibs.lombok
+}
+
+configurations {
+ testArtifacts.extendsFrom testRuntime
+}
+
+task testJar(type: Jar) {
+ archiveClassifier.set('test')
+ from sourceSets.test.output
+}
+
+artifacts {
+ testArtifacts testJar
+}
Review comment:
I think for this to work exposed test code have to be in bookkeeper-common/src/testFixtures directory, which is not the case. Correct me if I am wrong.
And I can not ready to move them at least now may be later.
----------------------------------------------------------------
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