mikaku / Fiwix

A UNIX-like kernel for the i386 architecture
https://www.fiwix.org
Other
401 stars 32 forks source link

Support override of configuration definitions #44

Closed rick-masters closed 9 months ago

rick-masters commented 9 months ago

Please support making it easier to customize the configuration of Fiwix from the command line.

The configuration of Fiwix for use with live-bootstrap requires customizing 12 different parameters so it would be helpful to allow specifying this in the build scripts instead of changing the source code.

For example, to use the 2/2 memory split it should be possible to compile like so:

gcc -DCONFIG_VM_SPLIT22 ...

To use the qemu debug console, it should be possible to compile like so:

gcc -DCONFIG_QEMU_DEBUGCON ...

To expand a limit, it should be possible to compile like so:

gcc -DOPEN_MAX=1536

However, these variables cannot be specified at the command line. Since the latest definition "wins" a definition in a source file takes precedence over a command line option.

A common way to allow command line definitions is to wrap all definitions with #ifndef. https://stackoverflow.com/questions/41437799/overwrite-a-macro-constant-from-a-makefile

Here is an example config file for a project that does it that way: https://github.com/gkostka/lwext4/blob/master/include/ext4_config.h Many examples of this pattern can be found in large C programs.

There are also many variables in Fiwix which have #undef before them. I understand this is to document their existence but an #undef also prevents defining a variable at the command line. A more common method in my experience is to comment out the definition to show that the definition is avaiable but is not defined by default. For example see FLUSH_ALL_TLBS in this file: https://github.com/qemu/qemu/blob/master/target/ppc/mmu_helper.c

Finally, it would be helpful if the Makefiles supported passing parameters from the command line to the compiler like so:

make CONFFLAGS="-DCONFIG_VM_SPLIT22 -DOPEN_MAX=1536"
mikaku commented 9 months ago

I see the advantage, the problem is that it creates excessive pollution in the files and reduces a lot the readability. This is only a cosmetic problem, of course.

Also there is the issue with the #undef constants as they don't follow the same mechanism and are treated in a different way. I see it a bit ugly, but again this is only another cosmetic problem.

How does Linux solve this? I mean, I don't follow the modern Linux kernels but I remember that there was a file called .config which included all the enabled options. I don't mean to change this PR now, but it's something we can do in the future?

Finally, I'm a bit concerned with the constant UTS_SYSNAME. Why do you need to change this one? I mean, the name Fiwix is bound to the copyright line that appears during the boot up of the kernel. If you change the name of the kernel, then there could be a collision with that copyright.

What do you think?

mikaku commented 9 months ago

You might want to rebase your PR because of my latest commit 1ee08fc

rick-masters commented 9 months ago

I see the advantage, the problem is that it creates excessive pollution in the files and reduces a lot the readability. This is only a cosmetic problem, of course.

Yes, it's not as clean but its the price to pay for the flexibility. However, this is a very common pattern and will be familiar to C programmers. Of course it's your decision whether to go this way or not. If you choose to decline this PR then I'll just have to patch the header files before building Fiwix.

The issues is that patching headers would be cumbersome, to be honest. When Fiwix is built in live-bootstrap, we have not yet built patch because patch requires make. And we can't run make on the builder-hex0 operating system that precedes Fiwix because builder-hex0 is not powerful enough. Instead of patch, there are some primitive patch-like tools that can run on builder-hex0 but every source line that must be patched requires a separate command. Here is an example:

https://github.com/fosslinux/live-bootstrap/blob/493ddfa8293fc158342dc796ab6021dae2a491aa/sysa/mes-0.24.2/mes-0.24.2.kaem#L71-L74

So, it can be done and patching the headers would have the benefit that I could remove the config overrides from the build commands. See here:

https://github.com/fosslinux/live-bootstrap/blob/493ddfa8293fc158342dc796ab6021dae2a491aa/sysa/fiwix-1.4.0-lb3/fiwix-1.4.0-lb3.kaem#L21-L23

Also there is the issue with the #undef constants as they don't follow the same mechanism and are treated in a different way. I see it a bit ugly, but again this is only another cosmetic problem.

Again, this is up to you but I'll note again that commented out defines are a common pattern. However, they can be avoided by replacing "blank" defines with 1 and 0, for example, replace:

/* #define CONFIG_KEXEC */
#ifdef CONFIG_KEXEC
<code>
#endif

with:

#ifndef CONFIG_KEXEC
#define CONFIG_KEXEC 0
#endif

#if CONFIG_KEXEC
<code>
#endif

How does Linux solve this? I mean, I don't follow the modern Linux kernels but I remember that there was a file called .config which included all the enabled options. I don't mean to change this PR now, but it's something we can do in the future?

Linux uses a complicated custom configuration system which presumes you will run a program to generate the configuration. I think it is too much for Fiwix. Another alternative is GNU autotools which can generate a config.h but its very complicated and live-bootstrap doesn't have it until long after Fiwix is built.

Another alternative would be to allow including overrides at the end of config.h (and limits.h, etc) like so:

#ifdef USER_CONFIG_H
#include "user_config.h"
#endif

Which this method, the build system produces a user_config.h and then builds with -DUSER_CONFIG_H to activate it.

Finally, I'm a bit concerned with the constant UTS_SYSNAME. Why do you need to change this one? I mean, the name Fiwix is bound to the copyright line that appears during the boot up of the kernel. If you change the name of the kernel, then there could be a collision with that copyright.

There are numerous packages in live-bootstrap that use the system name (uname -s) to define build parameters and they are looking for "Linux". Technically the right way to do this would be to patch everyone of them to also recognize Fiwix but that's an enormous amount of work because many of them use autotools and its buried in different places for each package. And it's a lot of packages. It's much easier to just mimic Linux.

This doesn't affect your copyright status. The source code is already protected with copyright declarations. (Even those declarations are not technically required because copyright declarations are not even required under U.S. law, although I understand Fiwix is not limited to the U.S. Just saying.) https://copyrightalliance.org/education/copyright-law-explained/copyright-basics/what-is-copyright-notice/#:~:text=Prior%20to%201989%20including%20a,a%20defense%20of%20innocent%20infringement.

The name "Fiwix" could just be hard coded into the start up message if you don't want it to be variable. But otherwise, I think the UTS_SYSNAME variable has some reason for existing, no? I think I'm using it for the intended purpose it might have...

What do you think?

I want to say again that you should feel free to manage the source code to your preference. I can patch if necessary. If you want the start up message to say "Fiwix" I can honor that. Just let me know so I can do what needs to be done.

BTW, I have rebased the PR.

rick-masters commented 9 months ago

After thinking a bit I think you might be happiest with the user_config.h method and that wouldn't be that hard to do. You could keep config.h, limits.h, etc mostly the same.

The downside is that there is an issue if config.h itself uses variables because the values may not be "settled" yet. For example NR_FLOCKS uses NR_PROCS, so if user_config.h changes NR_PROCS it should also define NR_FLOCKS appropriately. And user_config.h will have to be maintained to keep up with any new or changed definitions in the future that are dependent on definitions changed in user_config.h.

You've kept config.h fairly clean so this approach should work ok. Some projects have a lot of complicated nested definitions which make it harder to override definitions after the fact in a coherent way.

mikaku commented 9 months ago

Thank you for your always very detailed explanations. I really appreciate it.

... Another alternative would be to allow including overrides at the end of config.h (and limits.h, etc) like so:

#ifdef USER_CONFIG_H
#include "user_config.h"
#endif

I really like this idea. If I understood correctly, it would be as easy to include these thee lines on every header that needs to be overridden during the build, while keeping the readability of the original files. I guess that Makefile should always create at least an empty include/fiwix/user_config.h file, isn't?

Please, let's take this path, but only if this solution won't requires more effort for your work in the live-bootstrap project.

This doesn't affect your copyright status. The source code is already protected with copyright declarations.

Well, basically I'm concerned about what people will say if they see a live-bootstrap logfile with the following contents:

                    Linux kernel v1.4.0 for i386 architecture
                     Copyright (c) 2018-2022, Jordi Sanfeliu

             (built on Mon Oct 30 15:44:28 CET 2023 with GCC 4.7.4)

DEVICE    ADDRESS         IRQ   COMMENT
--------------------------------------------------------------------------------
...

I don't pretend to copyright the Linux kernel to me. :-)

rick-masters commented 9 months ago

OK, I'll work on a new PR with user_config.h, user_limit.h. I think these files would be generated before running make, not by the Makefile. I will also add them to .gitignore because they are not part of the "standard" source code. I can also hard code Fiwix in the startup message.

I'll be testing corresponding changes to live-bootstrap with this strategy so that may add some time before I'll have the PR ready.

mikaku commented 9 months ago

OK, I'll work on a new PR with user_config.h, user_limit.h. I think these files would be generated before running make, not by the Makefile.

Perfect.

I will also add them to .gitignore because they are not part of the "standard" source code. I can also hard code Fiwix in the startup message.

Yes, please. Thanks.

I'll be testing corresponding changes to live-bootstrap with this strategy so that may add some time before I'll have the PR ready.

I plan to release the next Fiwix version 1.5.0 on 15 November. Not sure if we have enough time to include all your pending changes from your own Fiwix branch. If not, no problem, we can continue adding your changes into 1.5.0.

mikaku commented 9 months ago

That is really neat. Thank you very much.