flare-foundation / go-songbird

BSD 3-Clause "New" or "Revised" License
66 stars 32 forks source link

Fix RocksDB issues. #21

Closed xrpdevs closed 2 years ago

xrpdevs commented 2 years ago

These commits update:

Anyone wanting to keep their existing database can do so with this build. Either move or link the rocksdb/v1.4.5 directory to be under .flare/db/songbird/...

I have extensively tested my own builds of RocksDB (v6.27.3) with running Flare nodes and found it to be much more stable than the version included with the default grocksdb that is included with Flare's distributions.

YMMV, but the problem with using RocksDB has always been that everyone was using a pre-built statically linked library that wasn't built against their own hardware.

This fully automates the build and installation of RocksDB from source.

Requires sudo and cmake

awfm9 commented 2 years ago

Hey @xrpdevs, thanks a lot for this PR. It's definitely much improved over the version that Avalanche bundles!

However, this turns a build with RocksDB into a binary that has external dependencies on the system. I think it's possible to include static C libraries with a Go binary, so that's probably a better way to go about it. We definitely want to keep Go binaries as portable as possible!

I will take a closer look at this when I get the time. There are some more major upgrades coming up, so it might be a while. In the meantime, people who use RocksDB should definitely look at your fork, it's the superior option.

xrpdevs commented 2 years ago

From what I can tell, when the golang binary is linked, the libs are included in the executable. This is certainly the case on Mac OS X which I tested by removing the libs from system dirs after compiling. Will check the Linux builds also. I believe golang’s linker only includes the symbols from the external libraries that it actually uses in the final executable.

Although it does add rocksdb as a dependency, compilation is handled in a way that just works in any environment, which has to be better than how the libs are packaged currently.

awfm9 commented 2 years ago

Hey @xrpdevs, it looks indeed like the library is built as a static library, I believe the corresponding flag is ROCKSDB_BUILD_SHARED. I will check to see if the same is true for the other libraries.

In any case, this means that there is no reason to install the library in any of the system directories. While this is a definite improvement already, it would be great if we could get it right before merging, so the build remains entirely self-contained.

With those changes, the build should also not need sudo.

xrpdevs commented 2 years ago

Sure, I'll take a look at keeping the build internal within the bounds of the go package directory - the only issue I can think of, is that depending on how much CPU power the machine that is doing the compiling has, the end-user might think that it has hung during build, so maybe it's a good idea to keep the cmake output display.

The other libs are indeed built as statics (.a) so the resulting executable remains portable and free of external req's.

Am happy to take responsibility for maintaining future updates to this, too..

xrpdevs commented 2 years ago

Removed requirement for sudo, updated Readme to reflect this. Build success on OS X, will comment again when I test under Linux, but should be no issues.

xrpdevs commented 2 years ago

OK... finally... building perfectly under Mac OS X and Debian/Ubuntu. ALMOST builds under Arch/Manjaro too, but fails due to some incompatibility with libpthread

This is ready for merge now if anyone wants to review.

xrpdevs commented 2 years ago

Up to date with 0.6.1 and ready for merge

awfm9 commented 2 years ago

Hey @xrpdevs, I'm looking at getting this into the next release. Are you reachable on Telegram to talk it through?

xrpdevs commented 2 years ago

Hey only just seen this. I think I have the telegram client on my machine, or also I'm on Discord as _jp#8626 , If you prefer to use Telegram, let me know your contact for there and I'll get in touch.

Ullaakut commented 2 years ago

@awfm9 Why was this closed?