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

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> On 9 Nov 2022, at 09:52, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 09.11.2022 10:36, Jan Beulich wrote:
>> 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.
> 
> But, sigh, I had to fix up the patch: Even the one submitted standalone
> used space indentation when the file in the tree uses hard tabs. And
> even if I had wanted to pull from your github tree I would have had to
> fix up at least the Fixes: tag.


I thought I fixed it (the missing '('), but the format of the Fixes: line was 
not documented in 
https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches (which is the 
canonical resource I use when sending patches)
and I just tried to guess the format based on the Fixes: entries I found in 
master (some of which have or less characters in the git hash)

I see there is a 
https://xenbits.xen.org/docs/unstable/process/sending-patches.html which has 
some more useful details,
including a way to automatically generate the Fixes: line using a git config, 
which is very useful and I'll use that in the future instead of crafting it by 
hand
(which is what I've been doing so far).
I've edited the wiki to include this information now.

>  So I ended up hand-editing indentation



Thanks,
--Edwin