[pkgsrc/trunk]: pkgsrc mk/subst.mk: fix combination of SUBST_FILTER_CMD with ...

[rillig <rillig%pkgsrc.org@localhost>]

details:   https://anonhg.NetBSD.org/pkgsrc/rev/811b4c4b6b9d
branches:  trunk
changeset: 428618:811b4c4b6b9d
user:      rillig <rillig%pkgsrc.org@localhost>
date:      Wed Apr 29 22:46:42 2020 +0000

description:
mk/subst.mk: fix combination of SUBST_FILTER_CMD with SUBST_NOOP_OK=no

Since SUBST_FILTER_CMD is a shell command, it may contain arbitrary
characters.  The condition in mk/subst.mk that tested whether
SUBST_FILTER_CMD was the default filter command was evaluated at run
time.  In such an evaluation, the variables (lhs and rhs) are fully
expanded before parsing the condition.  This means that these variables
must not contain quotes or unquoted condition operators.

Exactly this situation came up in one of the regression tests.  The
quoted "0-9" was copied verbatimly into the condition, including the
quotes.  The resulting condition was:

        "tr -d "0-9"" == "LC_ALL=C /usr/bin/sed "

This produced a syntax error because of the left-hand side. Adding a :Q
modifier would have helped for the left-hand side, but this would have
been necessary for the right-hand side as well.  Since an empty SUBST_SED
is defined not to "contain only identity substitutions", the first
condition can simply be removed.

The whole condition in the shell program had not worked anyway since it
expanded to either "[ true ]" or to "[ false ]", and both of these
commands exited successfully.

diffstat:

 mk/subst.mk                      |   9 ++-
 regress/infra-unittests/subst.sh |  98 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 103 insertions(+), 4 deletions(-)

diffs (140 lines):

diff -r ce0d1a223881 -r 811b4c4b6b9d mk/subst.mk
--- a/mk/subst.mk       Wed Apr 29 21:41:24 2020 +0000
+++ b/mk/subst.mk       Wed Apr 29 22:46:42 2020 +0000
@@ -1,4 +1,4 @@
-# $NetBSD: subst.mk,v 1.85 2020/04/29 18:33:56 rillig Exp $
+# $NetBSD: subst.mk,v 1.86 2020/04/29 22:46:42 rillig Exp $
 #
 # The subst framework replaces text in one or more files in the WRKSRC
 # directory. Packages can define several ``classes'' of replacements.
@@ -136,6 +136,10 @@
 .for class in ${SUBST_CLASSES:O:u}
 _SUBST_COOKIE.${class}=                ${WRKDIR}/.subst_${class}_done
 
+.if defined(SUBST_FILTER_CMD.${class}) && (defined(SUBST_SED.${class}) || defined(SUBST_VARS.${class}))
+PKG_FAIL_REASON+=              "[subst.mk:${class}] SUBST_FILTER_CMD and SUBST_SED/SUBST_VARS cannot be combined."
+.endif
+
 SUBST_FILTER_CMD.${class}?=    LC_ALL=C ${SED} ${SUBST_SED.${class}}
 SUBST_VARS.${class}?=          # none
 SUBST_MESSAGE.${class}?=       Substituting "${class}" in ${SUBST_FILES.${class}}
@@ -195,8 +199,7 @@
                        };                                              \
                        ${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}} \
+                               ${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;                    \
diff -r ce0d1a223881 -r 811b4c4b6b9d regress/infra-unittests/subst.sh
--- a/regress/infra-unittests/subst.sh  Wed Apr 29 21:41:24 2020 +0000
+++ b/regress/infra-unittests/subst.sh  Wed Apr 29 22:46:42 2020 +0000
@@ -1,5 +1,5 @@
 #! /bin/sh
-# $NetBSD: subst.sh,v 1.26 2020/04/29 18:33:56 rillig Exp $
+# $NetBSD: subst.sh,v 1.27 2020/04/29 22:46:42 rillig Exp $
 #
 # Tests for mk/subst.mk.
 #
@@ -1329,3 +1329,99 @@
 
        test_case_end
 fi
+
+
+if test_case_begin "SUBST_FILTER_CMD + SUBST_SED in NOOP_OK=no mode"; then
+
+       # If SUBST_FILTER_CMD is defined for a SUBST class, the
+       # corresponding SUBST_SED and SUBST_VARS are ignored. To avoid
+       # redundant variable definitions, this case fails fast.
+
+       create_file_lines "testcase.mk" \
+               'SUBST_CLASSES+=        id' \
+               'SUBST_FILES.id=        file' \
+               'SUBST_FILTER_CMD.id=   tr -d "0-9"' \
+               'SUBST_SED.id=          -e s,x,x,' \
+               'SUBST_NOOP_OK.id=      no' \
+               '' \
+               '.include "prepare-subst.mk"' \
+               '.include "mk/subst.mk"'
+       create_file_lines "file" \
+               'letters 123 letters'
+       create_file_lines "main.mk" \
+               "PKGSRCDIR=     $pkgsrcdir" \
+               ".PATH:         $mocked_pkgsrcdir" \
+               ".PATH:         $pkgsrcdir" \
+               ".include \"testcase.mk\"" \
+               '' \
+               'all: subst-id' \
+               '       @printf '\''fail reason: %s\n'\'' ${PKG_FAIL_REASON} 1>&2'
+
+       "$make" -f "$tmpdir/main.mk" "all" 1> "$tmpdir/out" 2>&1 \
+       && exitcode=0 || exitcode=$?
+
+       assert_that "out" --file-is-lines \
+               '=> Substituting "id" in file' \
+               'fail reason: [subst.mk:id] SUBST_FILTER_CMD and SUBST_SED/SUBST_VARS cannot be combined.'
+       assert_that "file" --file-is-lines \
+               'letters  letters'
+
+       test_case_end
+fi
+
+
+if test_case_begin "effective SUBST_FILTER_CMD in NOOP_OK=no mode"; then
+
+       create_file_lines "testcase.mk" \
+               'SUBST_CLASSES+=        id' \
+               'SUBST_FILES.id=        file' \
+               'SUBST_FILTER_CMD.id=   tr -d "0-9"' \
+               'SUBST_NOOP_OK.id=      no' \
+               '' \
+               '.include "prepare-subst.mk"' \
+               '.include "mk/subst.mk"'
+       create_file_lines "file" \
+               'letters 123 letters'
+
+       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 \
+               'letters  letters'
+
+       test_case_end
+fi
+
+
+if test_case_begin "no-op SUBST_FILTER_CMD in NOOP_OK=no mode"; then
+
+       create_file_lines "testcase.mk" \
+               'SUBST_CLASSES+=        id' \
+               'SUBST_FILES.id=        file' \
+               'SUBST_FILTER_CMD.id=   tr -d "0-9"' \
+               'SUBST_NOOP_OK.id=      no' \
+               '' \
+               '.include "prepare-subst.mk"' \
+               '.include "mk/subst.mk"'
+       create_file_lines "file" \
+               'only letters'
+
+       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 \
+               'only letters'
+
+       test_case_end
+fi