Skip to content
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

Merged

Conversation

roysc
Copy link
Contributor

@roysc roysc commented Nov 8, 2021

Updates Rocksdb install to include a version with bindings needed for new cosmos-sdk/db module.
See cosmos/cosmos-sdk#10470

@tac0turtle
Copy link
Contributor

@roysc can you fix conflicts here?

@creachadair
Copy link
Contributor

@roysc can you fix conflicts here?

Got your back. 🙂

@creachadair
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

roysc commented Nov 8, 2021

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
Copy link
Contributor

creachadair commented Nov 8, 2021

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
Copy link
Contributor

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

@creachadair creachadair self-requested a review as a code owner November 8, 2021 23:52
@creachadair
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor

@creachadair creachadair left a 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.

@roysc
Copy link
Contributor Author

roysc commented Nov 9, 2021

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

@tac0turtle
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Contributor

@creachadair creachadair left a 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.

@creachadair creachadair merged commit 8f92601 into tendermint:master Nov 9, 2021
@roysc
Copy link
Contributor Author

roysc commented Nov 10, 2021

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

@roysc roysc deleted the testing-dockerfile-update-rocksdb branch November 10, 2021 07:25
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.

None yet

4 participants