[v6,7/8] devtools: remove check-includes script

Message ID 20210127173330.1671341-8-bruce.richardson@intel.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series add checking of header includes |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Bruce Richardson Jan. 27, 2021, 5:33 p.m. UTC
  The check-includes script allowed checking header files in a given
directory to ensure that each header compiled alone without requiring
any other header inclusions.

With header checking now being done by the chkincs app in the build
system this script can be removed.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 MAINTAINERS                            |   1 -
 devtools/check-includes.sh             | 259 -------------------------
 doc/guides/rel_notes/release_21_02.rst |   3 +
 3 files changed, 3 insertions(+), 260 deletions(-)
 delete mode 100755 devtools/check-includes.sh
  

Comments

Thomas Monjalon Jan. 28, 2021, 11:10 a.m. UTC | #1
27/01/2021 18:33, Bruce Richardson:
> +* The ``check-includes.sh`` script for checking DPDK header files has been
> +  removed, being replaced by the ``check_includes`` build option described above.

How do they compare?
Is the new test checking C++ compliance? pedantic?
  
Bruce Richardson Jan. 28, 2021, 11:38 a.m. UTC | #2
On Thu, Jan 28, 2021 at 12:10:41PM +0100, Thomas Monjalon wrote:
> 27/01/2021 18:33, Bruce Richardson:
> > +* The ``check-includes.sh`` script for checking DPDK header files has
> > been +  removed, being replaced by the ``check_includes`` build option
> > described above.
> 
> How do they compare?  Is the new test checking C++ compliance? pedantic?
> 
The new support does not do either of those things - though pedantic checks
should be fairly easy to do - so I'm ok to drop this patch and not remove
the script.

Unfortunately, in my limited testing of it, the script has some usability
issues now that on build all the lib includes are no longer copies to a
single include folder.  There is no automatic detection of include paths
for other dependent headers, which means that to run the tool on e.g.
ethdev lib, you need to set up the CFLAGS to include the paths to the net,
eal, kvargs, meter, etc.  libraries.  Then again, it may work well on an
installed DPDK instance for testing, but the fact that nobody has
complained about it being hard to use before indicates that it is not being
used, and if it's not being used I felt it as well to just drop it.

/Bruce
  
Thomas Monjalon Jan. 28, 2021, 2:05 p.m. UTC | #3
28/01/2021 12:38, Bruce Richardson:
> On Thu, Jan 28, 2021 at 12:10:41PM +0100, Thomas Monjalon wrote:
> > 27/01/2021 18:33, Bruce Richardson:
> > > +* The ``check-includes.sh`` script for checking DPDK header files has
> > > been +  removed, being replaced by the ``check_includes`` build option
> > > described above.
> > 
> > How do they compare?  Is the new test checking C++ compliance? pedantic?
> > 
> The new support does not do either of those things - though pedantic checks
> should be fairly easy to do - so I'm ok to drop this patch and not remove
> the script.
> 
> Unfortunately, in my limited testing of it, the script has some usability
> issues now that on build all the lib includes are no longer copies to a
> single include folder.  There is no automatic detection of include paths
> for other dependent headers, which means that to run the tool on e.g.
> ethdev lib, you need to set up the CFLAGS to include the paths to the net,
> eal, kvargs, meter, etc.  libraries.  Then again, it may work well on an
> installed DPDK instance for testing, but the fact that nobody has
> complained about it being hard to use before indicates that it is not being
> used, and if it's not being used I felt it as well to just drop it.

I am OK to drop it.
My question is more about improving the new tool to reach
the initial goals of the old script.
  

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index d4f9ebe46d..8498b402db 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -81,7 +81,6 @@  F: devtools/check-dup-includes.sh
 F: devtools/check-maintainers.sh
 F: devtools/check-forbidden-tokens.awk
 F: devtools/check-git-log.sh
-F: devtools/check-includes.sh
 F: devtools/check-spdx-tag.sh
 F: devtools/check-symbol-maps.sh
 F: devtools/checkpatches.sh
diff --git a/devtools/check-includes.sh b/devtools/check-includes.sh
deleted file mode 100755
index 940cf0eb2a..0000000000
--- a/devtools/check-includes.sh
+++ /dev/null
@@ -1,259 +0,0 @@ 
-#!/bin/sh -e
-# SPDX-License-Identifier: BSD-3-Clause
-# Copyright 2016 6WIND S.A.
-
-# This script checks that header files in a given directory do not miss
-# dependencies when included on their own, do not conflict and accept being
-# compiled with the strictest possible flags.
-#
-# Files are looked up in the directory provided as the first argument,
-# otherwise build/include by default.
-#
-# Recognized environment variables:
-#
-# VERBOSE=1 is the same as -v.
-#
-# QUIET=1 is the same as -q.
-#
-# SUMMARY=1 is the same as -s.
-#
-# CC, CPPFLAGS, CFLAGS, CXX, CXXFLAGS are taken into account.
-#
-# PEDANTIC_CFLAGS, PEDANTIC_CXXFLAGS and PEDANTIC_CPPFLAGS provide strict
-# C/C++ compilation flags.
-#
-# IGNORE contains a list of globbing patterns matching files (relative to the
-# include directory) to avoid. It is set by default to known DPDK headers
-# which must not be included on their own.
-#
-# IGNORE_CXX provides additional files for C++.
-
-while getopts hqvs arg; do
-	case $arg in
-	h)
-		cat <<EOF
-usage: $0 [-h] [-q] [-v] [-s] [DIR]
-
-This script checks that header files in a given directory do not miss
-dependencies when included on their own, do not conflict and accept being
-compiled with the strictest possible flags.
-
-  -h    display this help and exit
-  -q    quiet mode, disable normal output
-  -v    show command lines being executed
-  -s    show summary
-
-With no DIR, default to build/include.
-
-Any failed header check yields a nonzero exit status.
-EOF
-		exit
-		;;
-	q)
-		QUIET=1
-		;;
-	v)
-		VERBOSE=1
-		;;
-	s)
-		SUMMARY=1
-		;;
-	*)
-		exit 1
-		;;
-	esac
-done
-
-shift $(($OPTIND - 1))
-
-include_dir=${1:-build/include}
-
-: ${PEDANTIC_CFLAGS=-std=c99 -pedantic -Wall -Wextra -Werror}
-: ${PEDANTIC_CXXFLAGS=}
-: ${PEDANTIC_CPPFLAGS=-D_XOPEN_SOURCE=600}
-: ${CC:=cc}
-: ${CXX:=c++}
-: ${IGNORE= \
-	'rte_atomic_32.h' \
-	'rte_atomic_64.h' \
-	'rte_byteorder_32.h' \
-	'rte_byteorder_64.h' \
-	'generic/*' \
-	'rte_vhost.h' \
-	'rte_eth_vhost.h' \
-	'rte_eal_interrupts.h' \
-}
-: ${IGNORE_CXX= \
-	'rte_vhost.h' \
-	'rte_eth_vhost.h' \
-}
-
-temp_cc=$(mktemp -t dpdk.${0##*/}.XXX.c)
-pass_cc=
-failures_cc=0
-
-temp_cxx=$(mktemp -t dpdk.${0##*/}.XXX.cc)
-pass_cxx=
-failures_cxx=0
-
-# Process output parameters.
-
-[ "$QUIET" = 1 ] &&
-exec 1> /dev/null
-
-[ "$VERBOSE" = 1 ] &&
-output ()
-{
-	local CCV
-	local CXXV
-
-	shift
-	CCV=$CC
-	CXXV=$CXX
-	CC="echo $CC" CXX="echo $CXX" "$@"
-	CC=$CCV
-	CXX=$CXXV
-
-	"$@"
-} ||
-output ()
-{
-
-	printf '  %s\n' "$1"
-	shift
-	"$@"
-}
-
-trap 'rm -f "$temp_cc" "$temp_cxx"' EXIT
-
-compile_cc ()
-{
-	${CC} -I"$include_dir" \
-		${PEDANTIC_CPPFLAGS} ${CPPFLAGS} \
-		${PEDANTIC_CFLAGS} ${CFLAGS} \
-		-c -o /dev/null "${temp_cc}"
-}
-
-compile_cxx ()
-{
-	${CXX} -I"$include_dir" \
-		${PEDANTIC_CPPFLAGS} ${CPPFLAGS} \
-		${PEDANTIC_CXXFLAGS} ${CXXFLAGS} \
-		-c -o /dev/null "${temp_cxx}"
-}
-
-ignore ()
-{
-	file="$1"
-	shift
-	while [ $# -ne 0 ]; do
-		case "$file" in
-		$1)
-			return 0
-			;;
-		esac
-		shift
-	done
-	return 1
-}
-
-# Check C/C++ compilation for each header file.
-
-while read -r path
-do
-	file=${path#$include_dir}
-	file=${file##/}
-	if ignore "$file" $IGNORE; then
-		output "SKIP $file" :
-		continue
-	fi
-	if printf "\
-#include <%s>
-
-int main(void)
-{
-	return 0;
-}
-" "$file" > "$temp_cc" &&
-		output "CC $file" compile_cc
-	then
-		pass_cc="$pass_cc $file"
-	else
-		failures_cc=$(($failures_cc + 1))
-	fi
-	if ignore "$file" $IGNORE_CXX; then
-		output "SKIP CXX $file" :
-		continue
-	fi
-	if printf "\
-#include <%s>
-
-int main()
-{
-}
-" "$file" > "$temp_cxx" &&
-		output "CXX $file" compile_cxx
-	then
-		pass_cxx="$pass_cxx $file"
-	else
-		failures_cxx=$(($failures_cxx + 1))
-	fi
-done <<EOF
-$(find "$include_dir" -name '*.h')
-EOF
-
-# Check C compilation with all includes.
-
-: > "$temp_cc" &&
-for file in $pass_cc; do
-	printf "\
-#include <%s>
-" "$file" >> $temp_cc
-done
-if printf "\
-int main(void)
-{
-	return 0;
-}
-" >> "$temp_cc" &&
-	output "CC (all includes that did not fail)" compile_cc
-then
-	:
-else
-	failures_cc=$(($failures_cc + 1))
-fi
-
-# Check C++ compilation with all includes.
-
-: > "$temp_cxx" &&
-for file in $pass_cxx; do
-	printf "\
-#include <%s>
-" "$file" >> $temp_cxx
-done
-if printf "\
-int main()
-{
-}
-" >> "$temp_cxx" &&
-	output "CXX (all includes that did not fail)" compile_cxx
-then
-	:
-else
-	failures_cxx=$(($failures_cxx + 1))
-fi
-
-# Report results.
-
-if [ "$SUMMARY" = 1 ]; then
-	printf "\
-Summary:
- %u failure(s) for C using '%s'.
- %u failure(s) for C++ using '%s'.
-" $failures_cc "$CC" $failures_cxx "$CXX" 1>&2
-fi
-
-# Exit with nonzero status if there are failures.
-
-[ $failures_cc -eq 0 ] &&
-[ $failures_cxx -eq 0 ]
diff --git a/doc/guides/rel_notes/release_21_02.rst b/doc/guides/rel_notes/release_21_02.rst
index 245d1a6473..c73ec6ede0 100644
--- a/doc/guides/rel_notes/release_21_02.rst
+++ b/doc/guides/rel_notes/release_21_02.rst
@@ -154,6 +154,9 @@  Removed Items
   ``ethdev_pci.h`` respectively in the source tree, to reflect the fact that
   they are non-public headers.
 
+* The ``check-includes.sh`` script for checking DPDK header files has been
+  removed, being replaced by the ``check_includes`` build option described above.
+
 
 API Changes
 -----------