On Thu, 2023-11-02 at 12:30 +0100, Pablo Neira Ayuso wrote: > On Thu, Nov 02, 2023 at 12:14:49PM +0100, Pablo Neira Ayuso wrote: > > On Thu, Oct 19, 2023 at 03:00:03PM +0200, Thomas Haller wrote: > > > diff --git a/Makefile.am b/Makefile.am > > > index 8b8de7bd141a..83f25dd8574b 100644 > > > --- a/Makefile.am > > > +++ b/Makefile.am > > > @@ -2,6 +2,8 @@ ACLOCAL_AMFLAGS = -I m4 > > > > > > EXTRA_DIST = > > > > > > +################################################################ > > > ############### > > > > This marker shows that this Makefile.am is really getting too big. > > > > Can we find a middle point? > > > > I understand that a single Makefile for something as little as > > examples/Makefile.am is probably too much. > > > > No revert please, something incremental, otherwise this looks like > > iptables' Makefile. > > Correction: Actually iptables' Makefiles show a better balance in how > things are split accross Makefiles, with some possibilities to > consolidate things but it looks much better these days. > Basically, what I said in the commit message of [1]. Do you disagree with anything specifically (or all of it?). [1] https://git.netfilter.org/nftables/commit/?id=686d987706bf643f2fa75c1993a5720ad55e6df1 It's a matter of opinion entirely, but I disagree that the iptables' Makefiles are a positive example. --- The ### line is only a visual aid. `wc -l` seems the better indication for when the file gets too big. A too big file, can be an indication that it's hard to maintain. After all, it's about maintainability, being understandable and correctness. But is it now really harder to understand, and would splitting it into multiple files make it better? - see how you would add a new .c/.h file. Can you find it easily with the large Makefile? - see how libnftables.la is build. Can you find it? - see who uses libnftables.la. Can you find it? - which dependencies has libnftables.la? Which CFLAGS? Don't try to understand the file at once. Look what you want to do or look at a single aspect you'd like to understand, and how hard that is now (I claim, it became simpler!). I mean, previously you could read "examples/Makefile.am" at once. But when you type `make -C examples` then the dependencies were wrong and parallel build doesn't work (across directories). Look how it's now. Can you find how examples are build in the large Makefile.am? It's all in one file, open in your editor and easy to jump around. -- Another example: "src/expression.c" is 5k+. See how most include/*.h include each other, meaning that most source files end up with all headers included. Overall, the cohesion/coupling of the code doesn't seem great. Maybe it needs to be that way and couldn't be better. The point is: 5k+ LOC may be a problem, but it's not as simple as "just split it" or "just move functions ~arbitrarily~ into more files". I say "arbitrarily" unless you find independent pieces that can be meaningfully split. Likewise, Makefile.am contains the build configuration of the project. It's strongly coupled and to some degree, you need to see it as a whole. At least, `make` needs to see it as a whole, which SUBDIRS= does not. This will be more relevant, when actually adding unit tests and integrating tests into `make check`. The lack of not integrating tests in `make` (and not having unit tests) is IMO bad and should be addressed. I claim, with one make file, that will fit beautifully together (maintainable). The unit tests will be in another directories, which requires correct dependencies between tests/unit/ and src/. With recursive make (SUBDIRS=) it's only "everything under src/ must build first", which hampers parallel make and does not express dependencies correctly. Branch [2] shows how this would look with unit tests. [2] https://gitlab.freedesktop.org/thaller/nftables/-/commits/th/no-recursive-make Thomas
