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

Expose librepo's checksum functions via SWIG #1164

Merged
merged 2 commits into from Mar 18, 2021

Conversation

malmond77
Copy link
Contributor

DNF has been carrying around yum's old checksum function. These
functions duplicate code in librepo. They are slower because librepo can
employ caching of digests. Lastly, these functions in Python do not know
about changes in checksum logic like
rpm-software-management/librepo#222

The choices here are:

  1. Replicate lr_checksum_cow_fd() and caching logic in Python
  2. Just use librepo from dnf.

This is 2. Note there is an open bug in librepo that forces no caching
for checksum_value()
(rpm-software-management/librepo#233). For
now we can keep the perf hit - this is no worse than the existing code
and aim for more correctness.

This change goes hand in hand with a change to dnf itself to make use
of the new functions and eliminate the old ones.

@m-blaha
Copy link
Member

m-blaha commented Mar 12, 2021

Also please bump the libdnf version in VERSION.cmake and libdnf.spec so that you can also bump the libdnf requirement in dnf.spec - rpm-software-management/dnf#1743
The same applies to librepo version in rpm-software-management/librepo#234 and bumping librepo version requirement here in libdnf.spec.

* We must pass FALSE here because librepo has a simple bug that
* doesn't set calculated if the checksum is cached
*/
checksum_valid ? TRUE : FALSE,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to bump %global librepo_version to 1.13.1 in libdnf.spec and update this code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also bumping the version of libdnf itself is needed - you need to update dependency in dnf package.

@softwarefactory-project-zuul
Copy link

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@softwarefactory-project-zuul
Copy link

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@malmond77
Copy link
Contributor Author

I realized that making a single commit with both the code changes, and the dependency info change makes backporting[1] more difficult. I am expressing this change in two commits to make my own life easier.

When it comes to actually merging this: you should be able to either merge the two commits, or if you want: squash them. In theory it doesn't affect backports.

[1] Aiming for https://git.centos.org/rpms/librepo/tree/c8s-sig-hyperscale-experimental and then c8s-sig-hyperscale branch in short order.

DNF has been carrying around yum's old checksum function. These
functions duplicate code in librepo. They are slower because librepo can
employ caching of digests. Lastly, these functions in Python do not know
about changes in checksum logic like
rpm-software-management/librepo#222

The choices here are:

1. Replicate `lr_checksum_cow_fd()` and caching logic in Python
2. Just use librepo from dnf.

This is 2. Note there was bug in librepo that forces no caching
for `checksum_value()`
(rpm-software-management/librepo#233). This is
now fixed, so we depend on librepo-1.13.1 to ensure this fix is present.

This change goes hand in hand with a change to `dnf` itself to make use
of the new functions and eliminate the old ones. This is
rpm-software-management/dnf#1743. We bump the
version of this library in the next commit to ensure this dependency is
expressed properly.

On errors, these functions raise libdnf.error.Error which can be easily
mapped into MiscError in dnf
This change depends on changes committed in librepo-1.13.1 and it is a
pre-requisite for another change in dnf, so we bump this version.

This is a distinct commit to make backporting this change easier.
@malmond77
Copy link
Contributor Author

I realized I was tracking the wrong base commit for my PR so I fixed it.

RPM build failure is due to librepo dependency packages not being available (yet). It wants 1.13.1 but that was only just merged.

@m-blaha
Copy link
Member

m-blaha commented Mar 18, 2021

Thanks, let's merge it. The test failure is not caused by the change.

@m-blaha m-blaha merged commit 90b1a26 into rpm-software-management:dnf-4-master Mar 18, 2021
@malmond77
Copy link
Contributor Author

@m-blaha can we close this?

@m-blaha
Copy link
Member

m-blaha commented Mar 18, 2021

Not sure I understand what you mean. This PR was already merged.

@malmond77
Copy link
Contributor Author

Thanks - I saw it was merged, but not closed. I wondered if this implied some other follow up work, or was just a missing step. Now I think it's just the latter.

@m-blaha
Copy link
Member

m-blaha commented Mar 19, 2021

No, merging of PR is a way of closing it - a successful one. I think you can't even close the issue once it is merged.

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

2 participants