Sat May 16 19:02:32 2020 UTC ()
mk/subst.mk: change default value for SUBST_NOOP_OK from yes to no

This makes the SUBST blocks stricter than before, to detect outdated or
unnecessary definitions.

Filename patterns that are not affected by any of the SUBST_SED
expressions make the build fail.  It is still ok if only some of the
files from a pattern are affected and some aren't.

The latest bulk build shows that most of the build failures are fixed.
The packages that fail in that build are mostly due to other failures,
like missing C headers, wrong PLIST files, unresolved references at link
time.  There may be a few packages that still fail because of this, but
these are near the leaves of the dependency tree.

https://mail-index.netbsd.org/pkgsrc-bulk/2020/05/14/msg018919.html


(rillig)
diff -r1.92 -r1.93 pkgsrc/mk/subst.mk
diff -r1.41 -r1.42 pkgsrc/regress/infra-unittests/subst.sh
cvs diff -r1.92 -r1.93 pkgsrc/mk/subst.mk (expand / switch to context diff)
--- pkgsrc/mk/subst.mk 2020/05/02 05:52:09 1.92
+++ pkgsrc/mk/subst.mk 2020/05/16 19:02:32 1.93
@@ -1,4 +1,4 @@
-# $NetBSD: subst.mk,v 1.92 2020/05/02 05:52:09 rillig Exp $
+# $NetBSD: subst.mk,v 1.93 2020/05/16 19:02:32 rillig Exp $
 #
 # The subst framework replaces text in one or more files in the WRKSRC
 # directory. Packages can define several ``classes'' of replacements.
@@ -22,23 +22,18 @@
 #	SUBST classes. Defaults to "no".
 #
 # SUBST_NOOP_OK
-#	Whether it is ok to list files in SUBST_FILES that don't contain
-#	any of the patterns from SUBST_SED or SUBST_VARS.  Such a
-#	situation often arises when a package is updated to a newer
-#	version, and the build instructions of the package have been
-#	made more portable or flexible.
+#	Whether it is ok to have patterns in SUBST_FILES that don't
+#	contain any of the patterns from SUBST_SED or SUBST_VARS and
+#	thus are not modified at all.
 #
-#	This setting only affects the filename patterns in SUBST_FILES.
-#	It does not (yet) affect the regular expressions in SUBST_SED.
+#	This setting only detects redundant filename patterns.  It does
+#	not detect redundant patterns in SUBST_SED.
 #
-#	From the viewpoint of sed(1), a pattern like s|man|man| may look
-#	redundant but it really isn't, because the second "man" probably
-#	comes from ${PKGMANDIR}, which may be configured to a different
-#	directory.  Patterns like these are therefore allowed, even if
-#	they are no-ops in the current configuration.
+#	Identity substitutions like s|man|man| do not count as no-ops
+#	since their replacement part usually comes from a variable, such
+#	as PKGMANDIR.
 #
-#	For backwards compatibility this defaults to "yes", but it
-#	should rather be set to "no".
+#	Defaults to no.  Will be removed after 2020Q3.
 #
 # Package-settable variables:
 #
@@ -93,12 +88,20 @@
 #	the actual changes as a unified diff.
 #
 # SUBST_NOOP_OK.<class>
-#	Whether to fail when a SUBST_FILES pattern has no effect.
-#	In most cases, "yes" is appropriate, to catch typos and outdated
-#	definitions.
+#	Whether to allow filename patterns in SUBST_FILES that don't
+#	contain any of the patterns from SUBST_SED.
 #
-#	Default: no (up to 2019Q4), yes (starting with 2020Q1)
+#	Defaults to no, since May 2020.
 #
+#	Typical reasons to change this to yes are:
+#
+#	1.  SUBST_FILES is generated dynamically and may include
+#	    unaffected files.
+#
+#	2.  There are multiple SUBST_SED patterns, and some of these
+#	    do not count as identity substitution since they contain
+#	    ".*" or similar parts.
+#
 # See also:
 #	PLIST_SUBST
 #
@@ -106,7 +109,7 @@
 #
 
 SUBST_SHOW_DIFF?=	no
-SUBST_NOOP_OK?=		yes	# only for backwards compatibility
+SUBST_NOOP_OK?=		no	# since May 2020
 
 _VARGROUPS+=		subst
 _USER_VARS.subst=	SUBST_SHOW_DIFF SUBST_NOOP_OK
cvs diff -r1.41 -r1.42 pkgsrc/regress/infra-unittests/subst.sh (expand / switch to context diff)
--- pkgsrc/regress/infra-unittests/subst.sh 2020/05/16 12:43:10 1.41
+++ pkgsrc/regress/infra-unittests/subst.sh 2020/05/16 19:02:32 1.42
@@ -1,5 +1,5 @@
 #! /bin/sh
-# $NetBSD: subst.sh,v 1.41 2020/05/16 12:43:10 rillig Exp $
+# $NetBSD: subst.sh,v 1.42 2020/05/16 19:02:32 rillig Exp $
 #
 # Tests for mk/subst.mk.
 #
@@ -141,7 +141,7 @@
 fi
 
 
-if test_case_begin 'pattern with 1 noop'; then
+if test_case_begin 'pattern with 1 noop, no-op ok'; then
 
 	# Several files are given via a pattern.
 	# Most of the files are patched, but one stays the same.
@@ -155,6 +155,7 @@
 		SUBST_STAGE.class=	pre-configure
 		SUBST_FILES.class=	pattern-*
 		SUBST_SED.class=	-e 's,file,example,'
+		SUBST_NOOP_OK.class=	yes
 
 		.include "prepare-subst.mk"
 		.include "mk/subst.mk"
@@ -166,7 +167,7 @@
 	create_file_lines 'pattern-second'	'the second is already an example'
 	create_file_lines 'pattern-third'	'the third file'
 
-	run_bmake 'testcase.mk' 1> "$tmpdir/output" \
+	run_bmake 'testcase.mk' 1> "$tmpdir/output" 2>&1 \
 	&& exitcode=0 || exitcode=$?
 
 	assert_that "$tmpdir/output" --file-is-lines \
@@ -180,6 +181,46 @@
 fi
 
 
+if test_case_begin 'pattern with 1 noop, no-op not ok'; then
+
+	# Several files are given via a pattern.
+	# Most of the files are patched, but one stays the same.
+	# Since it is easier to give a too broad pattern like *.py
+	# than to exclude a few files from such a pattern,
+	# only a warning is logged.
+	# This is not an error.
+
+	create_file 'testcase.mk' <<-EOF
+		SUBST_CLASSES+=		class
+		SUBST_STAGE.class=	pre-configure
+		SUBST_FILES.class=	pattern-*
+		SUBST_SED.class=	-e 's,file,example,'
+		SUBST_NOOP_OK.class=	no
+
+		.include "prepare-subst.mk"
+		.include "mk/subst.mk"
+
+		all: subst-class
+	EOF
+
+	create_file_lines 'pattern-first'	'the first file'
+	create_file_lines 'pattern-second'	'the second is already an example'
+	create_file_lines 'pattern-third'	'the third file'
+
+	run_bmake 'testcase.mk' 1> "$tmpdir/output" 2>&1 \
+	&& exitcode=0 || exitcode=$?
+
+	assert_that "$tmpdir/output" --file-is-lines \
+		'=> Substituting "class" in pattern-*' \
+		'warning: [subst.mk:class] Nothing changed in "pattern-second".'
+	assert_that 'pattern-first' --file-is-lines 'the first example'
+	assert_that 'pattern-second' --file-is-lines 'the second is already an example'
+	assert_that 'pattern-third' --file-is-lines 'the third example'
+
+	test_case_end
+fi
+
+
 if test_case_begin 'single file noop, noop_ok=yes'; then
 
 	create_file 'testcase.mk' <<-EOF
@@ -295,13 +336,14 @@
 fi
 
 
-if test_case_begin 'several patterns, 1 nonexistent'; then
+if test_case_begin 'several filename patterns, 1 nonexistent, no-op ok'; then
 
 	create_file 'testcase.mk' <<-EOF
 		SUBST_CLASSES+=		class
 		SUBST_STAGE.class=	pre-configure
 		SUBST_FILES.class=	*exist* *not-found*
 		SUBST_SED.class=	-e 's,file,example,'
+		SUBST_NOOP_OK.class=	yes
 
 		.include "prepare-subst.mk"
 		.include "mk/subst.mk"
@@ -322,23 +364,89 @@
 fi
 
 
-if test_case_begin 'multiple missing files, all are reported at once'; then
+if test_case_begin 'several filename patterns, 1 nonexistent, no-op not ok'; then
 
 	create_file 'testcase.mk' <<-EOF
 		SUBST_CLASSES+=		class
 		SUBST_STAGE.class=	pre-configure
+		SUBST_FILES.class=	*exist* *not-found*
+		SUBST_SED.class=	-e 's,file,example,'
+		SUBST_NOOP_OK.class=	no
+
+		.include "prepare-subst.mk"
+		.include "mk/subst.mk"
+	EOF
+
+	create_file_lines 'exists'	'this file exists'
+
+	run_bmake 'testcase.mk' 'subst-class' 1> "$tmpdir/output" 2>&1 \
+	&& exitcode=0 || exitcode=$?
+
+	assert_that "$tmpdir/output" --file-is-lines \
+		'=> Substituting "class" in *exist* *not-found*' \
+		'warning: [subst.mk:class] Ignoring nonexistent file "./*not-found*".' \
+		'fail: [subst.mk:class] The filename pattern "*not-found*" has no effect.' \
+		'*** Error code 1' \
+		'' \
+		'Stop.' \
+		"$make: stopped in $PWD"
+	assert_that 'exists' --file-is-lines 'this example exists'
+	assert_that "$exitcode" --equals '1'
+
+	test_case_end
+fi
+
+
+if test_case_begin 'multiple missing files, all are reported at once, no-op not ok'; then
+
+	create_file 'testcase.mk' <<-EOF
+		SUBST_CLASSES+=		class
+		SUBST_STAGE.class=	pre-configure
 		SUBST_FILES.class=	does not exist
 		SUBST_SED.class=	-e 'sahara'
+		SUBST_NOOP_OK.class=	no
 
 		.include "prepare-subst.mk"
 		.include "mk/subst.mk"
 	EOF
 
-	run_bmake 'testcase.mk' > "$tmpdir/output" \
+	run_bmake 'testcase.mk' 1> "$tmpdir/output" 2>&1 \
 	&& exitcode=0 || exitcode=$?
 
 	assert_that "$tmpdir/output" --file-is-lines \
 		'=> Substituting "class" in does not exist' \
+		'warning: [subst.mk:class] Ignoring nonexistent file "does".' \
+		'warning: [subst.mk:class] Ignoring nonexistent file "not".' \
+		'warning: [subst.mk:class] Ignoring nonexistent file "exist".' \
+		'fail: [subst.mk:class] The filename patterns "does not exist" have no effect.' \
+		'*** Error code 1' \
+		'' \
+		'Stop.' \
+		"$make: stopped in $PWD"
+	assert_that "$exitcode" --equals '1'
+
+	test_case_end
+fi
+
+
+if test_case_begin 'multiple missing files, all are reported at once, no-op ok'; then
+
+	create_file 'testcase.mk' <<-EOF
+		SUBST_CLASSES+=		class
+		SUBST_STAGE.class=	pre-configure
+		SUBST_FILES.class=	does not exist
+		SUBST_SED.class=	-e 'sahara'
+		SUBST_NOOP_OK.class=	yes
+
+		.include "prepare-subst.mk"
+		.include "mk/subst.mk"
+	EOF
+
+	run_bmake 'testcase.mk' 1> "$tmpdir/output" 2>&1 \
+	&& exitcode=0 || exitcode=$?
+
+	assert_that "$tmpdir/output" --file-is-lines \
+		'=> Substituting "class" in does not exist' \
 		'info: [subst.mk:class] Ignoring nonexistent file "does".' \
 		'info: [subst.mk:class] Ignoring nonexistent file "not".' \
 		'info: [subst.mk:class] Ignoring nonexistent file "exist".'
@@ -348,13 +456,14 @@
 fi
 
 
-if test_case_begin 'multiple no-op files, all are reported at once'; then
+if test_case_begin 'multiple no-op files, all are reported at once, no-op not ok'; then
 
 	create_file 'testcase.mk' <<-EOF
 		SUBST_CLASSES+=		class
 		SUBST_STAGE.class=	pre-configure
 		SUBST_FILES.class=	first second third
 		SUBST_SED.class=	-e 's,from,to,'
+		SUBST_NOOP_OK.class=	no
 
 		.include "prepare-subst.mk"
 		.include "mk/subst.mk"
@@ -363,11 +472,46 @@
 	create_file_lines 'second'	'second'
 	create_file_lines 'third'	'third'
 
-	run_bmake 'testcase.mk' > "$tmpdir/output" \
+	run_bmake 'testcase.mk' 1> "$tmpdir/output" 2>&1 \
 	&& exitcode=0 || exitcode=$?
 
 	assert_that "$tmpdir/output" --file-is-lines \
 		'=> Substituting "class" in first second third' \
+		'warning: [subst.mk:class] Nothing changed in "first".' \
+		'warning: [subst.mk:class] Nothing changed in "second".' \
+		'warning: [subst.mk:class] Nothing changed in "third".' \
+		'fail: [subst.mk:class] The filename patterns "first second third" have no effect.' \
+		'*** Error code 1' \
+		'' \
+		'Stop.' \
+		"$make: stopped in $PWD"
+	assert_that "$exitcode" --equals '1'
+
+	test_case_end
+fi
+
+
+if test_case_begin 'multiple no-op files, all are reported at once, no-op ok'; then
+
+	create_file 'testcase.mk' <<-EOF
+		SUBST_CLASSES+=		class
+		SUBST_STAGE.class=	pre-configure
+		SUBST_FILES.class=	first second third
+		SUBST_SED.class=	-e 's,from,to,'
+		SUBST_NOOP_OK.class=	yes
+
+		.include "prepare-subst.mk"
+		.include "mk/subst.mk"
+	EOF
+	create_file_lines 'first'	'text'
+	create_file_lines 'second'	'second'
+	create_file_lines 'third'	'third'
+
+	run_bmake 'testcase.mk' 1> "$tmpdir/output" 2>&1 \
+	&& exitcode=0 || exitcode=$?
+
+	assert_that "$tmpdir/output" --file-is-lines \
+		'=> Substituting "class" in first second third' \
 		'info: [subst.mk:class] Nothing changed in "first".' \
 		'info: [subst.mk:class] Nothing changed in "second".' \
 		'info: [subst.mk:class] Nothing changed in "third".'
@@ -1441,6 +1585,7 @@
 		'SUBST_CLASSES+=	id' \
 		'SUBST_FILES.id=	file' \
 		"SUBST_SED.id=		-e 's,\"\\\\\`,\"\\\\\`,'" \
+		'SUBST_NOOP_OK.id=	yes' \
 		'' \
 		'.include "prepare-subst.mk"' \
 		'.include "mk/subst.mk"'