Re: [PATCH for-4.17 v3 07/15] CODING_STYLE(tools/ocaml): add 'make format' and remove tabs

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> On 8 Nov 2022, at 16:03, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi,
> 
> On 08/11/2022 15:33, Edwin Török wrote:
>> See CODING_STYLE: Xen uses spaces, not tabs.
>> * OCaml code:
>> Using `ocp-indent` for now to just make minimal modifications in
>> tabs vs spaces and get the right indentation.
>> We can introduce `ocamlformat` later.
>> * C stubs:
>> just replace tabs with spaces now, using `indent` or `clang-format`
>> would change code too much for 4.17.
>> This avoids perpetuating a formatting style that is inconsistent with
>> the rest of Xen, and that makes preparing and submitting patches more
>> difficult (OCaml indentation tools usually only support spaces, not tabs).
>> Contains a bugfix for `abi-check` script to handle the change in the
>> amount of whitespace.
>> No functional change.
>> Signed-off-by: Edwin Török <edvin.torok@xxxxxxxxxx>
>> --
>> Reason for inclusion in 4.17:
>> - makes it easier to backport changes from master to 4.17
> 
> Right, but you will have the problem when backporting to 4.16 and older. So 
> the overhead will always be there for a couple of years.


There will always be more than one Xen release in support, which means we'd 
never be able to fix this.
At some point we need to decide to go ahead and just do it (which may or may 
not be 4.17)

The whitespace fixup needs to happen somewhere anyway, there are a couple of 
options:
* use git rebase/cherry-pick -Xignore-space-change to avoid backport failing to 
apply
* do a reformat on 4.16 branch too (backport the couple of lines of Makefile 
change and run 'make format' and commit the result)
* include the reformat as a prerequisite of the next security patch (I'd prefer 
not to, hence including it in 4.17)
* do the whitespace fixup prior to submission, turning a correctly formatted 
patch with spaces into an incorrectly formatted one with tabs. I wouldn't have 
minded doing this if the coding style said tabs,
I could've run my reformat tool locally and fix it up with 'sed' prior to 
submission. However this seems unnecessary overhead if doing this works 
actively against the existing coding style.

However if we put this into 4.17 and there are no new security issues 
discovered before 4.16 goes out of support then we'll be in a much better 
position for 4.18 onward.
There is still quite a lot of work outstanding in testing of oxenstored 
(updating my fuzzer for one), so I can't really predict at this point whether 
there are more security bugs in oxenstored or not,
so maybe we need to take this decision at 4.18 time? Not sure.

> 
>> - avoid perpetuating a different coding style (I thought tabs were
>>   mandated by Xen, and was about to fix up my editor config to match
>>   when I realized Xen already mandates the use of spaces)
>> - should make submitting patches for OCaml easier (OCaml indentation
>>   tools know only about spaces, so I either can't use them, or have to
>>   manually adjust indentation every time I submit a patch)
>> - it can be verified that the only change here is the Makefile change
>>   for the new rule, 'git log -p -1 -w' should be otherwise empty
> 
> While I understand the goal and support, this seems to be a bit too late to 
> do it in Xen 4.17 (we are only a couple of weeks away). At this stage of the 
> release we should only do bug fix.

I think it can be fairly easily proven that there is no functional change by 
rerunning the make format command manually, and by looking at the diff with 
ignore whitespace as suggested above.
I understand the reluctance in including it (which is why I was not sure 
whether to post it in the first place), but I think it might be beneficial to 
do it.
There is a large backlog of work in oxenstored that got piled up during the 
past couple of years of XSA work, and it'd be a lot easier to update and 
upstream those if we wouldn't have to worry
about indentation at all.

Usually patches on LCM and security branches are avoided to reduce the risk of 
breaking anything, but a reindentation patch should not really break anything 
(well other than the abi-check script in the build, but I fixed that to accept 
both ways).

One alternative would be that I add another step after reformat that runs sed 
and turns spaces back into tabs for now, and that way I can still run 'make 
format' at each step while preparing patches for master, or 4.17 or security 
patches and get something consistent, and that minimizes other whitespace 
changes, but it wouldn't completely eliminate them (e.g. there are pieces of 
code that are just wrongly indented, so there'd be at least a diff to fix all 
that).

> 
> This is clearly only a comesmetic change and there I would argue this should 
> be deferred to 4.18. That said the last call is from the RM.


Best regards,
--Edwin

> 
> Cheers,


> 
> 
> -- 
> Julien Grall