[PATCH nft v2 0/7] no recursive make

[PATCH nft v2 0/7] no recursive make

[Thomas Haller]

This is a RESEND of v1. Only minor adjustments to the commit message and
rebasing.

[Quote commit message from first patch]

Switch from recursive-make to a single top-level Makefile. This is the
first step, the following patches will continue this.

Unlike meson's subdir() or C's #include, automake's SUBDIRS= does not
include a Makefile. Instead, it calls `make -C $dir`.

  https://www.gnu.org/software/make/manual/html_node/Recursion.html
  https://www.gnu.org/software/automake/manual/html_node/Subdirectories.html

See also, "Recursive Make Considered Harmful".

  https://accu.org/journals/overload/14/71/miller_2004/

This has several problems, which we an avoid with a single Makefile:

- recursive-make is harder to maintain and understand as a whole.
  Recursive-make makes sense, when there are truly independent
  sub-projects. Which is not the case here. The project needs to be
  considered as a whole and not one directory at a time. When
  we add unit tests (which we should), those would reside in separate
  directories but have dependencies between directories. With a single
  Makefile, we see all at once. The build setup has an inherent complexity,
  and that complexity is not necessarily reduced by splitting it into more files.
  On the contrary it helps to have it all in once place, provided that it's
  sensibly structured, named and organized.

- typing `make` prints irrelevant "Entering directory" messages. So much
  so, that at the end of the build, the terminal is filled with such
  messages and we have to scroll to see what even happened.

- with recursive-make, during build we see:

    make[3]: Entering directory '.../nftables/src'
      CC       meta.lo
    meta.c:13:2: error: #warning hello test [-Werror=cpp]
       13 | #warning hello test
          |  ^~~~~~~

  With a single Makefile we get

      CC       src/meta.lo
    src/meta.c:13:2: error: #warning hello test [-Werror=cpp]
       13 | #warning hello test
          |  ^~~~~~~

  This shows the full filename -- assuming that the developer works from
  the top level directory. The full name is useful, for example to
  copy+paste into the terminal.

- single Makefile is also faster:

    $ make && perf stat -r 200 -B make -j

  I measure 35msec vs. 80msec.

- recursive-make limits parallel make. You have to craft the SUBDIRS= in
  the correct order. The dependencies between directories are limited,
  as make only sees "LDADD = $(top_builddir)/src/libnftables.la" and
  not the deeper dependencies for the library.

- I presume, some people like recursive-make because of `make -C $subdir`
  to only rebuild one directory. Rebuilding the entire tree is already very
  fast, so this feature seems not relevant. Also, as dependency handling
  is limited, we might wrongly not rebuild a target. For example,

        make check
        touch src/meta.c
        make -C examples check

  does not rebuild "examples/nft-json-file".
  What we now can do with single Makefile (and better than before), is
  `make examples/nft-json-file`, which works as desired and rebuilds all
  dependencies.

<<<<

Thomas Haller (7):
  gitignore: ignore ".dirstamp" files
  build: no recursive-make for "include/**/Makefile.am"
  build: no recursive make for "py/Makefile.am"
  build: no recursive make for "files/**/Makefile.am"
  build: no recursive make for "src/Makefile.am"
  build: no recursive make for "examples/Makefile.am"
  build: no recursive make for "doc/Makefile.am"

 .gitignore                                 |   1 +
 Make_global.am                             |  21 --
 Makefile.am                                | 415 ++++++++++++++++++++-
 configure.ac                               |  16 -
 doc/Makefile.am                            |  30 --
 examples/Makefile.am                       |   6 -
 files/Makefile.am                          |   3 -
 files/examples/Makefile.am                 |   5 -
 files/nftables/Makefile.am                 |  14 -
 files/osf/Makefile.am                      |   2 -
 include/Makefile.am                        |  43 ---
 include/linux/Makefile.am                  |  12 -
 include/linux/netfilter/Makefile.am        |  10 -
 include/linux/netfilter_arp/Makefile.am    |   1 -
 include/linux/netfilter_bridge/Makefile.am |   1 -
 include/linux/netfilter_ipv4/Makefile.am   |   1 -
 include/linux/netfilter_ipv6/Makefile.am   |   1 -
 include/nftables/Makefile.am               |   1 -
 py/Makefile.am                             |   1 -
 src/Makefile.am                            | 123 ------
 20 files changed, 407 insertions(+), 300 deletions(-)
 delete mode 100644 Make_global.am
 delete mode 100644 doc/Makefile.am
 delete mode 100644 examples/Makefile.am
 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
 delete mode 100644 include/Makefile.am
 delete mode 100644 include/linux/Makefile.am
 delete mode 100644 include/linux/netfilter/Makefile.am
 delete mode 100644 include/linux/netfilter_arp/Makefile.am
 delete mode 100644 include/linux/netfilter_bridge/Makefile.am
 delete mode 100644 include/linux/netfilter_ipv4/Makefile.am
 delete mode 100644 include/linux/netfilter_ipv6/Makefile.am
 delete mode 100644 include/nftables/Makefile.am
 delete mode 100644 py/Makefile.am
 delete mode 100644 src/Makefile.am

-- 
2.41.0

[PATCH nft v2 1/7] gitignore: ignore ".dirstamp" files

[Thomas Haller]

Those will be generated by automake, once the recursive Makefiles
are gone.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 2c12c4409720..a62e31f31c6b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,5 +1,6 @@
 # Generated by autoconf/configure/automake
 *.m4
+.dirstamp
 Makefile
 Makefile.in
 stamp-h1
-- 
2.41.0

[PATCH nft v2 2/7] build: no recursive-make for "include/**/Makefile.am"

[Thomas Haller]

Switch from recursive-make to a single top-level Makefile. This is the
first step, the following patches will continue this.

Unlike meson's subdir() or C's #include, automake's SUBDIRS= does not
include a Makefile. Instead, it calls `make -C $dir`.

  https://www.gnu.org/software/make/manual/html_node/Recursion.html
  https://www.gnu.org/software/automake/manual/html_node/Subdirectories.html

See also, "Recursive Make Considered Harmful".

  https://accu.org/journals/overload/14/71/miller_2004/

This has several problems, which we an avoid with a single Makefile:

- recursive-make is harder to maintain and understand as a whole.
  Recursive-make makes sense, when there are truly independent
  sub-projects. Which is not the case here. The project needs to be
  considered as a whole and not one directory at a time. When
  we add unit tests (which we should), those would reside in separate
  directories but have dependencies between directories. With a single
  Makefile, we see all at once. The build setup has an inherent complexity,
  and that complexity is not necessarily reduced by splitting it into more files.
  On the contrary it helps to have it all in once place, provided that it's
  sensibly structured, named and organized.

- typing `make` prints irrelevant "Entering directory" messages. So much
  so, that at the end of the build, the terminal is filled with such
  messages and we have to scroll to see what even happened.

- with recursive-make, during build we see:

    make[3]: Entering directory '.../nftables/src'
      CC       meta.lo
    meta.c:13:2: error: #warning hello test [-Werror=cpp]
       13 | #warning hello test
          |  ^~~~~~~

  With a single Makefile we get

      CC       src/meta.lo
    src/meta.c:13:2: error: #warning hello test [-Werror=cpp]
       13 | #warning hello test
          |  ^~~~~~~

  This shows the full filename -- assuming that the developer works from
  the top level directory. The full name is useful, for example to
  copy+paste into the terminal.

- single Makefile is also faster:

    $ make && perf stat -r 200 -B make -j

  I measure 35msec vs. 80msec.

- recursive-make limits parallel make. You have to craft the SUBDIRS= in
  the correct order. The dependencies between directories are limited,
  as make only sees "LDADD = $(top_builddir)/src/libnftables.la" and
  not the deeper dependencies for the library.

- I presume, some people like recursive-make because of `make -C $subdir`
  to only rebuild one directory. Rebuilding the entire tree is already very
  fast, so this feature seems not relevant. Also, as dependency handling
  is limited, we might wrongly not rebuild a target. For example,

        make check
        touch src/meta.c
        make -C examples check

  does not rebuild "examples/nft-json-file".
  What we now can do with single Makefile (and better than before), is
  `make examples/nft-json-file`, which works as desired and rebuilds all
  dependencies.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 Makefile.am                                | 75 +++++++++++++++++++++-
 configure.ac                               |  8 ---
 include/Makefile.am                        | 43 -------------
 include/linux/Makefile.am                  | 12 ----
 include/linux/netfilter/Makefile.am        | 10 ---
 include/linux/netfilter_arp/Makefile.am    |  1 -
 include/linux/netfilter_bridge/Makefile.am |  1 -
 include/linux/netfilter_ipv4/Makefile.am   |  1 -
 include/linux/netfilter_ipv6/Makefile.am   |  1 -
 include/nftables/Makefile.am               |  1 -
 10 files changed, 72 insertions(+), 81 deletions(-)
 delete mode 100644 include/Makefile.am
 delete mode 100644 include/linux/Makefile.am
 delete mode 100644 include/linux/netfilter/Makefile.am
 delete mode 100644 include/linux/netfilter_arp/Makefile.am
 delete mode 100644 include/linux/netfilter_bridge/Makefile.am
 delete mode 100644 include/linux/netfilter_ipv4/Makefile.am
 delete mode 100644 include/linux/netfilter_ipv6/Makefile.am
 delete mode 100644 include/nftables/Makefile.am

diff --git a/Makefile.am b/Makefile.am
index 84c3c366b86a..bfc64ebbed71 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,7 +1,76 @@
-ACLOCAL_AMFLAGS	= -I m4
+ACLOCAL_AMFLAGS = -I m4
 
-SUBDIRS = 	src	\
-		include	\
+pkginclude_HEADERS = \
+	include/nftables/libnftables.h \
+	$(NULL)
+
+noinst_HEADERS = \
+	\
+	include/linux/netfilter.h \
+	include/linux/netfilter/nf_conntrack_common.h \
+	include/linux/netfilter/nf_conntrack_tuple_common.h \
+	include/linux/netfilter/nf_log.h \
+	include/linux/netfilter/nf_nat.h \
+	include/linux/netfilter/nf_synproxy.h \
+	include/linux/netfilter/nf_tables.h \
+	include/linux/netfilter/nf_tables_compat.h \
+	include/linux/netfilter/nfnetlink.h \
+	include/linux/netfilter/nfnetlink_hook.h \
+	include/linux/netfilter/nfnetlink_osf.h \
+	include/linux/netfilter_arp.h \
+	include/linux/netfilter_arp/arp_tables.h \
+	include/linux/netfilter_bridge.h \
+	include/linux/netfilter_bridge/ebtables.h \
+	include/linux/netfilter_decnet.h \
+	include/linux/netfilter_ipv4.h \
+	include/linux/netfilter_ipv4/ip_tables.h \
+	include/linux/netfilter_ipv6.h \
+	include/linux/netfilter_ipv6/ip6_tables.h \
+	\
+	include/cache.h \
+	include/cli.h \
+	include/cmd.h \
+	include/ct.h \
+	include/datatype.h \
+	include/dccpopt.h \
+	include/erec.h \
+	include/expression.h \
+	include/exthdr.h \
+	include/fib.h \
+	include/gmputil.h \
+	include/hash.h \
+	include/headers.h \
+	include/iface.h \
+	include/intervals.h \
+	include/ipopt.h \
+	include/json.h \
+	include/list.h \
+	include/meta.h \
+	include/mini-gmp.h \
+	include/misspell.h \
+	include/mnl.h \
+	include/netlink.h \
+	include/nft.h \
+	include/nftables.h \
+	include/numgen.h \
+	include/osf.h \
+	include/owner.h \
+	include/parser.h \
+	include/payload.h \
+	include/proto.h \
+	include/rt.h \
+	include/rule.h \
+	include/sctp_chunk.h \
+	include/socket.h \
+	include/statement.h \
+	include/tcpopt.h \
+	include/utils.h \
+	include/xfrm.h \
+	include/xt.h \
+	\
+	$(NULL)
+
+SUBDIRS =	src	\
 		files	\
 		doc	\
 		examples\
diff --git a/configure.ac b/configure.ac
index 7bd33bdcc2cf..67ca50fddf67 100644
--- a/configure.ac
+++ b/configure.ac
@@ -118,14 +118,6 @@ AC_CONFIG_FILES([					\
 		Makefile				\
 		libnftables.pc				\
 		src/Makefile				\
-		include/Makefile			\
-		include/nftables/Makefile		\
-		include/linux/Makefile			\
-		include/linux/netfilter/Makefile	\
-		include/linux/netfilter_arp/Makefile	\
-		include/linux/netfilter_bridge/Makefile	\
-		include/linux/netfilter_ipv4/Makefile	\
-		include/linux/netfilter_ipv6/Makefile	\
 		files/Makefile				\
 		files/examples/Makefile			\
 		files/nftables/Makefile			\
diff --git a/include/Makefile.am b/include/Makefile.am
deleted file mode 100644
index 162807b03900..000000000000
--- a/include/Makefile.am
+++ /dev/null
@@ -1,43 +0,0 @@
-SUBDIRS =		linux		\
-			nftables
-
-noinst_HEADERS =	cli.h		\
-			cache.h		\
-			cmd.h		\
-			datatype.h	\
-			dccpopt.h	\
-			nft.h	\
-			expression.h	\
-			fib.h		\
-			hash.h		\
-			intervals.h	\
-			ipopt.h		\
-			json.h		\
-			mini-gmp.h	\
-			gmputil.h	\
-			iface.h		\
-			mnl.h		\
-			nftables.h	\
-			payload.h	\
-			tcpopt.h	\
-			statement.h	\
-			ct.h		\
-			erec.h		\
-			exthdr.h	\
-			headers.h	\
-			list.h		\
-			meta.h		\
-			misspell.h	\
-			numgen.h	\
-			netlink.h	\
-			osf.h		\
-			owner.h		\
-			parser.h	\
-			proto.h		\
-			sctp_chunk.h	\
-			socket.h	\
-			rule.h		\
-			rt.h		\
-			utils.h		\
-			xfrm.h		\
-			xt.h
diff --git a/include/linux/Makefile.am b/include/linux/Makefile.am
deleted file mode 100644
index eb9fc4e4a6bd..000000000000
--- a/include/linux/Makefile.am
+++ /dev/null
@@ -1,12 +0,0 @@
-SUBDIRS =		netfilter		\
-			netfilter_arp		\
-			netfilter_bridge	\
-			netfilter_ipv4		\
-			netfilter_ipv6
-
-noinst_HEADERS =	netfilter_arp.h		\
-			netfilter_bridge.h	\
-			netfilter_decnet.h	\
-			netfilter.h		\
-			netfilter_ipv4.h	\
-			netfilter_ipv6.h
diff --git a/include/linux/netfilter/Makefile.am b/include/linux/netfilter/Makefile.am
deleted file mode 100644
index 22f66a7e1ebf..000000000000
--- a/include/linux/netfilter/Makefile.am
+++ /dev/null
@@ -1,10 +0,0 @@
-noinst_HEADERS = 	nf_conntrack_common.h		\
-			nf_conntrack_tuple_common.h	\
-			nf_log.h			\
-			nf_nat.h			\
-			nf_tables.h			\
-			nf_tables_compat.h		\
-			nf_synproxy.h			\
-			nfnetlink_osf.h			\
-			nfnetlink_hook.h		\
-			nfnetlink.h
diff --git a/include/linux/netfilter_arp/Makefile.am b/include/linux/netfilter_arp/Makefile.am
deleted file mode 100644
index 0a16c1abd072..000000000000
--- a/include/linux/netfilter_arp/Makefile.am
+++ /dev/null
@@ -1 +0,0 @@
-noinst_HEADERS =	arp_tables.h
diff --git a/include/linux/netfilter_bridge/Makefile.am b/include/linux/netfilter_bridge/Makefile.am
deleted file mode 100644
index d2e8b38b196e..000000000000
--- a/include/linux/netfilter_bridge/Makefile.am
+++ /dev/null
@@ -1 +0,0 @@
-noinst_HEADERS =	ebtables.h
diff --git a/include/linux/netfilter_ipv4/Makefile.am b/include/linux/netfilter_ipv4/Makefile.am
deleted file mode 100644
index fec42533ab81..000000000000
--- a/include/linux/netfilter_ipv4/Makefile.am
+++ /dev/null
@@ -1 +0,0 @@
-noinst_HEADERS =	ip_tables.h
diff --git a/include/linux/netfilter_ipv6/Makefile.am b/include/linux/netfilter_ipv6/Makefile.am
deleted file mode 100644
index bec6c3f16694..000000000000
--- a/include/linux/netfilter_ipv6/Makefile.am
+++ /dev/null
@@ -1 +0,0 @@
-noinst_HEADERS =	ip6_tables.h
diff --git a/include/nftables/Makefile.am b/include/nftables/Makefile.am
deleted file mode 100644
index 5cfb0c6c5a92..000000000000
--- a/include/nftables/Makefile.am
+++ /dev/null
@@ -1 +0,0 @@
-pkginclude_HEADERS = libnftables.h
-- 
2.41.0

[PATCH nft v2 3/7] build: no recursive make for "py/Makefile.am"

[Thomas Haller]

Merge the Makefile.am under "py/" 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    | 20 ++++++++++++++++----
 configure.ac   |  1 -
 py/Makefile.am |  1 -
 3 files changed, 16 insertions(+), 6 deletions(-)
 delete mode 100644 py/Makefile.am

diff --git a/Makefile.am b/Makefile.am
index bfc64ebbed71..8b8de7bd141a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,5 +1,7 @@
 ACLOCAL_AMFLAGS = -I m4
 
+EXTRA_DIST =
+
 pkginclude_HEADERS = \
 	include/nftables/libnftables.h \
 	$(NULL)
@@ -73,11 +75,21 @@ noinst_HEADERS = \
 SUBDIRS =	src	\
 		files	\
 		doc	\
-		examples\
-		py
+		examples
+
+EXTRA_DIST += \
+	py/pyproject.toml \
+	py/setup.cfg \
+	py/setup.py \
+	py/src/__init__.py \
+	py/src/nftables.py \
+	py/src/schema.json \
+	$(NULL)
 
-EXTRA_DIST =	tests	\
-		files
+EXTRA_DIST += \
+	files \
+	tests \
+	$(NULL)
 
 pkgconfigdir = $(libdir)/pkgconfig
 pkgconfig_DATA = libnftables.pc
diff --git a/configure.ac b/configure.ac
index 67ca50fddf67..389efbe9f730 100644
--- a/configure.ac
+++ b/configure.ac
@@ -123,7 +123,6 @@ AC_CONFIG_FILES([					\
 		files/nftables/Makefile			\
 		files/osf/Makefile			\
 		doc/Makefile				\
-		py/Makefile				\
 		examples/Makefile			\
 		])
 AC_OUTPUT
diff --git a/py/Makefile.am b/py/Makefile.am
deleted file mode 100644
index 76aa082f8709..000000000000
--- a/py/Makefile.am
+++ /dev/null
@@ -1 +0,0 @@
-EXTRA_DIST = pyproject.toml setup.cfg setup.py src
-- 
2.41.0

[PATCH nft v2 4/7] build: no recursive make for "files/**/Makefile.am"

[Thomas Haller]

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

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
-- 
2.41.0

[PATCH nft v2 5/7] build: no recursive make for "src/Makefile.am"

[Thomas Haller]

Merge the Makefile.am under "src/" into the toplevel Makefile.am. This
is a step in the effort of dropping recursive make.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 Make_global.am  |  21 -----
 Makefile.am     | 214 +++++++++++++++++++++++++++++++++++++++++++++++-
 configure.ac    |   1 -
 src/Makefile.am | 123 ----------------------------
 4 files changed, 213 insertions(+), 146 deletions(-)
 delete mode 100644 Make_global.am
 delete mode 100644 src/Makefile.am

diff --git a/Make_global.am b/Make_global.am
deleted file mode 100644
index 5bb541f61388..000000000000
--- a/Make_global.am
+++ /dev/null
@@ -1,21 +0,0 @@
-# This is _NOT_ the library release version, it's an API version.
-# Extracted from Chapter 6 "Library interface versions" of the libtool docs.
-#
-# <snippet>
-# Here are a set of rules to help you update your library version information:
-#
-# 1. Start with version information of `0:0:0' for each libtool library.
-# 2. Update the version information only immediately before a public release
-# of your software. More frequent updates are unnecessary, and only guarantee
-# that the current interface number gets larger faster.
-# 3. If the library source code has changed at all since the last update,
-# then increment revision (`c:r:a' becomes `c:r+1:a').
-# 4. If any interfaces have been added, removed, or changed since the last
-# update, increment current, and set revision to 0.
-# 5. If any interfaces have been added since the last public release, then
-# increment age.
-# 6. If any interfaces have been removed since the last public release, then
-# set age to 0.
-# </snippet>
-#
-libnftables_LIBVERSION=2:0:1
diff --git a/Makefile.am b/Makefile.am
index 83f25dd8574b..b89d60e32d8c 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,6 +1,34 @@
+# This is _NOT_ the library release version, it's an API version.
+# Extracted from Chapter 6 "Library interface versions" of the libtool docs.
+#
+# <snippet>
+# Here are a set of rules to help you update your library version information:
+#
+# 1. Start with version information of `0:0:0' for each libtool library.
+# 2. Update the version information only immediately before a public release
+# of your software. More frequent updates are unnecessary, and only guarantee
+# that the current interface number gets larger faster.
+# 3. If the library source code has changed at all since the last update,
+# then increment revision (`c:r:a' becomes `c:r+1:a').
+# 4. If any interfaces have been added, removed, or changed since the last
+# update, increment current, and set revision to 0.
+# 5. If any interfaces have been added since the last public release, then
+# increment age.
+# 6. If any interfaces have been removed since the last public release, then
+# set age to 0.
+# </snippet>
+#
+libnftables_LIBVERSION = 2:0:1
+
+###############################################################################
+
 ACLOCAL_AMFLAGS = -I m4
 
 EXTRA_DIST =
+BUILT_SOURCES =
+lib_LTLIBRARIES =
+noinst_LTLIBRARIES =
+sbin_PROGRAMS =
 
 ###############################################################################
 
@@ -76,7 +104,191 @@ noinst_HEADERS = \
 
 ###############################################################################
 
-SUBDIRS =	src	\
+AM_CPPFLAGS = \
+	"-I$(srcdir)/include" \
+	"-DDEFAULT_INCLUDE_PATH=\"${sysconfdir}\"" \
+	$(LIBMNL_CFLAGS) \
+	$(LIBNFTNL_CFLAGS) \
+	$(NULL)
+
+if BUILD_DEBUG
+AM_CPPFLAGS += -g -DDEBUG
+endif
+if BUILD_XTABLES
+AM_CPPFLAGS += $(XTABLES_CFLAGS)
+endif
+if BUILD_MINIGMP
+AM_CPPFLAGS += -DHAVE_MINIGMP
+endif
+if BUILD_JSON
+AM_CPPFLAGS += -DHAVE_JSON
+endif
+if BUILD_XTABLES
+AM_CPPFLAGS += -DHAVE_XTABLES
+endif
+
+AM_CFLAGS = \
+	-Wall \
+	\
+	-Waggregate-return \
+	-Wbad-function-cast \
+	-Wcast-align \
+	-Wdeclaration-after-statement \
+	-Wformat-nonliteral \
+	-Wformat-security \
+	-Winit-self \
+	-Wmissing-declarations \
+	-Wmissing-format-attribute \
+	-Wmissing-prototypes \
+	-Wsign-compare \
+	-Wstrict-prototypes \
+	-Wundef \
+	-Wunused \
+	-Wwrite-strings \
+	\
+	$(GCC_FVISIBILITY_HIDDEN) \
+	\
+	$(NULL)
+
+AM_YFLAGS = -d -Wno-yacc
+
+###############################################################################
+
+BUILT_SOURCES += src/parser_bison.h
+
+# yacc and lex generate dirty code
+noinst_LTLIBRARIES += src/libparser.la
+
+src_libparser_la_SOURCES = \
+	src/parser_bison.y \
+	src/scanner.l \
+	$(NULL)
+
+src_libparser_la_CFLAGS = \
+	$(AM_CFLAGS) \
+	-Wno-implicit-function-declaration \
+	-Wno-missing-declarations \
+	-Wno-missing-prototypes \
+	-Wno-nested-externs \
+	-Wno-redundant-decls \
+	-Wno-undef \
+	-Wno-unused-but-set-variable \
+	$(NULL)
+
+###############################################################################
+
+if BUILD_MINIGMP
+
+noinst_LTLIBRARIES += src/libminigmp.la
+
+src_libminigmp_la_SOURCES = src/mini-gmp.c
+
+src_libminigmp_la_CFLAGS = \
+	$(AM_CFLAGS) \
+	-Wno-sign-compare \
+	$(NULL)
+
+endif
+
+###############################################################################
+
+lib_LTLIBRARIES += src/libnftables.la
+
+src_libnftables_la_SOURCES = \
+	src/libnftables.map \
+	\
+	src/cache.c \
+	src/cmd.c \
+	src/ct.c \
+	src/datatype.c \
+	src/dccpopt.c \
+	src/erec.c \
+	src/evaluate.c \
+	src/expression.c \
+	src/exthdr.c \
+	src/fib.c \
+	src/gmputil.c \
+	src/hash.c \
+	src/iface.c \
+	src/intervals.c \
+	src/ipopt.c \
+	src/libnftables.c \
+	src/mergesort.c \
+	src/meta.c \
+	src/misspell.c \
+	src/mnl.c \
+	src/monitor.c \
+	src/netlink.c \
+	src/netlink_delinearize.c \
+	src/netlink_linearize.c \
+	src/nfnl_osf.c \
+	src/nftutils.c \
+	src/nftutils.h \
+	src/numgen.c \
+	src/optimize.c \
+	src/osf.c \
+	src/owner.c \
+	src/payload.c \
+	src/print.c \
+	src/proto.c \
+	src/rt.c \
+	src/rule.c \
+	src/sctp_chunk.c \
+	src/segtree.c \
+	src/socket.c \
+	src/statement.c \
+	src/tcpopt.c \
+	src/utils.c \
+	src/xfrm.c \
+	$(NULL)
+
+src_libnftables_la_SOURCES += src/xt.c
+
+if BUILD_JSON
+src_libnftables_la_SOURCES += \
+	src/json.c \
+	src/parser_json.c \
+	$(NULL)
+endif
+
+src_libnftables_la_LDFLAGS = \
+	-version-info "${libnftables_LIBVERSION}" \
+	-Wl,--version-script="$(srcdir)/src//libnftables.map" \
+	$(NULL)
+
+src_libnftables_la_LIBADD = \
+	$(LIBMNL_LIBS) \
+	$(LIBNFTNL_LIBS) \
+	src/libparser.la \
+	$(NULL)
+
+if BUILD_MINIGMP
+src_libnftables_la_LIBADD += src/libminigmp.la
+endif
+
+if BUILD_XTABLES
+src_libnftables_la_LIBADD += $(XTABLES_LIBS)
+endif
+
+if BUILD_JSON
+src_libnftables_la_LIBADD += $(JANSSON_LIBS)
+endif
+
+###############################################################################
+
+sbin_PROGRAMS += src/nft
+
+src_nft_SOURCES = src/main.c
+
+if BUILD_CLI
+src_nft_SOURCES += src/cli.c
+endif
+
+src_nft_LDADD = src/libnftables.la
+
+###############################################################################
+
+SUBDIRS = \
 		doc	\
 		examples
 
diff --git a/configure.ac b/configure.ac
index 23581f91341d..79024e49ab28 100644
--- a/configure.ac
+++ b/configure.ac
@@ -117,7 +117,6 @@ AC_CHECK_DECLS([getprotobyname_r, getprotobynumber_r, getservbyport_r], [], [],
 AC_CONFIG_FILES([					\
 		Makefile				\
 		libnftables.pc				\
-		src/Makefile				\
 		doc/Makefile				\
 		examples/Makefile			\
 		])
diff --git a/src/Makefile.am b/src/Makefile.am
deleted file mode 100644
index 63a4ef43dae3..000000000000
--- a/src/Makefile.am
+++ /dev/null
@@ -1,123 +0,0 @@
-include $(top_srcdir)/Make_global.am
-
-sbin_PROGRAMS = nft
-
-AM_CPPFLAGS = -I$(top_srcdir)/include
-AM_CPPFLAGS += -DDEFAULT_INCLUDE_PATH="\"${sysconfdir}\"" \
-		${LIBMNL_CFLAGS} ${LIBNFTNL_CFLAGS}
-if BUILD_DEBUG
-AM_CPPFLAGS += -g -DDEBUG
-endif
-if BUILD_XTABLES
-AM_CPPFLAGS += ${XTABLES_CFLAGS}
-endif
-if BUILD_MINIGMP
-AM_CPPFLAGS += -DHAVE_MINIGMP
-endif
-if BUILD_JSON
-AM_CPPFLAGS += -DHAVE_JSON
-endif
-if BUILD_XTABLES
-AM_CPPFLAGS += -DHAVE_XTABLES
-endif
-
-AM_CFLAGS = -Wall								\
-	    -Wstrict-prototypes -Wmissing-prototypes -Wmissing-declarations	\
-	    -Wdeclaration-after-statement -Wsign-compare -Winit-self		\
-	    -Wformat-nonliteral -Wformat-security -Wmissing-format-attribute	\
-	    -Wcast-align -Wundef -Wbad-function-cast				\
-	    -Waggregate-return -Wunused -Wwrite-strings ${GCC_FVISIBILITY_HIDDEN}
-
-
-AM_YFLAGS = -d -Wno-yacc
-
-BUILT_SOURCES = parser_bison.h
-
-lib_LTLIBRARIES = libnftables.la
-
-libnftables_la_SOURCES =			\
-		rule.c				\
-		statement.c			\
-		cache.c				\
-		cmd.c				\
-		datatype.c			\
-		expression.c			\
-		evaluate.c			\
-		proto.c				\
-		payload.c			\
-		exthdr.c			\
-		fib.c				\
-		hash.c				\
-		intervals.c			\
-		ipopt.c				\
-		meta.c				\
-		rt.c				\
-		numgen.c			\
-		ct.c				\
-		xfrm.c				\
-		netlink.c			\
-		netlink_linearize.c		\
-		netlink_delinearize.c		\
-		misspell.c			\
-		monitor.c			\
-		owner.c				\
-		segtree.c			\
-		gmputil.c			\
-		utils.c				\
-		nftutils.c			\
-		nftutils.h			\
-		erec.c				\
-		mnl.c				\
-		iface.c				\
-		mergesort.c			\
-		optimize.c			\
-		osf.c				\
-		nfnl_osf.c			\
-		tcpopt.c			\
-		socket.c			\
-		print.c				\
-		sctp_chunk.c			\
-		dccpopt.c			\
-		libnftables.c			\
-		libnftables.map
-
-# yacc and lex generate dirty code
-noinst_LTLIBRARIES = libparser.la
-libparser_la_SOURCES = parser_bison.y scanner.l
-libparser_la_CFLAGS = ${AM_CFLAGS} \
-		      -Wno-missing-prototypes \
-		      -Wno-missing-declarations \
-		      -Wno-implicit-function-declaration \
-		      -Wno-unused-but-set-variable \
-		      -Wno-nested-externs \
-		      -Wno-undef \
-		      -Wno-redundant-decls
-
-libnftables_la_LIBADD = ${LIBMNL_LIBS} ${LIBNFTNL_LIBS} libparser.la
-libnftables_la_LDFLAGS = -version-info ${libnftables_LIBVERSION} \
-			 -Wl,--version-script=$(srcdir)/libnftables.map
-
-if BUILD_MINIGMP
-noinst_LTLIBRARIES += libminigmp.la
-libminigmp_la_SOURCES = mini-gmp.c
-libminigmp_la_CFLAGS = ${AM_CFLAGS} -Wno-sign-compare
-libnftables_la_LIBADD += libminigmp.la
-endif
-
-libnftables_la_SOURCES += xt.c
-if BUILD_XTABLES
-libnftables_la_LIBADD += ${XTABLES_LIBS}
-endif
-
-nft_SOURCES = main.c
-
-if BUILD_CLI
-nft_SOURCES += cli.c
-endif
-
-if BUILD_JSON
-libnftables_la_SOURCES += json.c parser_json.c
-libnftables_la_LIBADD += ${JANSSON_LIBS}
-endif
-
-nft_LDADD = libnftables.la
-- 
2.41.0

[PATCH nft v2 6/7] build: no recursive make for "examples/Makefile.am"

[Thomas Haller]

Merge the Makefile.am under "examples/" 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          | 19 ++++++++++++++++---
 configure.ac         |  1 -
 examples/Makefile.am |  6 ------
 3 files changed, 16 insertions(+), 10 deletions(-)
 delete mode 100644 examples/Makefile.am

diff --git a/Makefile.am b/Makefile.am
index b89d60e32d8c..af82f021203a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -26,9 +26,11 @@ ACLOCAL_AMFLAGS = -I m4
 
 EXTRA_DIST =
 BUILT_SOURCES =
+LDADD =
 lib_LTLIBRARIES =
 noinst_LTLIBRARIES =
 sbin_PROGRAMS =
+check_PROGRAMS =
 
 ###############################################################################
 
@@ -288,9 +290,20 @@ src_nft_LDADD = src/libnftables.la
 
 ###############################################################################
 
-SUBDIRS = \
-		doc	\
-		examples
+SUBDIRS = doc
+
+###############################################################################
+
+
+check_PROGRAMS += examples/nft-buffer
+
+examples_nft_buffer_AM_CPPFLAGS = -I$(srcdir)/include
+examples_nft_buffer_LDADD = src/libnftables.la
+
+check_PROGRAMS += examples/nft-json-file
+
+examples_nft_json_file_AM_CPPFLAGS = -I$(srcdir)/include
+examples_nft_json_file_LDADD = src/libnftables.la
 
 ###############################################################################
 
diff --git a/configure.ac b/configure.ac
index 79024e49ab28..c5e4113898a0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -118,7 +118,6 @@ AC_CONFIG_FILES([					\
 		Makefile				\
 		libnftables.pc				\
 		doc/Makefile				\
-		examples/Makefile			\
 		])
 AC_OUTPUT
 
diff --git a/examples/Makefile.am b/examples/Makefile.am
deleted file mode 100644
index 3b8b0b6708dc..000000000000
--- a/examples/Makefile.am
+++ /dev/null
@@ -1,6 +0,0 @@
-check_PROGRAMS	= nft-buffer		\
-		  nft-json-file
-
-AM_CPPFLAGS = -I$(top_srcdir)/include
-
-LDADD = $(top_builddir)/src/libnftables.la
-- 
2.41.0

[PATCH nft v2 7/7] build: no recursive make for "doc/Makefile.am"

[Thomas Haller]

Merge the Makefile.am under "doc/" 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     | 60 ++++++++++++++++++++++++++++++++++++++++++++-----
 configure.ac    |  1 -
 doc/Makefile.am | 30 -------------------------
 3 files changed, 55 insertions(+), 36 deletions(-)
 delete mode 100644 doc/Makefile.am

diff --git a/Makefile.am b/Makefile.am
index af82f021203a..0ed831a19e95 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -31,6 +31,8 @@ lib_LTLIBRARIES =
 noinst_LTLIBRARIES =
 sbin_PROGRAMS =
 check_PROGRAMS =
+dist_man_MANS =
+CLEANFILES =
 
 ###############################################################################
 
@@ -290,11 +292,6 @@ src_nft_LDADD = src/libnftables.la
 
 ###############################################################################
 
-SUBDIRS = doc
-
-###############################################################################
-
-
 check_PROGRAMS += examples/nft-buffer
 
 examples_nft_buffer_AM_CPPFLAGS = -I$(srcdir)/include
@@ -307,6 +304,59 @@ examples_nft_json_file_LDADD = src/libnftables.la
 
 ###############################################################################
 
+if BUILD_MAN
+
+dist_man_MANS += \
+	doc/nft.8 \
+	doc/libnftables-json.5 \
+	doc/libnftables.3 \
+	$(NULL)
+
+A2X_OPTS_MANPAGE = \
+	-L \
+	--doctype manpage \
+	--format manpage \
+	-D "${builddir}/doc" \
+	$(NULL)
+
+ASCIIDOC_MAIN = doc/nft.txt
+
+ASCIIDOC_INCLUDES = \
+	doc/data-types.txt \
+	doc/payload-expression.txt \
+	doc/primary-expression.txt \
+	doc/stateful-objects.txt \
+	doc/statements.txt \
+	$(NULL)
+
+ASCIIDOCS = \
+	$(ASCIIDOC_MAIN) \
+	$(ASCIIDOC_INCLUDES) \
+	$(NULL)
+
+EXTRA_DIST += \
+	$(ASCIIDOCS) \
+	doc/libnftables-json.adoc \
+	doc/libnftables.adoc \
+	$(NULL)
+
+CLEANFILES += doc/*~
+
+doc/nft.8: $(ASCIIDOCS)
+	$(AM_V_GEN)$(A2X) $(A2X_OPTS_MANPAGE) $<
+
+.adoc.3:
+	$(AM_V_GEN)$(A2X) $(A2X_OPTS_MANPAGE) $<
+
+.adoc.5:
+	$(AM_V_GEN)$(A2X) $(A2X_OPTS_MANPAGE) $<
+
+MAINTAINERCLEANFILES = ${dist_man_MANS}
+
+endif
+
+###############################################################################
+
 dist_pkgdata_DATA = \
 	files/nftables/all-in-one.nft \
 	files/nftables/arp-filter.nft \
diff --git a/configure.ac b/configure.ac
index c5e4113898a0..724a4ae726c1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -117,7 +117,6 @@ AC_CHECK_DECLS([getprotobyname_r, getprotobynumber_r, getservbyport_r], [], [],
 AC_CONFIG_FILES([					\
 		Makefile				\
 		libnftables.pc				\
-		doc/Makefile				\
 		])
 AC_OUTPUT
 
diff --git a/doc/Makefile.am b/doc/Makefile.am
deleted file mode 100644
index b43cb08d2d14..000000000000
--- a/doc/Makefile.am
+++ /dev/null
@@ -1,30 +0,0 @@
-if BUILD_MAN
-dist_man_MANS = nft.8 libnftables-json.5 libnftables.3
-
-A2X_OPTS_MANPAGE = -L --doctype manpage --format manpage -D ${builddir}
-
-ASCIIDOC_MAIN = nft.txt
-ASCIIDOC_INCLUDES = \
-       data-types.txt \
-       payload-expression.txt \
-       primary-expression.txt \
-       stateful-objects.txt \
-       statements.txt
-ASCIIDOCS = ${ASCIIDOC_MAIN} ${ASCIIDOC_INCLUDES}
-
-EXTRA_DIST = ${ASCIIDOCS} libnftables-json.adoc libnftables.adoc
-
-CLEANFILES = \
-	*~
-
-nft.8: ${ASCIIDOCS}
-	${AM_V_GEN}${A2X} ${A2X_OPTS_MANPAGE} $<
-
-.adoc.3:
-	${AM_V_GEN}${A2X} ${A2X_OPTS_MANPAGE} $<
-
-.adoc.5:
-	${AM_V_GEN}${A2X} ${A2X_OPTS_MANPAGE} $<
-
-MAINTAINERCLEANFILES = ${dist_man_MANS}
-endif
-- 
2.41.0

Re: [PATCH nft v2 0/7] no recursive make

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> This is a RESEND of v1. Only minor adjustments to the commit message and
> rebasing.

All applied, thanks Thomas.

Re: [PATCH nft v2 4/7] build: no recursive make for "files/**/Makefile.am"

[Pablo Neira Ayuso]

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.

Re: [PATCH nft v2 4/7] build: no recursive make for "files/**/Makefile.am"

[Pablo Neira Ayuso]

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.

Re: [PATCH nft v2 4/7] build: no recursive make for "files/**/Makefile.am"

[Thomas Haller]

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

Re: [PATCH nft v2 4/7] build: no recursive make for "files/**/Makefile.am"

[Pablo Neira Ayuso]

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.

Re: [PATCH nft v2 4/7] build: no recursive make for "files/**/Makefile.am"

[Thomas Haller]

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

Re: [PATCH nft v2 4/7] build: no recursive make for "files/**/Makefile.am"

[Sam James]

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.

Re: [PATCH nft v2 4/7] build: no recursive make for "files/**/Makefile.am"

[Pablo Neira Ayuso]

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.

Re: [PATCH nft v2 4/7] build: no recursive make for "files/**/Makefile.am"

[Pablo Neira Ayuso]

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.