ISSUE-2640: BP-43: Gradle: Build bookkeeper-server with gradle and run unit tests

[pkumar-singh]

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

[aahmed-se]

Add github workflow to test gradle build.

[eolivelli]

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")

[eolivelli]

I forgot to add that I tested the build and it works well

[pkumar-singh]

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.

[eolivelli]

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.

[lhotari]

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.

[lhotari]

one possibility is this (from docs):

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.

[pkumar-singh]

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')

[pkumar-singh]

I think we can revisit this later. Because I am sure we will encounter some more cases like that down the line.

[lhotari]

yes, it can be revisited later.

[lhotari]

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
    }

[pkumar-singh]

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.

[lhotari]

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 apache/pulsar#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.

[pkumar-singh]

If that's the conventional wisdom then I will accept this.

[lhotari]

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

[pkumar-singh]

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

[hsaputra]

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.

[lhotari]

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.

[pkumar-singh]

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.

[fpj]

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.

[lhotari]

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.

[lhotari]

does this work on Windows OS? sourceSets.main.output.classesDirs.getAsPath() would use File.pathSeparator and work for Windows.

[pkumar-singh]

Sure I will update that.
I think it still work on windows for the fact that there is only one dir.

[lhotari]

.join(":") -> .getAsPath()

[fpj]

Thanks for the pointers, @lhotari. I agree that following the current guidelines makes sense.

[hsaputra]

Some comments to add quick improvements, but it is good base to let more updates and contribs.