Source files and automated formatting

[PerditionC]

I would like to apply automatic formatting to kernel source files before applying changes. I.e update commit pipeline to run a formatting tool on source. Currently there is a mix of styles, tabs, and spacing. It doesn't help that each computer I use seems to show and convert tabs to spaces differently.

For discussion are 3 major questions:

1. should the changes be applied to all files with one big commit or should files be formatted as updates to that file made (probably initially as two steps, 1 patch for formatting then apply the change). Either way it will effect history but I think piecemeal preserves history better while the first way is more like a ripping off a bandaid and may be better long term.

2. what program do people suggest using to format the files? clang has one, and there are at least a few others

3. which options? i.e. what should the formatting look like. this one is largely personal taste, but I figure once set then nothing to argue about:-)

[boeckmann]

I think this should definitively be done. I already did this for FDISK.

1. I would do it on a per-file basis as the files get touched, in two commits, as you suggested.
2. I achieved good results with the clang formatter
3. I have a configuration in the FDISK repository, but I think most of you will not like it that much, as I prefer inner spaces in the function calls etc. https://github.com/FDOS/fdisk/blob/master/source/fdisk/clangfmt.cfg.

[boeckmann]

But not sure if it is that good doing it automatically in the pipeline. There are edge cases where the formatter may fail. I think it would be best to start with a configuration file for the formatter inside the repo and initially running the formatter manually.

[andrewbird]

I also use clang-format, but usually on a per function basis. I use it with vim integration so that I can select a region and just format that if required. I believe there are similar add-ons for Emacs and Vscode. In FDPP and Dosemu2 we've tried to limit the formatting changes where possible so that patches can go back and forth between FreeDOS kernel and Dosemu(1), respectively. If a function is so poorly formatted, or say more than 60% will be changed then it gets reformatted via clang-format in a separate commit first. Sure it will take longer to achieve a nice look, but with older projects where most of your development occurred before any global reformat, repository history will be considerably easier to navigate.

I'm not a fan of automatically reformatting source in the pipeline as I think it should be the responsibility of the committer what gets committed. Sometimes I like to adjust the formatted code to make it more readable.

As for style.

1. Try to match the majority of what you already have in the project, it will limit changes.
2. I like spaces, not tabs to indent.
3. These days I don't think it makes too much sense to restrict line length to 80 chars.

That said I don't commit that much to the kernel, so I'm happy to go with whatever you choose. And if you do choose clang-format to do the work leave the configuration file in the top level of the repository so others get to use it automatically.

Regarding CI pipeline checks, we already have build and boot tests for Watcom and Gcc on Linux as the tools are freely available. Can something be done for Windows too? I could probably get a DOS build with Dosemu2 and GCC to work if it doesn't offend anyone?

[PerditionC]

based on feedback so far, I will add a clang-format configuration file to the source for use, not make any changes to the ci pipeline so formatting will be done manually as files are changed. I like the idea of limiting to regions within file one is working on, so that will be the preferred choice. I will have to read up on how to do partial formatting of files. however, if big changes to a file are made, changes in over half the file, I'm fine if the whole file is reformatted - this should be rare. As far as style goes, I will look at the ones in use by fdisk and dosemu as the basis. I don't have strong preferences for most things, I generally try to match existing formatting. If anyone has any strong preferences or objections to this, please continue the discussion.

[PerditionC]

I'd like for there to be a DOS build in the CI. I usually build on Windows (NT family, ie currently Windows11) so I'm not concerned about it as much. But I will see about adding it. If we get a DOS based build working, I wonder if we can setup a Borland BC or TC build - I have extra licenses that can be used.

[andrewbird]

I wonder if we can setup a Borland BC or TC build - I have extra licenses that can be used.

I'm happy to do that. The difficulty is that the CI needs to get the compiler from somewhere on each run. As far as I know Github Actions has no private storage area we can upload it to, so it would have to get it from somewhere public.

I did a little test using GCC on Dosemu, but got foxed by the makefiles being gnu unfriendly. On the Linux build they are converted on the fly, see

kernel/makefile

Lines 92 to 96 in db949bf

	ifeq ($(COMPILER),gcc)
	MAKE=make
	MAKEADJUST=for i in utils lib drivers boot sys kernel setver; do sed 's@!include "\(.*\)"@include ../mkfiles/gcc.mak@' < $$i/makefile > $$i/GNUmakefile; done
	MAKEREMOVE=for i in utils lib drivers boot sys kernel setver; do rm -f $$i/GNUmakefile; done
	XLINK=ia16-elf-gcc

On the DOS build I'm trying to use the batch files that a user typically would, so scripting something on Linux first would be against the spirit of that. Perhaps I'll have more luck trying a Watcom DOS build instead.

[andrewbird]

@PerditionC I have Watcomc from FreeDOS 1.3 working. Now I'm moving onto Turbo C. I could be missing something as sys.c doesn't seem to be buildable with it as it is, but the breaking change looks to be very old(2017). Do you know the last time that you were able to build the kernel with Turbo C 2.01?

Turbo C  Version 2.01  Copyright (c) 1987, 1988 Borland International
sys.c:
Error sys.c 57: Unable to open include file 'sys/stat.h'
Error sys.c 561: Undefined structure 'stat' in function initOptions
Error sys.c 561: Size of structure or array not known in function initOptions
Warning sys.c 849: Call to function 'stat' with no prototype in function initOptions
Error sys.c 850: Undefined symbol 'st_size' in function initOptions
Warning sys.c 856: Call to function 'stat' with no prototype in function initOptions
Error sys.c 857: Undefined symbol 'st_size' in function initOptions
Warning sys.c 872: Call to function 'stat' with no prototype in function initOptions
Error sys.c 873: Undefined symbol 'st_size' in function initOptions
Warning sys.c 879: Call to function 'stat' with no prototype in function initOptions
Error sys.c 880: Undefined symbol 'st_size' in function initOptions
Warning sys.c 928: Call to function 'stat' with no prototype in function initOptions
Warning sys.c 932: Call to function 'stat' with no prototype in function initOptions
Warning sys.c 945: Call to function 'stat' with no prototype in function initOptions
Error sys.c 950: Undefined symbol 'st_size' in function initOptions
Warning sys.c 963: Call to function 'stat' with no prototype in function initOptions
Warning sys.c 966: Call to function 'stat' with no prototype in function initOptions
Error sys.c 1401: Undefined symbol 'S_IREAD' in function saveBS
Error sys.c 1401: Undefined symbol 'S_IWRITE' in function saveBS
Warning sys.c 1513: Constant is long in function put_boot
Error sys.c 1929: Undefined symbol 'S_IREAD' in function copy
Error sys.c 1929: Undefined symbol 'S_IWRITE' in function copy
*** 12 errors in Compile ***

        Available memory 296802

** error 1 ** deleting sys.obj
Compilation was aborted!

[PerditionC]

its been a really long time, its been years since I built with BC3 and I only built with TC2 on rare occasions when I regularly used Borland compilers - its one of the reasons I wanted a Borland compiler added to the CI. (I have soft spot for them, though find using OW is nicer since it runs natively on DOS, Windows, and Linux.)