From 4bafb8759ac5bc50927874c0e3ac70c109017774 Mon Sep 17 00:00:00 2001
From: rillig <rillig@pkgsrc.org>
Date: Wed, 29 Apr 2020 18:33:56 +0000
Subject: [PATCH] mk/subst.mk: allow identity substitutions in SUBST_NOOP_OK=no
 mode

There are several cases where patterns like s|man|${PKGMANDIR}| appear in
SUBST_SED.  Up to now, these had been categorized as no-ops and required
extra code to make the package build when SUBST_NOOP_OK was set to "no".

This was against the original intention of SUBST_NOOP_OK, which was to
find outdated substitution patterns that do not occur in SUBST_FILES
anymore, most often because the packages have been updated since.

The identity substitutions do appear in the files, they just don't change
them.  Typical cases are for PKGMANDIR, DEVOSSAUDIO, PREFIX, and these
variables may well be different in another pkgsrc setup.  These patterns
are therefore excluded from the SUBST_NOOP_OK check.
---
 mk/scripts/subst-identity.awk     |  38 ++++++
 mk/subst.mk                       |  29 ++++-
 regress/infra-unittests/subst.sh  | 200 +++++++++++++++++++++++++++++-
 regress/infra-unittests/test.subr |   4 +-
 4 files changed, 263 insertions(+), 8 deletions(-)
 create mode 100644 mk/scripts/subst-identity.awk

diff --git a/mk/scripts/subst-identity.awk b/mk/scripts/subst-identity.awk
new file mode 100644
index 000000000000..fb6bdaab9e28
--- /dev/null
+++ b/mk/scripts/subst-identity.awk
@@ -0,0 +1,38 @@
+#! /usr/bin/awk -f
+# $NetBSD: subst-identity.awk,v 1.1 2020/04/29 18:33:57 rillig Exp $
+#
+# Tests whether a sed(1) command line consists of only identity substitutions
+# like s,id,id,.
+#
+# See SUBST_NOOP_OK and regress/infra-unittests/subst.sh.
+#
+
+function is_safe_char(ch) {
+	return ch ~ /[\t -~]/ && ch !~ /[$&*.\[\\\]^]/;
+}
+
+function is_identity_subst(s,   len, i, sep, pat) {
+	len = length(s);
+	if (len < 6 || substr(s, 1, 1) != "s")
+		return 0;
+
+	sep = substr(s, 2, 1);
+	i = 3;
+	while (i < len && substr(s, i, 1) != sep && is_safe_char(substr(s, i, 1)))
+		i++;
+	pat = substr(s, 3, i - 3);
+
+	return (s == "s" sep pat sep pat sep ||
+		s == "s" sep pat sep pat sep "g");
+}
+
+function main(   i) {
+	for (i = 1; i + 1 < ARGC; i += 2)
+		if (ARGV[i] != "-e" || !is_identity_subst(ARGV[i + 1]))
+			return 0;
+	return i == ARGC && ARGC > 1;
+}
+
+BEGIN {
+	exit(main() ? 0 : 1);
+}
diff --git a/mk/subst.mk b/mk/subst.mk
index dc84bb227aad..eb15f28317dc 100644
--- a/mk/subst.mk
+++ b/mk/subst.mk
@@ -1,4 +1,4 @@
-# $NetBSD: subst.mk,v 1.84 2020/04/23 19:32:53 rillig Exp $
+# $NetBSD: subst.mk,v 1.85 2020/04/29 18:33:56 rillig Exp $
 #
 # The subst framework replaces text in one or more files in the WRKSRC
 # directory. Packages can define several ``classes'' of replacements.
@@ -22,8 +22,20 @@
 #	SUBST classes. Defaults to "no".
 #
 # SUBST_NOOP_OK
-#	Whether it is ok to have filename patterns in SUBST_FILES that
-#	don't have any effect.
+#	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.
+#
+#	This setting only affects the filename patterns in SUBST_FILES.
+#	It does not (yet) affect the regular expressions 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.
 #
 #	For backwards compatibility this defaults to "yes", but it
 #	should rather be set to "no".
@@ -94,7 +106,7 @@
 #
 
 SUBST_SHOW_DIFF?=	no
-SUBST_NOOP_OK?=		yes	# only for backwards compatiblity
+SUBST_NOOP_OK?=		yes	# only for backwards compatibility
 
 _VARGROUPS+=		subst
 _USER_VARS.subst=	SUBST_SHOW_DIFF SUBST_NOOP_OK
@@ -183,6 +195,13 @@ ${_SUBST_COOKIE.${class}}:
 			};						\
 			${SUBST_FILTER_CMD.${class}} < "$$file" > "$$tmpfile";	\
 			${CMP} -s "$$tmpfile" "$$file" && {		\
+				[ ${"${SUBST_FILTER_CMD.${class}}" == "LC_ALL=C ${SED} ${SUBST_SED.${class}}":?true:false} ] \
+				&& ${AWK} -f ${PKGSRCDIR}/mk/scripts/subst-identity.awk -- ${SUBST_SED.${class}} \
+				&& found=`LC_ALL=C ${SED} -n ${SUBST_SED.${class}:C,^['"]?s.*,&p,} "$$file"` \
+				&& [ "x$$found" != "x" ] && {		\
+					changed=yes;			\
+					continue;			\
+				};					\
 				${_SUBST_WARN.${class}} "Nothing changed in \"$$file\"."; \
 				${RM} -f "$$tmpfile";			\
 				continue;				\
@@ -193,7 +212,7 @@ ${_SUBST_COOKIE.${class}}:
 			${MV} -f "$$tmpfile" "$$file"; 			\
 			${ECHO} "$$file" >> ${.TARGET}.tmp;		\
 		done;							\
-	\
+									\
 		[ "$$changed,${SUBST_NOOP_OK.${class}:tl}" = no,no ] && { \
 			noop_count="$$noop_count+";			\
 			noop_patterns="$$noop_patterns$$noop_sep$$pattern"; \
diff --git a/regress/infra-unittests/subst.sh b/regress/infra-unittests/subst.sh
index 3359b323df7b..882d7726dc45 100644
--- a/regress/infra-unittests/subst.sh
+++ b/regress/infra-unittests/subst.sh
@@ -1,5 +1,5 @@
 #! /bin/sh
-# $NetBSD: subst.sh,v 1.25 2020/04/26 12:46:33 rillig Exp $
+# $NetBSD: subst.sh,v 1.26 2020/04/29 18:33:56 rillig Exp $
 #
 # Tests for mk/subst.mk.
 #
@@ -14,6 +14,7 @@ test_case_set_up() {
 	create_file "prepare-subst.mk" <<EOF
 
 # The tools that are used by subst.mk
+AWK=		awk
 CHMOD=		chmod
 CMP=		cmp
 DIFF=		diff
@@ -1131,3 +1132,200 @@ if test_case_begin "unreadable file"; then
 
 	test_case_end
 fi
+
+
+if test_case_begin "identity substitution implementation"; then
+
+	assert_identity() {
+		ai_expected="$1"; shift
+		awk -f "$pkgsrcdir/mk/scripts/subst-identity.awk" -- "$@" \
+		&& ai_actual="yes" || ai_actual="no"
+
+		[ "$ai_actual" = "$ai_expected" ] \
+		|| assert_fail "expected '%s', got '%s' for %s\n" "$ai_expected" "$ai_actual" "$*"
+	}
+
+	# If there is no SUBST_SED at all, this is not the situation
+	# that is targeted by this test for identity substitution.
+	assert_identity "no"	# no substitutions at all
+
+	# Even though this is an identity substitution, it is missing
+	# the -e option and thus does not follow the usual format.
+	# Therefore it is considered just a normal substitution.
+	assert_identity "no"	's,from,from,'
+
+	# The following are typical identity substitutions.
+	# It does not matter whether the g modifier is there or not.
+	# Unknown modifiers are not allowed though.
+	assert_identity "yes"	-e 's,from,from,'
+	assert_identity "yes"	-e 's;from;from;'
+	assert_identity "yes"	-e 's,from,from,g'
+	assert_identity "no"	-e 's,from,from,gunknown'
+
+	# The identity substitution may include characters other than
+	# A-Za-z0-9, but no characters that have a special meaning in
+	# basic regular expressions.
+	assert_identity "yes"	-e 's,/dev/audio,/dev/audio,'
+	assert_identity "yes"	-e 's!/dev/audio!/dev/audio!'
+
+	# There may be several identity substitutions in the same
+	# SUBST_SED.  As long as all these substitutions are identity
+	# substitutions, they may be skipped.  As soon as there is one
+	# other substitution, the whole SUBST_SED is treated as usual.
+	assert_identity "yes"	-e 's;from;from;' -e 's!second!second!'
+	assert_identity "no"	-e 's,changing,x,' -e 's,id,id,'
+	assert_identity "no"	-e 's,id,id,' -e 's,changing,x,'
+
+	# A demonstration of all ASCII characters that may appear in an
+	# identity substitution.
+	#
+	# The # and $ are excluded since they are interpreted specially
+	# in Makefiles and would thus be confusing to the human reader.
+	#
+	# The characters *.?[\]^ have a special meaning in the pattern of the
+	# substitution.
+	# The & has a special meaning in the replacement of the
+	# substitution.
+	specials='!"%'\''()+,-/:;<=>@_`{|}~'
+	assert_identity "yes"	-e "sX${specials}X${specials}X"
+
+	test_case_end
+fi
+
+
+if test_case_begin "identity substitution, found in file"; then
+
+	# There are many situations in which a fixed text is replaced
+	# with a dynamic value that may or may not be equal to the
+	# original text.
+	#
+	# Typical examples are s|man|${PKGMANDIR}|, s|/usr/pkg|${PREFIX}|,
+	# s|/dev/audio|${DEVOSSAUDIO}|.
+	#
+	# It is not an error if these substitutions result in a no-op,
+	# provided that the text is actually found in the file.
+	#
+	# Alternatives for this special exception would be:
+	#
+	# 1. Mark these blocks as SUBST_NOOP_OK.  This would not detect
+	# outdated definitions.  Since this detection is the main goal
+	# of SUBST_NOOP_OK, this is out of the question.
+	#
+	# 2. Surround these blocks with a condition like ".if ${VAR} !=
+	# fixed-value ... .endif".  This pattern only works if VAR is
+	# definitely assigned, which often requires a corresponding
+	# .include line, leading to code bloat.  It would also mean that
+	# variables defined in bsd.pkg.mk could not be used in SUBST
+	# blocks like these.
+
+	create_file_lines "testcase.mk" \
+		'SUBST_CLASSES+=	id' \
+		'SUBST_FILES.id=	file' \
+		'SUBST_SED.id=		-e s,before,before,' \
+		'SUBST_NOOP_OK.id=	no' \
+		'' \
+		'.include "prepare-subst.mk"' \
+		'.include "mk/subst.mk"'
+	create_file_lines "file" \
+		'before'
+
+	run_bmake "testcase.mk" "subst-id" 1> "$tmpdir/out" 2>&1 \
+	&& exitcode=0 || exitcode=$?
+
+	assert_that "out" --file-is-lines \
+		'=> Substituting "id" in file'
+
+	test_case_end
+fi
+
+
+if test_case_begin "identity substitution, not found in file"; then
+
+	create_file_lines "testcase.mk" \
+		'SUBST_CLASSES+=	id' \
+		'SUBST_FILES.id=	file' \
+		'SUBST_SED.id=		s,before,before,' \
+		'SUBST_NOOP_OK.id=	no' \
+		'' \
+		'.include "prepare-subst.mk"' \
+		'.include "mk/subst.mk"'
+	create_file_lines "file" \
+		'other'
+
+	run_bmake "testcase.mk" "subst-id" 1> "$tmpdir/out" 2>&1 \
+	&& exitcode=0 || exitcode=$?
+
+	assert_that "out" --file-is-lines \
+		'=> Substituting "id" in file' \
+		'warning: [subst.mk:id] Nothing changed in "file".' \
+		'fail: [subst.mk:id] The filename pattern "file" has no effect.' \
+		'*** Error code 1' \
+		'' \
+		'Stop.' \
+		"$make: stopped in $PWD"
+
+	test_case_end
+fi
+
+
+if test_case_begin "identity + effective substitution"; then
+
+	create_file_lines "testcase.mk" \
+		'SUBST_CLASSES+=	id' \
+		'SUBST_FILES.id=	file' \
+		'SUBST_SED.id=		-e s,no-op,no-op,g' \
+		'SUBST_SED.id+=		-e s,from,to,' \
+		'SUBST_NOOP_OK.id=	no' \
+		'' \
+		'.include "prepare-subst.mk"' \
+		'.include "mk/subst.mk"'
+	create_file_lines "file" \
+		'from'
+
+	run_bmake "testcase.mk" "subst-id" 1> "$tmpdir/out" 2>&1 \
+	&& exitcode=0 || exitcode=$?
+
+	assert_that "out" --file-is-lines \
+		'=> Substituting "id" in file'
+	assert_that "file" --file-is-lines \
+		'to'
+
+	test_case_end
+fi
+
+
+if test_case_begin "identity + no-op substitution"; then
+
+	# If there were only an identity substitution, it wouldn't be an
+	# error.  But since there is a regular substitution as well,
+	# that substitution is an unexpected no-op and is therefore
+	# flagged as an error.
+
+	create_file_lines "testcase.mk" \
+		'SUBST_CLASSES+=	id' \
+		'SUBST_FILES.id=	file' \
+		'SUBST_SED.id=		-e s,no-op,no-op,g' \
+		'SUBST_SED.id+=		-e s,from,to,' \
+		'SUBST_NOOP_OK.id=	no' \
+		'' \
+		'.include "prepare-subst.mk"' \
+		'.include "mk/subst.mk"'
+	create_file_lines "file" \
+		'other'
+
+	run_bmake "testcase.mk" "subst-id" 1> "$tmpdir/out" 2>&1 \
+	&& exitcode=0 || exitcode=$?
+
+	assert_that "out" --file-is-lines \
+		'=> Substituting "id" in file' \
+		'warning: [subst.mk:id] Nothing changed in "file".' \
+		'fail: [subst.mk:id] The filename pattern "file" has no effect.' \
+		'*** Error code 1' \
+		'' \
+		'Stop.' \
+		"$make: stopped in $PWD"
+	assert_that "file" --file-is-lines \
+		'other'
+
+	test_case_end
+fi
diff --git a/regress/infra-unittests/test.subr b/regress/infra-unittests/test.subr
index afad5b8a5cf1..b6fcaddd6660 100644
--- a/regress/infra-unittests/test.subr
+++ b/regress/infra-unittests/test.subr
@@ -1,5 +1,5 @@
 #! /bin/sh
-# $NetBSD: test.subr,v 1.10 2020/04/26 12:46:33 rillig Exp $
+# $NetBSD: test.subr,v 1.11 2020/04/29 18:33:56 rillig Exp $
 #
 # This file defines utilities for testing Makefile fragments and shell
 # programs from the pkgsrc infrastructure. While testing one part of the
@@ -246,7 +246,7 @@ create_pkgsrc_file() {
 
 run_bmake() {
 	cat <<EOF > "$tmpdir/test.subr.main.mk"
-PKGSRCDIR=	$relative_pkgsrcdir
+PKGSRCDIR=	$pkgsrcdir
 .PATH:		$mocked_pkgsrcdir
 .PATH:		$pkgsrcdir
 .include "$1"