search

tendermint / tm-db

Common database interface for various database backends for Tendermint Core and Cosmos SDK
Apache License 2.0
89 stars 136 forks source link

testing: tools/Dockerfile: Upgrade RocksDB to 6.24.2 #197

Closed roysc closed 2 years ago

roysc commented 2 years ago

Updates Rocksdb install to include a version with bindings needed for new cosmos-sdk/db module. See https://github.com/cosmos/cosmos-sdk/pull/10470

tac0turtle commented 2 years ago

@roysc can you fix conflicts here?

creachadair commented 2 years ago

@roysc can you fix conflicts here?

Got your back. 🙂

creachadair commented 2 years ago

I believe the tests here (and on #194) will fail until the Docker workflow pushes a new version of the image. I think that will happen automatically soon, but I'm not positive. That image hasn't been updated in a while, and the credentials could be stale.

tac0turtle commented 2 years ago

I dont have permission to see what is in the secrets so i guess we merge and hop it works or someone who has permission looks into it

roysc commented 2 years ago

The only change is the url and file name and I tested building the image locally, so it should be fine. But the illegal instruction errors are kind of weird, is it some architecture mismatch in the CI build? anyway, it is happening on all open PRs from what I see.

creachadair commented 2 years ago

The only change is the url and file name and I tested building the image locally, so it should be fine. But the illegal instruction errors are kind of weird, is it some architecture mismatch in the CI build? anyway, it is happening on all open PRs from what I see.

Yes, the image hadn't been updated in a while, so in addition to the Go version change I suspect there may have been ABI or OS version changes. The new image tag (post #195) should help, I think.

creachadair commented 2 years ago

Something is weird in the build. I'll figure out whatever it is, and merge this once it's working again.

creachadair commented 2 years ago

Ok, I sorted out the build issues, but I believe in the current state, the updated rocksdb version is not compatible with the C API expected by the Go library. I didn't attempt to fix that, but at least now the tests should be diagnostic.

creachadair commented 2 years ago

Ok, I sorted out the build issues, but I believe in the current state, the updated rocksdb version is not compatible with the C API expected by the Go library. I didn't attempt to fix that, but at least now the tests should be diagnostic.

Or, maybe I'm wrong. I had build failures with this version previously, but the don't seem to be manifesting here. Let's see what happens.

creachadair commented 2 years ago

Or, maybe I'm wrong. I had build failures with this version previously, but the don't seem to be manifesting here. Let's see what happens.

Wait, I see why: The tests are running against the old version, because the build image does not update until after the Dockerfile change is merged. I believe the issue is still real.

roysc commented 2 years ago

My mistake, we also needed to update gorocksdb to the fork. The latest commit should pass.

tac0turtle commented 2 years ago

Something is weird in the build. I'll figure out whatever it is, and merge this once it's working again.

secrets arent set for a contribution from a fork. we have to open this ourself to get it to pass

tac0turtle commented 2 years ago

Everything passes now. This should be good to merge. Ill wait for @creachadair to approve.

roysc commented 2 years ago

Yeah, the gorocksdb maintainers were unresponsive about merging bindings we need, so I had to fork the repo.