-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
testing: tools/Dockerfile: Upgrade RocksDB to 6.24.2 #197
testing: tools/Dockerfile: Upgrade RocksDB to 6.24.2 #197
Conversation
@roysc can you fix conflicts here? |
Got your back. 🙂 |
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. |
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 |
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. |
Something is weird in the build. I'll figure out whatever it is, and merge this once it's working again. |
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make sure the tests pass in a container built from this config change. The one used for this PR is from the last change, and doesn't reflect this update.
My mistake, we also needed to update |
secrets arent set for a contribution from a fork. we have to open this ourself to get it to pass |
Everything passes now. This should be good to merge. Ill wait for @creachadair to approve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a little weird to introduce a fork that wasn't there before, but I guess we at least know this has been working in the SDK.
Yeah, the |
Updates Rocksdb install to include a version with bindings needed for new
cosmos-sdk/db
module.See cosmos/cosmos-sdk#10470