[Spice-devel] [PATCH spice-common] Integrate recorder library

[Christophe de Dinechin]

> On 29 Nov 2018, at 18:23, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>> Hey,
>> 
>> On Thu, Nov 29, 2018 at 08:50:47AM -0500, Frediano Ziglio wrote:
>>>> 
>>>> On Wed, Nov 28, 2018 at 11:23:17AM +0100, Christophe de Dinechin wrote:
>>>>>> On 27 Nov 2018, at 15:38, Christophe Fergeau <cfergeau at redhat.com>
>>>>>> wrote:
>>>>>> I'm not really asking how to use it, but it's very odd to have a
>>>>>> patch
>>>>>> adding a new dep without seeing any accompanying patches which need
>>>>>> that
>>>>>> dependency.
>>>>> 
>>>>> You may want to look there:
>>>>> https://github.com/c3d/spice/tree/smart-streaming.
>>>>> I’m surprised you are not aware of this proposal.
>>>> 
>>>> I'm not saying I'm unable to find any examples of how it's used. By
>>>> looking at the patch sent to the mailing list, I have no idea if I
>>>> should assume the instrumentation is going to be merged as is, if only a
>>>> subset of it is intended to be submitted, if it's going to be modified,
>>>> ... So yes I'm aware of your work, but I don't know for sure what's
>>>> the plan for upstream.
>>>> 
>>>> Christophe
>>>> 
>>> 
>>> The plan is the same for downstream, downstream is mostly some former
>>> release with some patches on top taken from newer upstream (like CVEs
>>> and other important fixes), the only exception, as far as I know, are
>>> some customer related not still integrated to upstream that will be
>>> merged upstream when ready.
>>> 
>>> Why should be different for this patch?
>>> (Probably I didn't understand your concern)
>> 
>> I don't really understand what you mean here, so I guess we are indeed
>> not understanding each other. What I'm trying to say as that it's
>> quite unusual to have a patch which is "Add a new dependency" without
>> any accompanying patch making use of that new dependency.
>> 
> 
> I think I was confused by the "upstream" and though that your concern
> was related to some upstream/downstream difference.
> 
>> For example, when Marc-André added the json-glib dependency (
>> https://lists.freedesktop.org/archives/spice-devel/2018-August/045202.html
>> ), we did not get first a patch adding the new dep and not doing
>> anythingelse, and the rest of the series after this initial patch was
>> merged. The addition of the new dep was sent at the same time as the
>> patches which need json-glib.

Your comment makes sense to me now.

> 
> Usually following patches are also an explanation of the stuff you are
> adding. In this case the usage, as you also said, is pretty clear.
> A difference of this patch is that is not a simple library addition,
> there's an option, a dummy replacement, a test and configure function.
> 
> What you probably want to say is "I'd prefer the merge to be done
> with some code really making use of this library". If that the case
> however should not be an excuse to avoid to review it and discuss.

What about making a build option to conditionally implement the logging
integration that I had initially put in common/log.h? Would that be a better
way to proceed?


Thanks,
Christophe

> 
>> Christophe
>> 
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel