[pkgsrc/trunk]: pkgsrc mk/subst.mk: fix early evaluation of SUBST_FILES

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

details:   https://anonhg.NetBSD.org/pkgsrc/rev/e2df192b2ba2
branches:  trunk
changeset: 425157:e2df192b2ba2
user:      rillig <rillig%pkgsrc.org@localhost>
date:      Fri Mar 20 06:17:48 2020 +0000

description:
mk/subst.mk: fix early evaluation of SUBST_FILES

In the case of pkglocaledir, the SUBST_FILES are generated by a shell
command. That command assumes that the WRKDIR already exists. Therefore
SUBST_FILES must be evaluated as late as possible.

See mk/configure/replace-localedir.mk; an example package that fails is
devel/gettext-tools.

diffstat:

 mk/subst.mk                      |  20 +++++++++++++-------
 regress/infra-unittests/subst.sh |  35 +++++++++++++++++++++--------------
 2 files changed, 34 insertions(+), 21 deletions(-)

diffs (180 lines):

diff -r cc30264a9884 -r e2df192b2ba2 mk/subst.mk
--- a/mk/subst.mk       Fri Mar 20 04:01:24 2020 +0000
+++ b/mk/subst.mk       Fri Mar 20 06:17:48 2020 +0000
@@ -1,4 +1,4 @@
-# $NetBSD: subst.mk,v 1.63 2020/03/19 16:57:35 rillig Exp $
+# $NetBSD: subst.mk,v 1.64 2020/03/20 06:17:48 rillig Exp $
 #
 # The subst framework replaces text in one or more files in the WRKSRC
 # directory. Packages can define several ``classes'' of replacements.
@@ -136,10 +136,15 @@
 .  if !empty(SUBST_MESSAGE.${_class_})
        ${RUN} ${ECHO_SUBST_MSG} ${SUBST_MESSAGE.${_class_}:Q}
 .  endif
-.for pattern in ${SUBST_FILES.${_class_}}
-       ${RUN} cd ${WRKSRC:Q};                                          \
+       ${RUN}                                                          \
+       basedir=${WRKSRC:Q};                                            \
+       emptydir="$$basedir/.subst-empty";                              \
+       patterns=${SUBST_FILES.${_class_}:Q};                           \
+       ${MKDIR} "$$emptydir"; cd "$$emptydir";                         \
+       for pattern in $$patterns; do                                   \
        changed=no;                                                     \
-       for file in ${pattern}; do                                      \
+       cd "$$basedir";                                                 \
+       for file in $$pattern; do                                       \
                case $$file in /*) ;; *) file="./$$file";; esac;        \
                tmpfile="$$file.subst.sav";                             \
                if [ ! -f "$$file" ]; then                              \
@@ -166,8 +171,9 @@
        done;                                                           \
        \
        if test "$$changed,${SUBST_NOOP_OK.${_class_}:tl}" = no,no; then \
-               ${FAIL_MSG} "[subst.mk:${_class_}] The pattern "${pattern:Q}" has no effect."; \
-       fi
-.endfor
+               ${FAIL_MSG} "[subst.mk:${_class_}] The pattern $$pattern has no effect."; \
+       fi; \
+       done; \
+       ${RMDIR} "$$emptydir"
        ${RUN} ${TOUCH} ${TOUCH_FLAGS} ${.TARGET:Q}
 .endfor
diff -r cc30264a9884 -r e2df192b2ba2 regress/infra-unittests/subst.sh
--- a/regress/infra-unittests/subst.sh  Fri Mar 20 04:01:24 2020 +0000
+++ b/regress/infra-unittests/subst.sh  Fri Mar 20 06:17:48 2020 +0000
@@ -9,7 +9,7 @@
 testcase() {
        test_name="$1"
 
-       rm -rf "$tmpdir/".subst_*_done
+       rm -rf "$tmpdir"/.subst_*_done "$tmpdir"/.subst-empty
        rm -rf "$tmpdir"/*
        ls -A "$tmpdir"
 
@@ -19,8 +19,10 @@
 CHMOD=         chmod-is-not-used
 CMP=           cmp
 ECHO=          echo
+MKDIR=         mkdir -p
 MV=            mv
 RM=            rm
+RMDIR=         rmdir
 SED=           sed
 TEST=          test
 TOUCH=         touch
@@ -29,7 +31,7 @@
 TRUE=          true
 
 # Commands that are specific to pkgsrc
-RUN=           @: set -x;
+RUN=           @set -e;
 STEP_MSG=      echo "=>"
 DO_NADA=       : do-nada
 INFO_MSG=      echo "info:"
@@ -164,7 +166,7 @@
 fi
 
 
-if testcase "single file noop"; then
+if testcase "single file noop, noop_ok=yes"; then
 
        create_file "testcase.mk" <<EOF
 SUBST_CLASSES+=                class
@@ -182,17 +184,17 @@
        create_file_lines "single"      "already an example"
 
        test_file "testcase.mk" > "$tmpdir/actual-output" && exitcode=0 || exitcode=$?
-       assert_that "$exitcode" --equals "0"
+
        create_file_lines "expected-output" \
                '=> Substituting "class" in single' \
                'info: [subst.mk:class] Nothing changed in ./single.'
-
        assert_that "actual-output" --file-equals "expected-output"
        assert_that "single" --file-contains-exactly "already an example"
+       assert_that "$exitcode" --equals "0"
 fi
 
 
-if testcase "single file noop ok"; then
+if testcase "single file noop, noop_ok=no"; then
 
        create_file "testcase.mk" <<EOF
 SUBST_CLASSES+=                class
@@ -210,7 +212,7 @@
        create_file_lines "single"      "already an example"
 
        test_file "testcase.mk" > "$tmpdir/actual-output" && exitcode=0 || exitcode=$?
-       assert_that "$exitcode" --equals "1"
+
        create_file_lines "expected-output" \
                '=> Substituting "class" in single' \
                'info: [subst.mk:class] Nothing changed in ./single.' \
@@ -219,9 +221,9 @@
                '' \
                'Stop.' \
                "$make: stopped in $PWD"
-
        assert_that "actual-output" --file-equals "expected-output"
        assert_that "single" --file-contains-exactly "already an example"
+       assert_that "$exitcode" --equals "1"
 fi
 
 
@@ -241,7 +243,7 @@
 EOF
 
        test_file "testcase.mk" > "$tmpdir/actual-output" && exitcode=0 || exitcode=$?
-       assert_that "$exitcode" --equals "1"
+
        create_file_lines "expected-output" \
                '=> Substituting "class" in nonexistent' \
                'warning: [subst.mk:class] Ignoring non-existent file "./nonexistent".' \
@@ -250,8 +252,8 @@
                '' \
                'Stop.' \
                "$make: stopped in $PWD"
-
        assert_that "actual-output" --file-equals "expected-output"
+       assert_that "$exitcode" --equals "1"
 fi
 
 
@@ -271,12 +273,12 @@
 EOF
 
        test_file "testcase.mk" > "$tmpdir/actual-output" && exitcode=0 || exitcode=$?
-       assert_that "$exitcode" --equals "0"
+
        create_file_lines "expected-output" \
                '=> Substituting "class" in nonexistent' \
                'warning: [subst.mk:class] Ignoring non-existent file "./nonexistent".'
-
        assert_that "actual-output" --file-equals "expected-output"
+       assert_that "$exitcode" --equals "0"
 fi
 
 
@@ -326,7 +328,6 @@
                'warning: [subst.mk:class] Ignoring non-existent file "./does".' \
                'warning: [subst.mk:class] Ignoring non-existent file "./not".' \
                'warning: [subst.mk:class] Ignoring non-existent file "./exist".'
-
        assert_that "actual-output" --file-equals "expected-output"
        assert_that "$exitcode" --equals "0"
 fi
@@ -354,7 +355,13 @@
                'info: [subst.mk:class] Nothing changed in ./first.' \
                'info: [subst.mk:class] Nothing changed in ./second.' \
                'info: [subst.mk:class] Nothing changed in ./third.'
-
        assert_that "actual-output" --file-equals "expected-output"
        assert_that "$exitcode" --equals "0"
 fi
+
+# TODO: Add test that ensures SUBST_FILES is evaluated as late as possible.
+# Evaluating it early breaks cases like _pkglocaledir where the list of files
+# is generated by a shell command. See mk/configure/replace-localedir.mk.
+
+# TODO: Add tests for files having special characters, like space, dollar,
+# backticks, basically every ASCII character.