search

rpm-software-management / librepo

A library providing C and Python (libcURL like) API for downloading packages and linux repository metadata in rpm-md format
http://rpm-software-management.github.io/librepo/
GNU Lesser General Public License v2.1
75 stars 91 forks source link

Dowload package over damaged rpm #162

Closed j-mracek closed 5 years ago

lukash commented 5 years ago

Hi, it's not really clear from the first commit ("Adjust test for a new behavior of retrying mirrors") description why the h.allowedmirrorfailures = 1 is needed and (without digging into librepo code) I don't see why it should be? (I know the commit is already merged in a different PR).

I'm also not entirely sure this fixes the issue from https://bugzilla.redhat.com/show_bug.cgi?id=1690894 (which is that you write garbage to a partially downloaded file, so a resume will never end up with the right content). Again, didn't go looking right now. But I think that case is not hard to cover with a test, how about adding one?

j-mracek commented 5 years ago

Hi, it's not really clear from the first commit ("Adjust test for a new behavior of retrying mirrors") description why the h.allowedmirrorfailures = 1 is needed and (without digging into librepo code) I don't see why it should be? (I know the commit is already merged in a different PR).

I'm also not entirely sure this fixes the issue from https://bugzilla.redhat.com/show_bug.cgi?id=1690894 (which is that you write garbage to a partially downloaded file, so a resume will never end up with the right content). Again, didn't go looking right now. But I think that case is not hard to cover with a test, how about adding one?

The commit was part of another PR. The commit was removed from PR

j-mracek commented 5 years ago

With test we have a problem. It cannot be tested in /tmp or our ci-dnf-stack because file system there does not allow to use extended file attribute.

lukash commented 5 years ago

The commit was part of another PR. The commit was removed from PR

Yes, but you didn't answer my question...

With test we have a problem. It cannot be tested in /tmp or our ci-dnf-stack because file system there does not allow to use extended file attribute.

Yeah... does it not do any resuming if the attribute isn't there?

The test data could be moved to the current directory, which works as long as that doesn't end up being a tmpfs (like when the whole build happens in tmpfs which AFAIK is what e.g. tito does).

j-mracek commented 5 years ago

The commit was part of another PR. The commit was removed from PR

Yes, but you didn't answer my question...

With test we have a problem. It cannot be tested in /tmp or our ci-dnf-stack because file system there does not allow to use extended file attribute.

Yeah... does it not do any resuming if the attribute isn't there?

Correct when attribute is not present, librepo always download file from scratch

The test data could be moved to the current directory, which works as long as that doesn't end up being a tmpfs (like when the whole build happens in tmpfs which AFAIK is what e.g. tito does).

Even if test data are moved into current directory it doesn't provide an insurance that the location allows to use extended attributes.

lukash commented 5 years ago

I think this can be merged, however, it is most likely unrelated to the bug and won't fix it?

j-mracek commented 5 years ago

Thanks for review.

j-mracek commented 5 years ago

@rh-atomic-bot r+

j-mracek commented 5 years ago

I believe that there is no bot.