Install callbacks

[jreidinger]

Problem

The last missing piece for libzypp callbacks are the ones used for package installation.

Previously in this series: #2901

Solution

Implement them. Also improve previous ones with override keyword in C++ to ensure we use proper method to avoid "overload" instead of "override".

Testing

No testing as it is postponed to time when we have working install phase with new stack.

TODO: test this as part of this PBI:

• https://trello.com/c/HRqBeOi7/5450-support-for-instinstallurl

[imobachgs]

The C/C++ part is a little bit of dark magic for me :-) The Rust code looks fine, perhaps @mvidner could have a closer look to the C/C++ one.

[imobachgs]

?

[jreidinger]

I am still not sure how to properly report progress when there is two callbacks struct where the first one basically mainly download packages and here it install packages. It also need a bit of additional support from libzypp part as yast precompute for progress amount of packages to install, so it can create nicer sub-progress.

[imobachgs]

👍 that's what I meant with this comment.

[jreidinger]

I forgot to commit it in previous PR. It was already there :)

[mvidner]

Naming: I would expect convert_response since that is the argument type.

(Actually I would like to see Rust-like function names, like to_foo, as_foo for standalone functions or from_foo for methods)

[jreidinger]

yeah, using such method names can be really helpful. So I will change it

[mvidner]

You mean, YaST does nothing special on this callback and just continues with the next package? If it works, then fine, we can keep doing nothing in Agama too

[jreidinger]

It is more tricky as YaST for backward compatibility reason use identical callback for Problem and Finish, so it do a bit of magic there. But part specific to finish is really quite trivial logging.

[mvidner]

In the new callbacks (PatchScriptReport, InstallResolvableReport) we do call the base class method but here we avoid it. Why?

[jreidinger]

right, it should also call the original one which will just call that true. Good catch, I overlook this change.

[mvidner]

Oh, I've missed this: we need at least a lifetime comment, better a smart pointer that provides some safety.

[jreidinger]

well, smart pointer won't help you when rust deallocate it. Comment would be OK. Basically lifetime is for commit phase where we set and unset it ( it is same for all callbacks. I want to have it living only for limited time needed for operation ).

[mvidner]

// lifetime of pointer is quite short. Only during operation which takes
// callbacks as parameter.

"Lifetime of pointer is quite short" is a nice philosophical take 🤣
but I wanted something more specific:
Why is PatchScriptReport::callbacks valid at all times? It is trivial to see that PatchScriptReport has a static instance so the danger of the pointer outliving its target is real.

IIUC, the answer is: callbacks is only assigned via set_callbacks, which is in turn called only from {set,unset}_zypp_foo_callbacks, in this fashion which makes sure that foo_callbacks are only borrowed by FooReport for the duration of the method.

some_zypp_method(foo_callbacks) {
  set_zypp_foo_callbacks(foo_callbacks);
  zypp::some_method_using_the_callbacks();
  unset_zypp_foo_callbacks();
}

As you can see I had to do quite some explaining. It would be nice to design the API in such a way where it would be more obvious and ideally checked by the C++ compiler.

[mvidner]

I feel like I'm nitpicking, but:

IMHO something named from_foo must be a constructor, but here we return DownloadResolvableError in a method of DownloadResolvableReport.

So I would use into_error or into_dre_error here. What do you think?

[jreidinger]

it is fine for me. Probably I will use simple into_error because I like how they are consistent across different callbacks

[mvidner]

Thanks, the C++ part looks good now

[jreidinger]

@mvidner what about having this doc at top of file to make it clear lifetime at single place instead of all pointers?

/**
 * @file callbacks.cxx
 * @brief Implements C-style wrappers for libzypp callbacks.
 *
 * This file defines a mechanism to expose libzypp's C++ callback system
 * to C-compatible interfaces, primarily for use with FFI (Foreign Function Interface)
 * bindings, such as those used in Rust.
 *
 * Overall Approach:
 * -----------------
 * 1.  **Custom `ReceiveReport` Implementations:** For each type of libzypp report
 *     (e.g., `ProgressReport`, `DownloadProgressReport`, `KeyRingReport`), a custom
 *     C++ class (e.g., `ProgressReceive`, `DownloadProgressReceive`) is defined.
 *     These classes inherit from `zypp::callback::ReceiveReport<T>` where `T` is
 *     the specific libzypp report type.
 * 2.  **Static Instances:** A single, static instance of each custom `ReceiveReport`
 *     implementation is declared (e.g., `static ProgressReceive progress_receive;`).
 *     These static instances act as the central point for registering and
 *     deregistering with libzypp's global callback system.
 * 3.  **C-style Callback Structs:** Each custom `ReceiveReport` instance holds a
 *     pointer to a C-style `struct` (e.g., `struct DownloadProgressCallbacks *callbacks;`).
 *     These structs are defined in `callbacks.h` and contain function pointers
 *     for the actual C-compatible callback functions, along with `void *user_data`
 *     for passing context back to the caller.
 * 4.  **Registration (`set_zypp_*_callbacks`):** Functions like
 *     `set_zypp_progress_callback` or `set_zypp_download_callbacks` are provided
 *     to allow external C/FFI code to register its callbacks. These functions:
 *     -   Store the provided C-style callback struct pointer in the static
 *         `ReceiveReport` instance.
 *     -   Call `connect()` on the static `ReceiveReport` instance, which registers
 *         it with the appropriate libzypp global reporter.
 * 5.  **Deregistration (`unset_zypp_*_callbacks`):** Corresponding `unset_zypp_*_callbacks`
 *     functions are provided to deregister callbacks. These functions:
 *     -   Set the internal C-style callback struct pointer to `NULL` to prevent
 *         dangling pointer issues if the caller's struct is deallocated.
 *     -   Call `disconnect()` on the static `ReceiveReport` instance, removing
 *         it from libzypp's global reporter.
 *
 * Lifetime of Callback Structs:
 * -----------------------------
 * The `struct` containing the C-style callback function pointers and `user_data`
 * (e.g., `DownloadProgressCallbacks`) is passed by pointer to the `set_zypp_*_callbacks`
 * functions. The static `ReceiveReport` instances *store this pointer*.
 *
 * **It is critical that the caller of `set_zypp_*_callbacks` ensures the lifetime
 * of the provided callback struct.** The struct must remain valid and in scope
 * for the entire duration that the callbacks are registered with libzypp.
 * If the struct is deallocated or goes out of scope while libzypp still holds
 * a pointer to it (i.e., before `unset_zypp_*_callbacks` is called), any subsequent
 * callback invocation by libzypp will result in undefined behavior, likely a crash
 * due to use-after-free.
 *
 * Therefore, callers must always pair `set_zypp_*_callbacks` with a corresponding
 * `unset_zypp_*_callbacks` call when the callback struct is no longer valid or needed.
 */