On 08/11/2022 17:02, Edwin Torok wrote:
On 8 Nov 2022, at 16:03, Julien Grall <jul...@xen.org> 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.to...@citrix.com>
--
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.
Note that I haven't said this should never be done. This just need to be
correctly timed. Doing it in the middle of a deep freeze doesn't look
right to me.
[...]
- 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.
That's not really the point here. The point is that if we start to allow
large coding style change (whether automatic or manual) very late in the
release then it will be hard to reject it in the future.
In fact we already have guidelines for that. If you look at [1], only
bug fixes should be done past the code freeze (23rd September).
As I wrote before, this patch only seem to be a cosmetic/quality
improvement. IOW this is not a bug fix and would not qualify for 4.17.
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.
This is an argument for including this patch in Xen 4.18. As I wrote
above, I am not against that.
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).
What does LCM stands for?
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).
I would view this as a feature. Which again doesn't qualify for Xen 4.17
release. This doesn't mean the patch couldn't be backported afterwards.
Cheers,
[1]
as8pr08mb7991145c8063d6939affed8f92...@as8pr08mb7991.eurprd08.prod.outlook.com
Best regards,
--Edwin
Cheers,
--
Julien Grall
--
Julien Grall
