| Message ID | 20231019130057.2719096-5-thaller@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Florian Westphal |
| Headers | show |
| Series | no recursive make | expand |
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. Thanks.
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.
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
On Thu, Nov 02, 2023 at 03:03:42PM +0100, Thomas Haller wrote: > 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. Yes, because it is too large and you needed this 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? We __rarely__ need to look at all these things at once. I do not even remember when it was last time I needed to look into these Makefiles, well now after this churning^H^H^H^H^H^H^H update :-) > 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. Yes, it is as long as this email :-) > -- > > 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. No, some .c files are really asking for split, and I have patches to start doing so. > 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`. Yes, it is great, really. But this is completely inconsistent with what we have in other existing Netfilter trees. > 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. No please, do not follow that path. I have to run `make distcheck` to create tarballs, and I do not have to wait to run all tests to do so. Please do not couple tests with make process.
On Thu, 2023-11-02 at 17:05 +0100, Pablo Neira Ayuso wrote: > On Thu, Nov 02, 2023 at 03:03:42PM +0100, Thomas Haller wrote: > > 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. > > Yes, because it is too large and you needed this 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? > > We __rarely__ need to look at all these things at once. > > I do not even remember when it was last time I needed to look into > these Makefiles, well now after this churning^H^H^H^H^H^H^H update :- > ) > > > 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. > > Yes, it is as long as this email :-) Sorry about that. I figured, when the long commit message didn't convince you, a long mail should :) > > > -- > > > > 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. > > No, some .c files are really asking for split, and I have patches to > start doing so. Cool. > > > 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`. > > Yes, it is great, really. > > But this is completely inconsistent with what we have in other > existing > Netfilter trees. That would also be fixable, by adjusting those trees (I'd volunteer). The question is what's better, and not what the projects copy-pasted since 1995 do. > > > 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. > > No please, do not follow that path. > > I have to run `make distcheck` to create tarballs, and I do not have > to wait to run all tests to do so. > > Please do not couple tests with make process. On the branch, those tests work and it's convenient to run them and reasonably fast! `make -j distcheck` takes 59 seconds on my machine. When doing a release, one(!) minute should be invested to run all tests again. And when tests fail, that should block the release. On the other hand, the tests could be easily excluded during `make distcheck`. So that's not really an argument. But not running tests is IMO not favorable. Anyway. Please play around with what's on current master. I still hope you come to like it. Thomas
Keep in mind for the concerns wrt large Makefiles, you can do 'include' with automake too which keeps things flat in terms of what automake generates and what make ultimately runs.
On Fri, Nov 03, 2023 at 11:37:16AM +0000, Sam James wrote: > Keep in mind for the concerns wrt large Makefiles, you can do 'include' > with automake too which keeps things flat in terms of what automake > generates and what make ultimately runs. That would be good to restore if it helps restore modularity to some extend. A few notes: - Python support also depends on one option. - There is nftables/tests/build/run-tests.sh to test for all configurare options, I am not sure if Thomas run this test.
On Thu, Nov 02, 2023 at 05:53:43PM +0100, Thomas Haller wrote: > On Thu, 2023-11-02 at 17:05 +0100, Pablo Neira Ayuso wrote: [...] > > But this is completely inconsistent with what we have in other > > existing Netfilter trees. > > That would also be fixable, by adjusting those trees (I'd volunteer). > > The question is what's better, and not what the projects copy-pasted > since 1995 do. I don't think it is worth the update, maybe some simplification to remove silly things such as Makefile.am with one singleton line, but there are better things to look at IMO. [...] > > Please do not couple tests with make process. > > On the branch, those tests work and it's convenient to run them and > reasonably fast! `make -j distcheck` takes 59 seconds on my machine. CI is what is missing, a single run is proving not giving much in return these days after your improvements. The recent bugs that were uncovered have been spotted by running this is a loop, and also exercising standalone 30s-stress from Florian for many hours. A few minutes does not harm (I can check how long it takes on my AMD Epyc box that I use for testing), but CI might provide more reliable information on what is going on. Thanks.
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 = +############################################################################### + pkginclude_HEADERS = \ include/nftables/libnftables.h \ $(NULL) @@ -72,11 +74,48 @@ noinst_HEADERS = \ \ $(NULL) +############################################################################### + SUBDIRS = src \ - files \ doc \ examples +############################################################################### + +dist_pkgdata_DATA = \ + files/nftables/all-in-one.nft \ + files/nftables/arp-filter.nft \ + files/nftables/bridge-filter.nft \ + files/nftables/inet-filter.nft \ + files/nftables/inet-nat.nft \ + files/nftables/ipv4-filter.nft \ + files/nftables/ipv4-mangle.nft \ + files/nftables/ipv4-nat.nft \ + files/nftables/ipv4-raw.nft \ + files/nftables/ipv6-filter.nft \ + files/nftables/ipv6-mangle.nft \ + files/nftables/ipv6-nat.nft \ + files/nftables/ipv6-raw.nft \ + files/nftables/netdev-ingress.nft \ + $(NULL) + +pkgdocdir = ${docdir}/examples + +dist_pkgdoc_SCRIPTS = \ + files/examples/ct_helpers.nft \ + files/examples/load_balancing.nft \ + files/examples/secmark.nft \ + files/examples/sets_and_maps.nft \ + $(NULL) + +pkgsysconfdir = ${sysconfdir}/nftables/osf + +dist_pkgsysconf_DATA = \ + files/osf/pf.os \ + $(NULL) + +############################################################################### + EXTRA_DIST += \ py/pyproject.toml \ py/setup.cfg \ @@ -86,6 +125,8 @@ EXTRA_DIST += \ py/src/schema.json \ $(NULL) +############################################################################### + EXTRA_DIST += \ files \ tests \ diff --git a/configure.ac b/configure.ac index 389efbe9f730..23581f91341d 100644 --- a/configure.ac +++ b/configure.ac @@ -118,10 +118,6 @@ AC_CONFIG_FILES([ \ Makefile \ libnftables.pc \ src/Makefile \ - files/Makefile \ - files/examples/Makefile \ - files/nftables/Makefile \ - files/osf/Makefile \ doc/Makefile \ examples/Makefile \ ]) diff --git a/files/Makefile.am b/files/Makefile.am deleted file mode 100644 index 7deec1512977..000000000000 --- a/files/Makefile.am +++ /dev/null @@ -1,3 +0,0 @@ -SUBDIRS = nftables \ - examples \ - osf diff --git a/files/examples/Makefile.am b/files/examples/Makefile.am deleted file mode 100644 index b29e9f614203..000000000000 --- a/files/examples/Makefile.am +++ /dev/null @@ -1,5 +0,0 @@ -pkgdocdir = ${docdir}/examples -dist_pkgdoc_SCRIPTS = ct_helpers.nft \ - load_balancing.nft \ - secmark.nft \ - sets_and_maps.nft diff --git a/files/nftables/Makefile.am b/files/nftables/Makefile.am deleted file mode 100644 index ee88dd896743..000000000000 --- a/files/nftables/Makefile.am +++ /dev/null @@ -1,14 +0,0 @@ -dist_pkgdata_DATA = all-in-one.nft \ - arp-filter.nft \ - bridge-filter.nft \ - inet-filter.nft \ - inet-nat.nft \ - ipv4-filter.nft \ - ipv4-mangle.nft \ - ipv4-nat.nft \ - ipv4-raw.nft \ - ipv6-filter.nft \ - ipv6-mangle.nft \ - ipv6-nat.nft \ - ipv6-raw.nft \ - netdev-ingress.nft diff --git a/files/osf/Makefile.am b/files/osf/Makefile.am deleted file mode 100644 index d80196dd7388..000000000000 --- a/files/osf/Makefile.am +++ /dev/null @@ -1,2 +0,0 @@ -pkgsysconfdir = ${sysconfdir}/nftables/osf -dist_pkgsysconf_DATA = pf.os
Merge the Makefile.am under "files/" into the toplevel Makefile.am. This is a step in the effort of dropping recursive make. Signed-off-by: Thomas Haller <thaller@redhat.com> --- Makefile.am | 43 +++++++++++++++++++++++++++++++++++++- configure.ac | 4 ---- files/Makefile.am | 3 --- files/examples/Makefile.am | 5 ----- files/nftables/Makefile.am | 14 ------------- files/osf/Makefile.am | 2 -- 6 files changed, 42 insertions(+), 29 deletions(-) delete mode 100644 files/Makefile.am delete mode 100644 files/examples/Makefile.am delete mode 100644 files/nftables/Makefile.am delete mode 100644 files/osf/Makefile.am