Re: [ptxdist] [RFC 3/4] libptxdist: add a function to find unreferenced source directories

[Roland Hieber <rhi@pengutronix.de>]

On Thu, Aug 22, 2019 at 05:38:19PM +0200, Michael Olbrich wrote:
> On Thu, Aug 22, 2019 at 12:42:40PM +0200, Roland Hieber wrote:
> > A recurring task for package version bumps is to make sure the
> > respective patch stacks are ported too. Add functionality to find
> > forgotten patch stacks.
> > 
> > We do that by first going through all available packages and getting
> > their respective <PKG> variables. Then we list all available patch
> > directories and take only the last path component. We compare both of
> > those lists to find out which patch directories are not referenced by
> > any package, i.e. print the lines that were part of the second list
> > but not of the first list.
> > 
> > This adds additional coreutils dependencies for comm and uniq.
> > 
> > Unfortunately, printing all those make variables is quite slow. On my
> > current BSP it takes approximately
> > 
> > -  2.7 seconds for ptxd_make /print-PACKAGES etc…
> > - 37.1 seconds for ptxd_make /print-PTX_MAP_TO_PACKAGE_cross-nasm etc…
> > - 37.4 seconds for ptxd_make /print-GLIB /print-KEYUTILS /print-IPTABLES etc…
> > -  0.5 seconds for listing all existing patch dirs
> > - 0.01 seconds to compare both lists
> > 
> > Signed-off-by: Roland Hieber <rhi@pengutronix.de>
> > ---
> >  Makefile.in           |  2 ++
> >  configure.ac          |  2 ++
> >  scripts/libptxdist.sh | 50 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 54 insertions(+)
> > 
> > diff --git a/Makefile.in b/Makefile.in
> > index d337b9ec552b..bc8bc98e2492 100644
> > --- a/Makefile.in
> > +++ b/Makefile.in
> > @@ -48,6 +48,7 @@ environment:
> >  	@ln -sf @GNU_cat@ "$(abs_srcdir)/bin/cat"
> >  	@ln -sf @GNU_chmod@ "$(abs_srcdir)/bin/chmod"
> >  	@ln -sf @GNU_chown@ "$(abs_srcdir)/bin/chown"
> > +	@ln -sf @GNU_comm@ "$(abs_srcdir)/bin/comm"
> >  	@ln -sf @GNU_cp@ "$(abs_srcdir)/bin/cp"
> >  	@ln -sf @GNU_dirname@ "$(abs_srcdir)/bin/dirname"
> >  	@ln -sf @PTXDIST_FILE@ "$(abs_srcdir)/bin/file"
> > @@ -66,6 +67,7 @@ environment:
> >  	@ln -sf @GNU_stat@ "$(abs_srcdir)/bin/stat"
> >  	@ln -sf @GNU_tar@ "$(abs_srcdir)/bin/tar"
> >  	@ln -sf @GNU_tty@ "$(abs_srcdir)/bin/tty"
> > +	@ln -sf @GNU_uniq@ "$(abs_srcdir)/bin/uniq"
> >  	@ln -sf @GNU_xargs@ "$(abs_srcdir)/bin/xargs"
> >  	@ln -sf @MAKE@ "$(abs_srcdir)/bin/make"
> >  	@ln -sf @PYTHON@ "$(abs_srcdir)/bin/python"
> > diff --git a/configure.ac b/configure.ac
> > index 0549c38da7ba..cb3552ac8ea0 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -182,6 +182,7 @@ AC_DEFUN([GNU_TOOL],
> >  GNU_TOOL(cat, coreutils)
> >  GNU_TOOL(chmod, coreutils)
> >  GNU_TOOL(chown, coreutils)
> > +GNU_TOOL(comm, coreutils)
> >  GNU_TOOL(cp, coreutils)
> >  GNU_TOOL(dirname, coreutils)
> >  GNU_TOOL(install, coreutils)
> > @@ -198,6 +199,7 @@ GNU_TOOL(sort, coreutils)
> >  GNU_TOOL(stat, coreutils)
> >  GNU_TOOL(touch, coreutils)
> >  GNU_TOOL(tty, coreutils)
> > +GNU_TOOL(uniq, coreutils)
> >  GNU_TOOL(tar, tar)
> >  GNU_TOOL(find, findutils)
> >  GNU_TOOL(xargs, findutils)
> > diff --git a/scripts/libptxdist.sh b/scripts/libptxdist.sh
> > index 8c6fbc073f64..d93f61f56e97 100644
> > --- a/scripts/libptxdist.sh
> > +++ b/scripts/libptxdist.sh
> > @@ -927,3 +927,53 @@ ptxd_ipkg_rev_smaller() {
> >  
> >  	ptxd_error "packets $1 and $2 have the same revision"
> >  }
> > +
> > +#
> > +#
> > +ptxd_find_old_patchdirs() {
> > +
> > +	local pkgslistfile=${PTXDIST_TEMPDIR}/check_old_patchdirs_pkgs
> > +	local patchdirfile=${PTXDIST_TEMPDIR}/check_old_patchdirs_dirs
> > +	local print_PKGS=""
> > +	local print_pkgs=""
> > +	local i
> > +
> > +	# get content of all existing <PKG> make variables
> > +	local pkgvars="/print-PACKAGES /print-EXTRA_PACKAGES /print-LAZY_PACKAGES"
> 
> unused?
> 
> > +	for i in $(ptxd_make -k \
> > +		/print-PACKAGES /print-EXTRA_PACKAGES /print-LAZY_PACKAGES \
> > +		/print-HOST_PACKAGES /print-CROSS_PACKAGES  \
> > +		/print-PACKAGES- /print-EXTRA_PACKAGES- /print-LAZY_PACKAGES- \
> > +		/print-HOST_PACKAGES- /print-CROSS_PACKAGES-  \
> > +		); do
> > +		print_PKGS="/print-PTX_MAP_TO_PACKAGE_${i} ${print_PKGS}"
> > +	done
> > +	for i in $(ptxd_make -k ${print_PKGS}); do
> > +		print_pkgs="/print-${i} ${print_pkgs}"
> > +	done
> > +	ptxd_make -k ${print_pkgs} | tr ' \t' '\n' | sort | uniq | grep -v '^$' > ${pkgslistfile}
> 
> You can change the make code as well. Add this next to the print stuff:
> 
> # like /print-% but expand twice
> /printnext-%: FORCE
> 	@:$(foreach v,$(or $(filter $(*),$(.VARIABLES)),$(if $(filter k,$(MAKEFLAGS)),$(*),$(error $(*) undefined))),\
> 		$(info $(if $(filter 1,$(PTXDIST_VERBOSE)),$($(v))=)$($($(v)))))
> 
> Then you can just do:
> 
> 	ptxd_make -k /printnext-PTX_MAP_TO_PACKAGE_% | sort | uniq | grep -v '^$' > ${pkgslistfile}
> 
> That should be a lot faster.
> 
> > +
> > +	# get list of existing patch dirs
> > +	echo > ${patchdirfile}
> > +	ptxd_in_path PTXDIST_PATH_PATCHES '*'
> 
> 	ptxd_in_path PTXDIST_PATH_PATCHES '*/'
> 
> Then you can skip filtering out autogen.sh, I think.
> 
> > +	echo "${ptxd_reply[@]}" | tr ' \t' '\n' | while read dir; do
> 
> for dir in "${ptxd_reply[@]}"; do
> 
> > +		echo $(basename ${dir})
> > +	done | sort | uniq | grep -v '^$\|^autogen.sh$' > ${patchdirfile}
> 
> There should be no empty lines.
> 
> > +
> > +	# get list of dirs for which no package exists
> > +	comm --nocheck-order -13 ${pkgslistfile} ${patchdirfile} | while read dir; do
> > +		ptxd_in_path PTXDIST_PATH_PATCHES ${dir}
> > +		ptxd_print_path ${ptxd_reply}
> > +	done | while read line; do
> > +		# annotate packages that are built specially or their version
> > +		# depends on enabling kconfig entries
> > +		case i in
> > +			*alsa-lib*|*at91bootstrap*|*barebox*)
> 
> I think we can 'fix' the alsa-lib false positives.
> And maybe we should just remove the at91bootstrap patches, they are really
> old anyways.
> But why barebox? There should be no old patches, right?

Ah, yes. I was thinking at91bootstrap can be disabled, so the version
number is empty, and the same can be said for barebox too. But the only
location where Barebox patches occur is at the BSP level, and I guess
it doesn't happen that someone switches from barebox to at91bootstrap
with patches lying around locally.

> 
> > +				echo "${line} (possible false positive)"
> > +				;;
> > +			*)
> > +				echo ${line}
> 
> I'd like to get a non-zero return value when at least one issue was found.

All the other comments make sense for me to, I'll address them in v2.

 - Roland

> 
> Michael
> 
> > +				;;
> > +		esac
> > +	done
> > +}
> > -- 
> > 2.20.1
> > 
> > 
> > _______________________________________________
> > ptxdist mailing list
> > ptxdist@pengutronix.de
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> _______________________________________________
> ptxdist mailing list
> ptxdist@pengutronix.de

-- 
Roland Hieber                     | r.hieber@pengutronix.de     |
Pengutronix e.K.                  | https://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim | Phone: +49-5121-206917-5086 |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de