dnf5 icon indicating copy to clipboard operation
dnf5 copied to clipboard

Python DownloadProgress.add_new_download() callback user data issue

Open es-fabricemarie opened this issue 10 months ago • 5 comments

Hi,

I'm trying to use DownloadProgress callbacks using the Python bindings and I face an issue with the add_new_download method.

The code to reproduce my issue can be simplified to:

#!/usr/bin/python3
import libdnf5

class DownloadProgress(libdnf5.repo.DownloadCallbacks):
    # Notify the client that a new download has been created.
    #
    # @param user_data User data entered together with url/package to download.
    # @param description The message describing new download (url/packagename).
    # @param total_to_download Total number of bytes to download.
    # @return Associated user data for new download.
    def add_new_download(self, user_data, description, total_to_download):
        print("Just added package %s to download list" % description)
        return description


base = libdnf5.base.Base()
base.load_config_from_file()
base.setup()
sack = base.get_repo_sack()
sack.create_repos_from_system_configuration()
sack.update_and_load_enabled_repos(True)
goal = libdnf5.base.Goal(base)
goal.add_rpm_upgrade()
transaction = goal.resolve()
downloader_callbacks = DownloadProgress()
base.set_download_callbacks(
    libdnf5.repo.DownloadCallbacksUniquePtr(downloader_callbacks)
)
transaction.download()

I'm trying to set the package name as the Associated user data for new download, so that it gets sent back to the other callbacks (progress and end for instance), so I can report fine grained progress.

I run this code, the script crashes and a core is dumped:

Just added package docker-buildx-plugin-0:0.14.0-1.fc39.x86_64 to download list
terminate called after throwing an instance of 'Swig::DirectorTypeMismatchException'
  what():  SWIG director type mismatch in output value of type 'void *'
Aborted (core dumped)

What am I doing wrong here? Or is this an issue with the Python binding?

es-fabricemarie avatar Apr 24 '24 04:04 es-fabricemarie

The first thing I would look into is probably implementing all the methods from the libdnf5.repo.DownloadCallbacks interface in your DownloadProgress class - there is also the progress, end, mirror_failure and fastest_mirror methods. That might be the source of an error if these methods are missing and the library tries to call them.

jan-kolarik avatar May 02 '24 13:05 jan-kolarik

I did of course. But the code above is the shortest way to reproduce my issue. In my example, as soon as add_new_download() method returns, it dumps core. All the examples I found return None. But when returning none, we lose the ability to pass metadata to the subsequent progress, end, etc.. callbacks.

es-fabricemarie avatar May 02 '24 14:05 es-fabricemarie

Right, I was able to reproduce the error with your code snippet. The problem lies in the return value from add_new_download. In the original C++ code, it's of type void *, which isn't currently handled by SWIG in our Python bindings. While this works with the C++ API, we need to address it in the Python bindings. In the meantime, you might find a workaround by using locally defined variables in the DownloadProgress class to store state. Check out this code example from our Python unit tests.

jan-kolarik avatar May 07 '24 07:05 jan-kolarik

The example code you linked doesn't seem to allow a program to keep track of concurrent packages downloading simultaneously. This appears to only be doable with the callbacks.

I've looked at SWIG documentation on how to add typemaps in the repo.i file to support that, but I've a hit a massive high wall, the learning curve is just to steep for me at this point in time. :cold_sweat:

es-fabricemarie avatar Jun 25 '24 06:06 es-fabricemarie

@jan-kolarik let me know how I can help with this issue (short of contributing a PR), like with testing and such.

es-fabricemarie avatar Sep 18 '24 15:09 es-fabricemarie

@jan-kolarik I hope this PR helps.

es-fabricemarie avatar Oct 10 '24 08:10 es-fabricemarie

Hi @es-fabricemarie, sorry for the late response. We've already started looking into this, will update the upstream issue here to reflect the correct state. And thanks for your related PR!

jan-kolarik avatar Oct 10 '24 11:10 jan-kolarik