-
Notifications
You must be signed in to change notification settings - Fork 197
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
composepost: Normalize rpmdb.sqlite-shm
#4074
base: main
Are you sure you want to change the base?
Conversation
rust/src/composepost.rs
Outdated
@@ -274,6 +274,11 @@ fn postprocess_cleanup_rpmdb_impl(rootfs_dfd: &Dir) -> Result<()> { | |||
if matches!(name, ".dbenv.lock" | ".rpm.lock") || name.starts_with("__db.") { | |||
d.remove_file(name)?; | |||
} | |||
// SQLite case. Today, it seems to work to remove the WAL, but removing the SHM | |||
// file causes a runtime error as RPM tries to mutate a read-only database. | |||
if matches!(name, "rpmdb.sqlite-shm" | "rpmdb.sqlite-wal") { |
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.
In a quick search, I stumbled on https://www.sqlite.org/wal.html which seems to indicate the WAL file is a kind of transaction journal. It wasn't clear to me whether there could be pending transactions not yet committed back that we could be clobbering. Though it looks like librpm force a checkpoint when releasing: https://github.com/rpm-software-management/rpm/blob/c4eb357fe87e7fb7e3895e52cb9dad587b72b2b9/lib/backend/sqlite.c#L207. Hmm, so it should be empty if it was just checkpointed. Do you get reproducible builds if you drop out the WAL file from this match?
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.
I also don't know much about SQLite, so I reserve the right to be completely wrong about all the above. :)
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.
A better page is https://www.sqlite.org/walformat.html which talks about the role of the -shm
file.
I commented over in rpm-software-management/rpm#2219 but basically the problem seems to be that while sqlite will happily recreate it on start, it will only do that successfully if the directory (and database) isn't read only. I think librpm may be able to fix this by avoiding any database mutation operations when the file is read only.
This code was untested before, I just did one more quick untested thing of just entirely zeroing it but if that doesn't work at some point I'll try digging into a more real solution.
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.
Out of curiosity, would zero-sized files work for this case (i.e. just truncating the entry)?
The docpage only mentions that the files should be present; it looks like a purely cosmetic placeholder may be good enough (and deterministic/shareable).
In some testing I have using `SOURCE_DATE_EPOCH=0`, this file is now the only thing that differs across builds.
e7e91ee
to
3948d86
Compare
Filed rpm-software-management/rpm#2219 to track fixing this more properly; I didn't dig deep but I think librpm needs to learn how to handle a read-only rpmdb. |
@cgwalters: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
In some testing I have using
SOURCE_DATE_EPOCH=0
, this file is now the only thing that differs across builds.