Header checker scripts should fail on failure

Started by Thomas Munroover 3 years ago3 messages
#1Thomas Munro
thomas.munro@gmail.com
1 attachment(s)

Hi,

I thought commit 81b9f23c9c8 had my back, but nope, we still need to
make CI turn red if "headerscheck" and "cpluspluscheck" don't like our
patches (crake in the build farm should be a secondary defence...).
See attached.

Attachments:

fail.patchtext/x-patch; charset=US-ASCII; name=fail.patchDownload
diff --git a/src/tools/pginclude/cpluspluscheck b/src/tools/pginclude/cpluspluscheck
index eb06ee0111..2d3b785342 100755
--- a/src/tools/pginclude/cpluspluscheck
+++ b/src/tools/pginclude/cpluspluscheck
@@ -52,6 +52,8 @@ tmp=`mktemp -d /tmp/$me.XXXXXX`
 
 trap 'rm -rf $tmp' 0 1 2 3 15
 
+exit_status=0
+
 # Scan all of src/ and contrib/ for header files.
 for f in `cd "$srcdir" && find src contrib -name '*.h' -print`
 do
@@ -167,9 +169,13 @@ do
 	esac
 
 	# Run the test.
-	${CXX:-g++} -I $builddir -I $srcdir \
+	if ! ${CXX:-g++} -I $builddir -I $srcdir \
 		-I $builddir/src/include -I $srcdir/src/include \
 		-I $builddir/src/interfaces/libpq -I $srcdir/src/interfaces/libpq \
 		$EXTRAINCLUDES $EXTRAFLAGS $CXXFLAGS -c $tmp/test.cpp
-
+	then
+		exit_status=1
+	fi
 done
+
+exit $exit_status
diff --git a/src/tools/pginclude/headerscheck b/src/tools/pginclude/headerscheck
index 3f8640a03d..b8419e46a4 100755
--- a/src/tools/pginclude/headerscheck
+++ b/src/tools/pginclude/headerscheck
@@ -48,6 +48,8 @@ tmp=`mktemp -d /tmp/$me.XXXXXX`
 
 trap 'rm -rf $tmp' 0 1 2 3 15
 
+exit_status=0
+
 # Scan all of src/ and contrib/ for header files.
 for f in `cd "$srcdir" && find src contrib -name '*.h' -print`
 do
@@ -150,9 +152,13 @@ do
 	esac
 
 	# Run the test.
-	${CC:-gcc} $CPPFLAGS $CFLAGS -I $builddir -I $srcdir \
+	if ! ${CC:-gcc} $CPPFLAGS $CFLAGS -I $builddir -I $srcdir \
 		-I $builddir/src/include -I $srcdir/src/include \
 		-I $builddir/src/interfaces/libpq -I $srcdir/src/interfaces/libpq \
 		$EXTRAINCLUDES $EXTRAFLAGS -c $tmp/test.c -o $tmp/test.o
-
+	then
+		exit_status=1
+	fi
 done
+
+exit $exit_status
#2Andrew Dunstan
andrew@dunslane.net
In reply to: Thomas Munro (#1)
Re: Header checker scripts should fail on failure

On 2022-08-15 Mo 01:38, Thomas Munro wrote:

Hi,

I thought commit 81b9f23c9c8 had my back, but nope, we still need to
make CI turn red if "headerscheck" and "cpluspluscheck" don't like our
patches (crake in the build farm should be a secondary defence...).
See attached.

Yeah, the buildfarm module works around that by looking for non-empty
output, but this is better,

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

#3Andres Freund
andres@anarazel.de
In reply to: Thomas Munro (#1)
Re: Header checker scripts should fail on failure

Hi,

On 2022-08-15 17:38:21 +1200, Thomas Munro wrote:

I thought commit 81b9f23c9c8 had my back, but nope, we still need to
make CI turn red if "headerscheck" and "cpluspluscheck" don't like our
patches (crake in the build farm should be a secondary defence...).
See attached.

+1

Greetings,

Andres Freund