Skip to content

Conversation

@dlg99
Copy link
Contributor

@dlg99 dlg99 commented Mar 22, 2022

Motivation

Bringing back maven build as per conversation on ML.

Changes

Reverted change that deleted mvn build, fixed dependencies that were changed after the mvn deletion, suppressed warnings in tests (gradle build seems to ignore the warnings)

Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

Are you able to restore github actions? In this way we are sure the build is stable

@eolivelli
Copy link
Contributor

Apache RAT fails due to "site3" directory, please add the exclusion:

diff --git a/pom.xml b/pom.xml
index aa1cb6f62..e7108f889 100644
--- a/pom.xml
+++ b/pom.xml
@@ -1014,6 +1014,7 @@
             <!-- Website -->
             <exclude>site/**</exclude>
             <exclude>site2/**</exclude>
+            <exclude>site3/**</exclude>
 
             <!-- Thrift generated files -->
             <exclude>**/org/apache/distributedlog/thrift/*</exclude>

@eolivelli
Copy link
Contributor

I built the package locally and run "localbookie" and a small bookkeeper cluster (tested with BKVM).
Everything looks good.
I am now running all the tests locally.

@eolivelli
Copy link
Contributor

Unit tests seem to pass locally (I didn't run integration tests)

@dlg99
Copy link
Contributor Author

dlg99 commented Mar 22, 2022

@nicoloboschi @eolivelli I could not just revert the changes for the GH actions because there commit after that. I did the changes manually/picked them from branch 4.14 mostly. I left javadoc generation with gradle. Backwards compat tests also left running with gradle, plus added one action that simply builds everything with gradle but does not run tests.

@eolivelli
Copy link
Contributor

There are problems with the license check

@eolivelli
Copy link
Contributor

Problems with the docker image for tests
Error: Failed to execute goal com.spotify:dockerfile-maven-plugin:1.4.13:build (default) on project current-version-image: Missing Dockerfile in context directory: /home/runner/work/bookkeeper/bookkeeper/tests/docker-images/current-version-image -> [Help 1]

…adle build didn't force versions consistently
@dlg99 dlg99 marked this pull request as draft March 22, 2022 21:55
@eolivelli
Copy link
Contributor

Very good!
I see only CompactionByEntriesWithoutMetadataCacheTest is failing.
It is not related to Maven

@dlg99 dlg99 marked this pull request as ready for review March 23, 2022 22:13
@dlg99
Copy link
Contributor Author

dlg99 commented Mar 23, 2022

I fixed CompactionByEntriesWithMetadataCacheTest in this PR because it was failing rather consistently.
The cases that were failing pass when I ran them one-by-one, it is possible gradle re-runs the failed tests differently from maven hence it was not as noticeable there.
org.apache.bookkeeper.test.BookieZKExpireTest flakes on CI ("Remaining Tests") but I cannot repro it locally and it finally passed on the re-run.

@eolivelli @nicoloboschi Please take another look.

Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

I did some checks locally:

  • build with mvn (no tests)
  • started a bookie and run simple test shell command
  • checked dependencies versions matching between maven and gradle

great work @dlg99 !

Copy link
Member

@zymap zymap left a comment

Choose a reason for hiding this comment

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

+1

Build with this branch
Run the zookeeper server + 1 bookie
Run the simpletest command successfully

@eolivelli eolivelli merged commit 2fe585c into apache:master Mar 24, 2022
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
* Revert "[build] remove Maven POM files (apache#3009)"

This reverts commit e089b51.

* rxjava: add maven dependency

(cherry picked from commit ac73541)

* Bring guava to the same version as gradle

* ignore deprecation warnings in tests

* mockito-inline, as in gradle + suppress warnings

* suppressed warning

* Exclude site3/ from RAT check

* CI to use (mostly) maven

* OWASP check with maven

* Up'd versions to match gradle, corrected license files: looks like gradle build didn't force versions consistently

* Removed current-version-image to match apache#3027

* Shading patetrn to match gradle

* Fixed/suppressed CVEs

* Attempt to fix failing tests in CompactionByEntriesWithMetadataCacheTest

Co-authored-by: lushiji <lushiji@didiglobal.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants