Re: [PATCH] MAKE: Fix parallel builds; .o files; .gitignore; new makedocs

[Stefano Brivio <sbrivio@redhat.com>]

Thanks for the patch, and sorry for the delay!

First off, as I was mentioning on IRC: it would help to have this as
two/three patches -- not just because it's easier to review, but also
because the Makefile changes have, I guess, a relatively high risk of
introducing some regressions, so keeping it simple for 'git bisect'
would also be a plus here.

If you want to split this in your working tree, you can do something
like:

  git reset HEAD~

meaning: reset (the git state, not the file contents) to the commit
before head (~ is a shortcut for "just before"). Then 'git add' your
pieces back -- and if you have two changes to the Makefile you want to
keep separated, go with 'git add -i', which lets you add just parts.

If that's not your HEAD anymore, 'git rebase -i' to something before
this commit, and do the same git reset/git add trick, then 'git rebase
--continue'.

Another general comment (about the title): this doesn't actually fix
parallel builds... it makes builds more parallel.

Now, to the changes:

On Wed, 28 Jun 2023 16:07:28 +0200
KuhnChris <kuhnchris+git@kuhnchris.eu> wrote:

> From: KuhnChris <kuhnchris@kuhnchris.eu>
> 
> ---
>  .gitignore  |  1 +
>  Makefile    | 51 +++++++++++++++++++++++++--------------------------
>  makedocs.pl | 19 +++++++++++++++++++
>  3 files changed, 45 insertions(+), 26 deletions(-)
>  create mode 100644 makedocs.pl
> 
> diff --git a/.gitignore b/.gitignore
> index d3d0e2c..caeafa5 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -1,4 +1,5 @@
>  *~
> +*.o
>  /passt
>  /passt.avx2
>  /pasta
> diff --git a/Makefile b/Makefile
> index a5256f5..4c99771 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -100,29 +100,41 @@ else
>  BIN := passt pasta qrap
>  endif
>  
> -all: $(BIN) $(MANPAGES) docs
> +.NOTPARALLEL: seccomp.h
> +all: seccomp.h $(BIN) $(MANPAGES) docs
> +
> +PASST_OBJS = $(PASST_SRCS:.c=.o)
> +PASST_AVX2_OBJS = $(PASST_SRCS:.c=.avx2.o)
> +OBJS = $(SRCS:.c=.o)
> +QRAP_OBJS = $(QRAP_SRCS:.c=.o)
> +AVXFLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops

This all looks correct to me, but for some reason, on a machine
with 12 (free-ish) CPU threads, 'make -j1' takes the same time as
'make -j10', and seems to lead to the same amount of parallel gcc
processes... no idea why, yet. I would have expected actual parallelism
resulting from this.

By the way, build time (after a make clean) is the same as without this
change, with -j1 or -j10.

I know you're introducing this to avoid a full rebuild if only some
source files changed, which is an advantage in any case, but I'm
missing something...

>  
>  static: FLAGS += -static -DGLIBC_NO_STATIC_NSS
> -static: clean all
> +static: clean seccomp.h all
>  
>  seccomp.h: seccomp.sh $(PASST_SRCS) $(PASST_HEADERS)
>  	@ EXTRA_SYSCALLS="$(EXTRA_SYSCALLS)" ARCH="$(TARGET_ARCH)" CC="$(CC)" ./seccomp.sh $(PASST_SRCS) $(PASST_HEADERS)
>  
> -passt: $(PASST_SRCS) $(HEADERS)
> -	$(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o passt $(LDFLAGS)
> +.c.o:
> +	$(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) -c $< -o $@
> +
> +$(PASST_AVX2_OBJS): seccomp.h
> +	$(CC) $(AVXFLAGS) $(FLAGS) $(CFLAGS) $(CPPFLAGS) -c $(@:.avx2.o=.c) -o $@
> +
> +passt: $(PASST_OBJS) seccomp.h
> +	$(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(PASST_OBJS) -o passt $(LDFLAGS)
>  
> -passt.avx2: FLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops
> -passt.avx2: $(PASST_SRCS) $(HEADERS)
> -	$(CC) $(filter-out -O2,$(FLAGS)) $(CFLAGS) $(CPPFLAGS) \
> -		$(PASST_SRCS) -o passt.avx2 $(LDFLAGS)
> +passt.avx2: $(PASST_AVX2_OBJS) $(HEADERS) seccomp.h
> +	$(CC) $(filter-out -O2,$(FLAGS)) $(CFLAGS) $(CPPFLAGS) $(AVXFLAGS) \
> +		$(PASST_AVX2_OBJS) -o passt.avx2 $(LDFLAGS)
>  
> -passt.avx2: passt
> +passt.avx2: passt seccomp.h
>  
>  pasta.avx2 pasta.1 pasta: pasta%: passt%
>  	ln -sf $< $@
>  
> -qrap: $(QRAP_SRCS) passt.h
> -	$(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(QRAP_SRCS) -o qrap $(LDFLAGS)
> +qrap: $(QRAP_OBJS) passt.h seccomp.h
> +	$(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(QRAP_OBJS) -o qrap $(LDFLAGS)
>  
>  valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction	\
>  			    getpid gettid kill clock_gettime mmap	\
> @@ -170,21 +182,8 @@ pkgs: static
>  # other way around: the web version should be obtained by adding HTML and
>  # JavaScript portions to a plain Markdown, instead. However, cgit needs to use
>  # a file in the git tree. Find a better way around this.
> -docs: README.md
> -	@(								\
> -		skip=0;							\
> -		while read l; do					\
> -			case $$l in					\
> -			"## Demo")	exit 0		;;		\
> -			"<!"*)				;;		\
> -			"</"*)		skip=1		;;		\
> -			"<"*)		skip=2		;;		\
> -			esac;						\
> -									\
> -			[ $$skip -eq 0 ]	&& echo "$$l";		\
> -			[ $$skip -eq 1 ]	&& skip=0;		\
> -		done < README.md;					\
> -	) > README.plain.md
> +docs:

The output is much more complete now (and the Makefile cleaner).

> +	[ ! -e doc/ ]; mkdir docs; perl makedocs.pl > docs/README.plain.md

Shouldn't we (naturally) use Makefile dependencies for this? Also note
that you're moving the README.plain.md file to docs/, so you would also
need to change the install target:

        cp -d README.plain.md $(DESTDIR)$(docdir)/README.md

...but, anyway, I think it would make more sense to keep the output in
the root directory (simply because README.md is already there, so one
can more easily spot the "plain" version).

>  
>  # Checkers currently disabled for clang-tidy:
>  # - llvmlibc-restrict-system-libc-headers
> diff --git a/makedocs.pl b/makedocs.pl
> new file mode 100644

This should probably have 0755 permissions, it's an interpreted
executable after all.

> index 0000000..295e1d9
> --- /dev/null
> +++ b/makedocs.pl

This makes it sound like it does more than just stripping away the HTML
stuff. Perhaps make_plain_readme.pl, or plain_readme.pl,
strip_html.pl... something like that?

> @@ -0,0 +1,19 @@

Missing license header:

# SPDX-License-Identifier: GPL-2.0-or-later

and:

#!/usr/bin/perl

> +use strict;
> +
> +my $str = '';
> +my $regex = qr/(<[^\/].*?>.*?<\/.*?>\n?)|(<\/div>)|(<\/p>)/msp;
> +my $subst = '';
> +
> +
> +local $/=undef;
> +open FILE, '<', 'README.md' or die "Can't open file $!";
> +my $file_content = <FILE>;
> +close FILE;
> +#print "Source: $file_content\n";
> +my $result = $file_content =~ s/$regex//rg;
> +
> +my $regex2 = qr/\n[ \n]{3,}\n/msp;
> +$result = $result =~ s/$regex2/\n\n/rg;
> +
> +#print "The result of the substitution is: $result\n";
> +print "$result\n";

This looks good to me... I'm not sure if you intended to leave in those
comments, but I'm fine either way.

-- 
Stefano