[pacman-dev] [PATCH] libalpm: fix alpm_fetch_pkgurl with a fetch callback

[Guillaume Benoit]

Le 02/05/2021 à 11:47, Allan McRae a écrit :
> On 2/5/21 12:33 am, Guillaume Benoit wrote:
>> Le 01/05/2021 à 05:53, Andrew Gregory a écrit :
>>> On 04/30/21 at 12:09pm, Guillaume Benoit wrote:
>>>> After download, alpm_fetch_pkgurl uses payload->destfile_name
>>>> and payload->tempfile_name in order to find the downloaded file path.
>>>> Those fields are not set if a custom fetch callback is defined.
>>>> Use filecache_find_url instead, like in the beginning of the function.
>>>>
>>>> ---
>>>>    lib/libalpm/dload.c | 13 ++++---------
>>>>    1 file changed, 4 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
>>>> index 6f33451a..d45c7707 100644
>>>> --- a/lib/libalpm/dload.c
>>>> +++ b/lib/libalpm/dload.c
>>>> @@ -1007,16 +1007,11 @@ int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t
>>>> *handle, const alpm_list_t *urls,
>>>>                EVENT(handle, &event);
>>>>            }
>>>>    -        for(i = payloads; i; i = i->next) {
>>>> -            struct dload_payload *payload = i->data;
>>>> -            char *filepath;
>>>> +        for(i = urls; i; i = i->next) {
>>>> +            char *url = i->data;
>>>>    -            if(payload->destfile_name) {
>>>> -                const char *filename =
>>>> mbasename(payload->destfile_name);
>>>> -                filepath = _alpm_filecache_find(handle, filename);
>>>> -            } else {
>>>> -                STRDUP(filepath, payload->tempfile_name,
>>>> GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
>>>> -            }
>>>> +            /* attempt again to find the file in our pkgcache */
>>>> +            char *filepath = filecache_find_url(handle, url);
>>>>                if(filepath) {
>>>>                    alpm_list_append(fetched, filepath);
>>>>                } else {
>>>
>>> Without testing since this needs to be rebased now; this looks broken.
>>> filecache_find_url with fail for any url where destfile_name doesn't
>>> match the file name in the url.  Notably, that includes the download
>>> links on Arch's package pages:
>>> https://archlinux.org/packages/core/x86_64/pacman/download/
>>>
>> Yes, with or without this patch, the current implementation doesn't work
>> for this kind of url when using a fetch callback.
>> The solution I see is to change the fetch callback signature by making
>> it return the path of the downloaded file.
>> Otherwise this patch works for standard mirror urls.
>> .
> 
> While this patch does improve the current situation, it is a step
> backwards in terms the full fix, which will need to pass the full
> payload to the front-end so it can fill in the needed values.
> 
> On that basis, I will not be accepting this patch.
> 
> Given fetching a URL with XferCommand has not worked for a long time (or
> ever?), this issue is not a blocker for 6.0.
> 
> Allan
> 
Sorry for the bad formatting of my previous message. I had the brilliant idea to
answer from my phone...
I understand the priority is that the internal curl functions work so,
with this point of view, this patch can be seen as a regression.
The point is, currently alpm_fetch_pkgurl is broken with a fetch callback defined,
whatever url is used.
If I have time to work on another version of a patch that pass the full payload to
the front-end, is there a chance that it could be included in pacman 6.0 ?