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

SWIG bindings for user_cb_data in repo::DownloadCallbacks, unit tests #1849

Conversation

jrohel
Copy link
Contributor

@jrohel jrohel commented Nov 11, 2024

Closes: #1441

SWIG bindings for user_cb_data in repo::DownloadCallbacks in the C++ API, the user can return a pointer (void *) to user data in the add_new_download method of the repo::DownlodCallbacks class. This pointer is then passed as an argument to void * user_cb_data in other methods of the repo::DownloadCallbacks class.

There was a problem of how to use this mechanism in SWIG bindings.

I implemented and tested several solutions. In the case of passing a pointer, there was a problem with the ownership of the data the pointer points to. Users of Python and some other languages are used to the gargabe collector and automatically holding ownership of the passed data. While I've solved this for user_cb_data, passing another void pointer will need to be solved (I'll do that later). And from there, the ownership will be more complicated. We need the solution to work reliably in Python, Ruby and Perl. And possibly be easy to use for other languages in the future.

In the end, I chose the method of passing an integer instead of a pointer. When passing an integer, there is no need to deal with who owns the number. The integer is passed by value. And if the user needs to pass objects, for example, he can create an array of objects and pass the index (integer) to the array. The array also provides ownership of the objects.

PR also improves and adds unit tests for Perl and extends unit tests for other languages.

Unit tests for Python and Ruby implement the `BaseTestCase` class,
which containsshared initialization code.
This implements the `BaseTestCase` class for Perl unit tests.
There is no need to write initialization code in the tests. Instead, we can use
the `BaseTestCase` class, just like in Python and Ruby tests.
In the C++ API, the user can return a pointer (void *) to user data
in the `add_new_download` method of the `repo::DownlodCallbacks` class.
This pointer is then passed as an argument to `void * user_cb_data`
in other methods of the `repo::DownloadCallbacks` class.

There was a problem of how to use this mechanism in SWIG bindings.

I implemented and tested several solutions.
In the case of passing a pointer, there was a problem with the ownership
of the data the pointer points to. Users of Python and some other languages
are used to the gargabe collector and automatically holding ownership
of the passed data. While I've solved this for `user_cb_data`, passing another
void pointer will need to be solved (I'll do that later). And from there,
the ownership will be more complicated. We need the solution to work reliably
in Python, Ruby and Perl. And possibly be easy to use for other languages
in the future.

In the end, I chose the method of passing an integer instead of a pointer.
When passing an integer, there is no need to deal with who owns the number.
The integer is passed by value. And if the user needs to pass objects,
for example, he can create an array of objects and pass the index (integer)
to the array. The array also provides ownership of the objects.
The `test_package_downloader` unit tests for C++, Python, Ruby, Perl now test
passing `user_cb_data`.
In the `test_repo` tests, the implementation of the `add_new_download` and
`progress` methods has been intentionally removed to test functionality in case
the library user does not implement these methods.
@m-blaha m-blaha self-assigned this Nov 18, 2024
Copy link
Member

@m-blaha m-blaha left a comment

Choose a reason for hiding this comment

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

Thank you very much!

@m-blaha m-blaha added this pull request to the merge queue Nov 18, 2024
Merged via the queue into rpm-software-management:main with commit dabead7 Nov 18, 2024
20 checks passed
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.

Python DownloadProgress.add_new_download() callback user data issue
2 participants