CVS commit: pkgsrc

["Roland Illig" <rillig%netbsd.org@localhost>]

Module Name:    pkgsrc
Committed By:   rillig
Date:           Wed Apr 29 22:46:42 UTC 2020

Modified Files:
        pkgsrc/mk: subst.mk
        pkgsrc/regress/infra-unittests: subst.sh

Log Message:
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.


To generate a diff of this commit:
cvs rdiff -u -r1.85 -r1.86 pkgsrc/mk/subst.mk
cvs rdiff -u -r1.26 -r1.27 pkgsrc/regress/infra-unittests/subst.sh

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: pkgsrc/mk/subst.mk
diff -u pkgsrc/mk/subst.mk:1.85 pkgsrc/mk/subst.mk:1.86
--- pkgsrc/mk/subst.mk:1.85     Wed Apr 29 18:33:56 2020
+++ pkgsrc/mk/subst.mk  Wed Apr 29 22:46:42 2020
@@ -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 @@ PKG_FAIL_REASON+=  "[subst.mk] duplicate 
 .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_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}} \
+                               ${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;                    \

Index: pkgsrc/regress/infra-unittests/subst.sh
diff -u pkgsrc/regress/infra-unittests/subst.sh:1.26 pkgsrc/regress/infra-unittests/subst.sh:1.27
--- pkgsrc/regress/infra-unittests/subst.sh:1.26        Wed Apr 29 18:33:56 2020
+++ pkgsrc/regress/infra-unittests/subst.sh     Wed Apr 29 22:46:42 2020
@@ -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 @@ if test_case_begin "identity + no-op sub
 
        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