Re: [PATCH for-4.17 v3 09/15] tools/ocaml/xenstored/store.ml: fix build error

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.11.2022 10:21, Edwin Torok wrote:
> 
> 
>> On 9 Nov 2022, at 07:10, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 09.11.2022 03:47, Henry Wang wrote:
>>>> -----Original Message-----
>>>> From: Edwin Török <edvin.torok@xxxxxxxxxx>
>>>> Subject: [PATCH for-4.17 v3 09/15] tools/ocaml/xenstored/store.ml: fix 
>>>> build
>>>> error
>>>>
>>>> Building with Dune in release mode fails with:
>>>> ```
>>>> File "ocaml/xenstored/store.ml", line 464, characters 13-32:
>>>> Warning 18: this type-based record disambiguation is not principal.
>>>> File "ocaml/xenstored/store.ml", line 1:
>>>> Error: Some fatal warnings were triggered (1 occurrences)
>>>> ```
>>>>
>>>> This is a warning to help keep the code futureproof, quoting from its
>>>> documentation:
>>>>> Check information path during type-checking, to make sure that all types
>>>> are
>>>>> derived in a principal way. When using labelled arguments and/or
>>>> polymorphic
>>>>> methods, this flag is required to ensure future versions of the compiler 
>>>>> will
>>>>> be able to infer types correctly, even if internal algorithms change. All
>>>>> programs accepted in -principal mode are also accepted in the default
>>>> mode with
>>>>> equivalent types, but different binary signatures, and this may slow down
>>>> type
>>>>> checking; yet it is a good idea to use it once before publishing source 
>>>>> code.
>>>>
>>>> Fixes: db471408edd46 "tools/ocaml/xenstored: Fix quota bypass on domain
>>>> shutdown"
>>>
>>> Nit: The format of this "Fixes:" tag might need to be fixed?
>>>
>>>>
>>>> Signed-off-by: Edwin Török <edvin.torok@xxxxxxxxxx>
>>>> ---
>>>> Reason for inclusion in 4.17:
>>>> - fixes a build error in a previous commit that is already in master
>>>
>>> Yes, given this is a simple enough patch:
>>>
>>> Release-acked-by: Henry Wang <Henry.Wang@xxxxxxx>
>>
>> Afaics this patch was previously posted in isolation, and it was
>> already release-acked. What's lacking there is a 2nd maintainer's
>> ack or a proper R-b. When it now is patch 9 in a series, it isn't
>> really obvious whether this could also be committed in isolation
>> (it looks like it does, but a clear statement to this effect
>> would have been beneficial).
>>
> 
> 
> You're right it already has both acks, it just hasn't been commited yet: 

Oh, that's only because I overlooked Christian's ack. Will commit this now.

Jan