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.
This commit is contained in:
rillig 2020-04-29 18:33:56 +00:00
parent 5dde5f6cf4
commit 4bafb8759a
4 changed files with 263 additions and 8 deletions

View file

@ -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);
}

View file

@ -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 # The subst framework replaces text in one or more files in the WRKSRC
# directory. Packages can define several ``classes'' of replacements. # directory. Packages can define several ``classes'' of replacements.
@ -22,8 +22,20 @@
# SUBST classes. Defaults to "no". # SUBST classes. Defaults to "no".
# #
# SUBST_NOOP_OK # SUBST_NOOP_OK
# Whether it is ok to have filename patterns in SUBST_FILES that # Whether it is ok to list files in SUBST_FILES that don't contain
# don't have any effect. # 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 # For backwards compatibility this defaults to "yes", but it
# should rather be set to "no". # should rather be set to "no".
@ -94,7 +106,7 @@
# #
SUBST_SHOW_DIFF?= no SUBST_SHOW_DIFF?= no
SUBST_NOOP_OK?= yes # only for backwards compatiblity SUBST_NOOP_OK?= yes # only for backwards compatibility
_VARGROUPS+= subst _VARGROUPS+= subst
_USER_VARS.subst= SUBST_SHOW_DIFF SUBST_NOOP_OK _USER_VARS.subst= SUBST_SHOW_DIFF SUBST_NOOP_OK
@ -183,6 +195,13 @@ ${_SUBST_COOKIE.${class}}:
}; \ }; \
${SUBST_FILTER_CMD.${class}} < "$$file" > "$$tmpfile"; \ ${SUBST_FILTER_CMD.${class}} < "$$file" > "$$tmpfile"; \
${CMP} -s "$$tmpfile" "$$file" && { \ ${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\"."; \ ${_SUBST_WARN.${class}} "Nothing changed in \"$$file\"."; \
${RM} -f "$$tmpfile"; \ ${RM} -f "$$tmpfile"; \
continue; \ continue; \

View file

@ -1,5 +1,5 @@
#! /bin/sh #! /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. # Tests for mk/subst.mk.
# #
@ -14,6 +14,7 @@ test_case_set_up() {
create_file "prepare-subst.mk" <<EOF create_file "prepare-subst.mk" <<EOF
# The tools that are used by subst.mk # The tools that are used by subst.mk
AWK= awk
CHMOD= chmod CHMOD= chmod
CMP= cmp CMP= cmp
DIFF= diff DIFF= diff
@ -1131,3 +1132,200 @@ if test_case_begin "unreadable file"; then
test_case_end test_case_end
fi 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

View file

@ -1,5 +1,5 @@
#! /bin/sh #! /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 # This file defines utilities for testing Makefile fragments and shell
# programs from the pkgsrc infrastructure. While testing one part of the # programs from the pkgsrc infrastructure. While testing one part of the
@ -246,7 +246,7 @@ create_pkgsrc_file() {
run_bmake() { run_bmake() {
cat <<EOF > "$tmpdir/test.subr.main.mk" cat <<EOF > "$tmpdir/test.subr.main.mk"
PKGSRCDIR= $relative_pkgsrcdir PKGSRCDIR= $pkgsrcdir
.PATH: $mocked_pkgsrcdir .PATH: $mocked_pkgsrcdir
.PATH: $pkgsrcdir .PATH: $pkgsrcdir
.include "$1" .include "$1"