[gentoo-dev] [PATCH 0/2] Add esed.eclass for sed that dies if caused no changes

[gentoo-dev] [PATCH 0/2] Add esed.eclass for sed that dies if caused no changes

[Ionen Wolkens]

Often preferable to use patches so this happens, but sed have its
uses/convenience and this intend to help reduce the amount of old
broken seds causing issues that go unnoticed on bumps.

Inspired by app-portage/iwdevtools' qa-sed (warns on any seds), but
this is for more deterministic use in ebuilds.

Also slightly shortens sed use, -i is default, and no need to || die.
(see @EXAMPLE in eclass for a quick usage overview).

Implementation / available wrappers / usefulness still up for debate,
but without further comments I consider this ready (albeit first time
touching / making an eclass, so I could be overlooking simple things).
Also partly uses >=bash-4.4, so EAPI-7 is not considered.

See github PR[1] for old changelog background.

Up to maintainers but personally would encourage to slowly replace
(almost) all use of sed with either this or patches. Some cases
where it can be inconvenient like eclasses "guessing" that a package
may or may not have something to replace, and that nothing happened
is not an issue.

[1] https://github.com/gentoo/gentoo/pull/25662

Ionen Wolkens (2):
  esed.eclass: new eclass
  eclass/tests/esed.sh: basic tests for esed.eclass

 eclass/esed.eclass   | 199 +++++++++++++++++++++++++++++++++++++++++++
 eclass/tests/esed.sh | 173 +++++++++++++++++++++++++++++++++++++
 2 files changed, 372 insertions(+)
 create mode 100644 eclass/esed.eclass
 create mode 100755 eclass/tests/esed.sh

-- 
2.35.1

[gentoo-dev] [PATCH 1/2] esed.eclass: new eclass

[Ionen Wolkens]

Signed-off-by: Ionen Wolkens <ionen@gentoo.org>
---
 eclass/esed.eclass | 199 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 199 insertions(+)
 create mode 100644 eclass/esed.eclass

diff --git a/eclass/esed.eclass b/eclass/esed.eclass
new file mode 100644
index 00000000000..69f546804c4
--- /dev/null
+++ b/eclass/esed.eclass
@@ -0,0 +1,199 @@
+# Copyright 2022 Gentoo Authors
+# Distributed under the terms of the GNU General Public License v2
+
+# @ECLASS: esed.eclass
+# @MAINTAINER:
+# Ionen Wolkens <ionen@gentoo.org>
+# @AUTHOR:
+# Ionen Wolkens <ionen@gentoo.org>
+# @SUPPORTED_EAPIS: 8
+# @BLURB: sed(1) wrappers that die if expressions did not modify any files
+# @EXAMPLE:
+#
+# @CODE
+# esed 's/a/b/' src/file.c # -i is default, dies if 'a' does not become 'b'
+#
+# enewsed 's/a/b/' project.pc.in "${T}"/project.pc # stdin/out not supported
+#
+# esedfind . -type f -name '*.c' -esed 's/a/b/' # dies if zero files changed
+#
+# local esedexps=(
+#     # dies if /any/ of these did nothing, -e 's/a/b/' -e 's/c/d/' would not
+#     's/a/b/'
+#     's/c/d/' # bug 000000
+#     # use quotes around "$(use..)" to avoid word splitting/globs, won't run
+#     # sed(1) for empty elements (i.e. if USE is disabled)
+#     "$(usev fnord "s/foo bar/${baz}/")"
+# )
+# esed Makefile lib/Makefile # unsets esedexps so it's not re-used
+#
+# use prefix && esed "s|^prefix=|&${EPREFIX}|" project.pc # deterministic
+# @CODE
+#
+# Migration note: be wary of non-deterministic esed() involving variables,
+# e.g. s|lib|$(get_libdir)|, s|-O3|${CFLAGS}|, and the above ${EPREFIX} one.
+# esed() dies if these do nothing, like libdir being 'lib' on x86.  Either
+# verify, keep sed(1), or ensure a change (extra space, @placeholders@).
+
+case ${EAPI} in
+	8) ;;
+	*) die "${ECLASS}: EAPI ${EAPI:-0} not supported" ;;
+esac
+
+if [[ ! -v _ESED_ECLASS ]]; then
+_ESED_ECLASS=1
+
+# @ECLASS_VARIABLE: ESED_VERBOSE
+# @DEFAULT_UNSET
+# @USER_VARIABLE
+# @DESCRIPTION:
+# If set to a non-empty value, esed() and its wrappers will use diff(1)
+# if available to display file differences.
+
+# @VARIABLE: esedexps
+# @DEFAULT_UNSET
+# @DESCRIPTION:
+# Bash array that can optionally contain sed expressions to use sequencially
+# on separate sed calls when using esed() and its wrappers.  Allows inspection
+# of modifications per-expressions.  Unset after use so it's not used in
+# subsequent calls.  Will not run sed(1) for empty array elements.
+
+# @FUNCTION: esed
+# @USAGE: <sed-argument>...
+# @DESCRIPTION:
+# sed(1) wrapper that dies if the expression(s) did not modify any files.
+# sed's -i/--in-place is forced, and so stdin/out cannot be used.
+esed() {
+	local -i i
+
+	if [[ ${esedexps@a} =~ a ]]; then
+		# expression must be before -- but after the rest for e.g. -E to work
+		local -i pos
+		for ((pos=1; pos<=${#}; pos++)); do
+			[[ ${!pos} == -- ]] && break
+		done
+
+		for ((i=0; i<${#esedexps[@]}; i++)); do
+			[[ ${esedexps[i]} ]] &&
+				esedexps= esed "${@:1:pos-1}" -e "${esedexps[i]}" "${@:pos}"
+		done
+
+		unset esedexps
+		return 0
+	fi
+
+	# Roughly attempt to find files in arguments by checking if it's a
+	# readable file (aka s/// is not a file) and does not start with -
+	# (unless after --), then store contents for comparing after sed.
+	local contents=() endopts files=()
+	for ((i=1; i<=${#}; i++)); do
+		if [[ ${!i} == -- && ! -v endopts ]]; then
+			endopts=1
+		elif [[ ${!i} =~ ^(-i|--in-place)$ && ! -v endopts ]]; then
+			# detect rushed sed -i -> esed -i, -i also silently breaks enewsed
+			die "passing ${!i} to ${FUNCNAME[0]} is invalid"
+		elif [[ ${!i} =~ ^(-f|--file)$ && ! -v endopts ]]; then
+			i+=1 # ignore script files
+		elif [[ ( ${!i} != -* || -v endopts ) && -f ${!i} && -r ${!i} ]]; then
+			files+=( "${!i}" )
+
+			# eval 2>/dev/null to silence \0 warnings if sed binary files
+			eval 'contents+=( "$(<"${!i}")" )' 2>/dev/null \
+				|| die "failed to read: ${!i}"
+		fi
+	done
+	(( ${#files[@]} )) || die "no readable files found from '${*}' arguments"
+
+	local verbose
+	[[ ${ESED_VERBOSE} ]] && type diff &>/dev/null && verbose=1
+
+	local changed newcontents
+	if [[ -v _esed_output ]]; then
+		[[ -v verbose ]] &&
+			einfo "${FUNCNAME[0]}: sed ${*} > ${_esed_output} ..."
+
+		sed "${@}" > "${_esed_output}" \
+			|| die "failed to run: sed ${*} > ${_esed_output}"
+
+		eval 'newcontents=$(<${_esed_output})' 2>/dev/null \
+			|| die "failed to read: ${_esed_output}"
+
+		local IFS=$'\n' # sed concats with newline even if none at EOF
+		contents=${contents[*]}
+		unset IFS
+
+		[[ ${contents} != "${newcontents}" ]] && changed=1
+
+		[[ -v verbose ]] &&
+			diff -u --color --label="${files[*]}" --label="${_esed_output}" \
+				<(echo "${contents}") <(echo "${newcontents}")
+	else
+		[[ -v verbose ]] && einfo "${FUNCNAME[0]}: sed -i ${*} ..."
+
+		sed -i "${@}" || die "failed to run: sed -i ${*}"
+
+		for ((i=0; i<${#files[@]}; i++)); do
+			eval 'newcontents=$(<"${files[i]}")' 2>/dev/null \
+				|| die "failed to read: ${files[i]}"
+
+			if [[ ${contents[i]} != "${newcontents}" ]]; then
+				changed=1
+				[[ -v verbose ]] || break
+			fi
+
+			[[ -v verbose ]] &&
+				diff -u --color --label="${files[i]}"{,} \
+					<(echo "${contents[i]}") <(echo "${newcontents}")
+		done
+	fi
+
+	[[ -v changed ]] \
+		|| die "no-op: ${FUNCNAME[0]} ${*}${_esed_command:+ (from: ${_esed_command})}"
+}
+
+# @FUNCTION: enewsed
+# @USAGE: <esed-argument>... <output-file>
+# @DESCRIPTION:
+# esed() wrapper to save the result to <output-file>.  Same as using
+# `sed ... input > output` given esed() does not support stdin/out.
+enewsed() {
+	local _esed_command="${FUNCNAME[0]} ${*}"
+	local _esed_output=${*: -1:1}
+	esed "${@:1:${#}-1}"
+}
+
+# @FUNCTION: esedfind
+# @USAGE: <find-argument>... [-esed [<esed-argument>...]]
+# @DESCRIPTION:
+# esed() wrapper to ease use with find(1) given -exec wouldn't see a shell
+# function.  Will die if find(1) found no files, or if not a single file
+# was changed.  -esed is optional with the esedexps=( .. ) array.  -print0
+# will be appended to <find-arguments>.
+#
+# Requires that the found list not exceed args limit for file changes to be
+# evaluated together in a single esed() call.  Use is discouraged if modifying
+# files with a large total size (50+MB), as they will be loaded in memory
+# and compared ineffectively by the shell.
+esedfind() {
+	local _esed_command="${FUNCNAME[0]} ${*}"
+
+	local find=( find )
+	while (( ${#} )); do
+		if [[ ${1} == -esed ]]; then
+			shift
+			break
+		fi
+		find+=( "${1}" )
+		shift
+	done
+	find+=( -print0 )
+
+	local files
+	mapfile -d '' -t files < <("${find[@]}" || die "failed to run: ${find[*]}")
+
+	(( ${#files[@]} )) || die "no files found from: ${find[*]}"
+
+	esed "${@}" -- "${files[@]}"
+}
+
+fi
-- 
2.35.1

Re: [gentoo-dev] [PATCH 1/2] esed.eclass: new eclass

[Ionen Wolkens]

[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]

On Tue, May 31, 2022 at 07:23:18AM -0400, Ionen Wolkens wrote:
> +			if [[ ${contents[i]} != "${newcontents}" ]]; then
> +				changed=1
> +				[[ -v verbose ]] || break
> +			fi
> +
> +			[[ -v verbose ]] &&
> +				diff -u --color --label="${files[i]}"{,} \
> +					<(echo "${contents[i]}") <(echo "${newcontents}")

self nitpick, didn't think much of these given optional but diff has
nothing to say if contents == newcontents so will replace with:

if [[ ${contents[i]} != "${newcontents}" ]]; then
	changed=1

	[[ -v verbose ]] || break

	diff -u --color --label="${files[i]}"{,} \
		<(echo "${contents[i]}") <(echo "${newcontents}")
fi

> +		[[ ${contents} != "${newcontents}" ]] && changed=1
> +
> +		[[ -v verbose ]] &&
> +			diff -u --color --label="${files[*]}" --label="${_esed_output}" \
> +				<(echo "${contents}") <(echo "${newcontents}")

and:

if [[ ${contents} != "${newcontents}" ]]; then
	changed=1

	[[ -v verbose ]] &&
		diff -u --color --label="${files[*]}" --label="${_esed_output}" \
			<(echo "${contents}") <(echo "${newcontents}")
fi

(updated on github PR)

-- 
ionen

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [gentoo-dev] [PATCH 1/2] esed.eclass: new eclass

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 4016 bytes --]

>>>>> On Tue, 31 May 2022, Ionen Wolkens wrote:

> +# @FUNCTION: esed
> +# @USAGE: <sed-argument>...
> +# @DESCRIPTION:
> +# sed(1) wrapper that dies if the expression(s) did not modify any files.
> +# sed's -i/--in-place is forced, and so stdin/out cannot be used.

This sounds like a simple enough task ...

> +esed() {
> +	local -i i
> +
> +	if [[ ${esedexps@a} =~ a ]]; then
> +		# expression must be before -- but after the rest for e.g. -E to work
> +		local -i pos
> +		for ((pos=1; pos<=${#}; pos++)); do
> +			[[ ${!pos} == -- ]] && break
> +		done
> +
> +		for ((i=0; i<${#esedexps[@]}; i++)); do
> +			[[ ${esedexps[i]} ]] &&
> +				esedexps= esed "${@:1:pos-1}" -e "${esedexps[i]}" "${@:pos}"
> +		done
> +
> +		unset esedexps
> +		return 0
> +	fi
> +
> +	# Roughly attempt to find files in arguments by checking if it's a
> +	# readable file (aka s/// is not a file) and does not start with -
> +	# (unless after --), then store contents for comparing after sed.
> +	local contents=() endopts files=()
> +	for ((i=1; i<=${#}; i++)); do
> +		if [[ ${!i} == -- && ! -v endopts ]]; then
> +			endopts=1
> +		elif [[ ${!i} =~ ^(-i|--in-place)$ && ! -v endopts ]]; then
> +			# detect rushed sed -i -> esed -i, -i also silently breaks enewsed
> +			die "passing ${!i} to ${FUNCNAME[0]} is invalid"
> +		elif [[ ${!i} =~ ^(-f|--file)$ && ! -v endopts ]]; then
> +			i+=1 # ignore script files
> +		elif [[ ( ${!i} != -* || -v endopts ) && -f ${!i} && -r ${!i} ]]; then
> +			files+=( "${!i}" )
> +
> +			# eval 2>/dev/null to silence \0 warnings if sed binary files
> +			eval 'contents+=( "$(<"${!i}")" )' 2>/dev/null \
> +				|| die "failed to read: ${!i}"
> +		fi
> +	done
> +	(( ${#files[@]} )) || die "no readable files found from '${*}' arguments"
> +
> +	local verbose
> +	[[ ${ESED_VERBOSE} ]] && type diff &>/dev/null && verbose=1
> +
> +	local changed newcontents
> +	if [[ -v _esed_output ]]; then
> +		[[ -v verbose ]] &&
> +			einfo "${FUNCNAME[0]}: sed ${*} > ${_esed_output} ..."
> +
> +		sed "${@}" > "${_esed_output}" \
> +			|| die "failed to run: sed ${*} > ${_esed_output}"
> +
> +		eval 'newcontents=$(<${_esed_output})' 2>/dev/null \
> +			|| die "failed to read: ${_esed_output}"
> +
> +		local IFS=$'\n' # sed concats with newline even if none at EOF
> +		contents=${contents[*]}
> +		unset IFS
> +
> +		[[ ${contents} != "${newcontents}" ]] && changed=1
> +
> +		[[ -v verbose ]] &&
> +			diff -u --color --label="${files[*]}" --label="${_esed_output}" \
> +				<(echo "${contents}") <(echo "${newcontents}")
> +	else
> +		[[ -v verbose ]] && einfo "${FUNCNAME[0]}: sed -i ${*} ..."
> +
> +		sed -i "${@}" || die "failed to run: sed -i ${*}"
> +
> +		for ((i=0; i<${#files[@]}; i++)); do
> +			eval 'newcontents=$(<"${files[i]}")' 2>/dev/null \
> +				|| die "failed to read: ${files[i]}"
> +
> +			if [[ ${contents[i]} != "${newcontents}" ]]; then
> +				changed=1
> +				[[ -v verbose ]] || break
> +			fi
> +
> +			[[ -v verbose ]] &&
> +				diff -u --color --label="${files[i]}"{,} \
> +					<(echo "${contents[i]}") <(echo "${newcontents}")
> +		done
> +	fi
> +
> +	[[ -v changed ]] \
> +		|| die "no-op: ${FUNCNAME[0]} ${*}${_esed_command:+ (from: ${_esed_command})}"
> +}

... but then it's almost 100 lines of shell code, including very
convoluted parsing of parameters. The code for detection whether a
parameter is actually a file is a heuristic at best and looks rather
brittle. Also, don't use eval because it is evil.

So IMHO this isn't the right approach to the problem. If anything, make
it a simple function with well defined arguments, e.g. exactly one
expression and exactly one file, and call "sed -i" on it.

Then again, we had something similar in the past ("dosed" as a package
manager command) but later banned it for good reason.

Detection whether the file contents changed also seems complicated.
Why not compute a hash of the file before and after sed operated on it?
md5sum or even cksum should be good enough, since security isn't an
issue here.

Ulrich

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 507 bytes --]

Re: [gentoo-dev] [PATCH 1/2] esed.eclass: new eclass

[Ionen Wolkens]

[-- Attachment #1: Type: text/plain, Size: 8091 bytes --]

On Fri, Jun 03, 2022 at 09:14:05AM +0200, Ulrich Mueller wrote:
> >>>>> On Tue, 31 May 2022, Ionen Wolkens wrote:
> 
> > +# @FUNCTION: esed
> > +# @USAGE: <sed-argument>...
> > +# @DESCRIPTION:
> > +# sed(1) wrapper that dies if the expression(s) did not modify any files.
> > +# sed's -i/--in-place is forced, and so stdin/out cannot be used.
> 
> This sounds like a simple enough task ...
> 
> > +esed() {
> > +	local -i i
> > +
> > +	if [[ ${esedexps@a} =~ a ]]; then
> > +		# expression must be before -- but after the rest for e.g. -E to work
> > +		local -i pos
> > +		for ((pos=1; pos<=${#}; pos++)); do
> > +			[[ ${!pos} == -- ]] && break
> > +		done
> > +
> > +		for ((i=0; i<${#esedexps[@]}; i++)); do
> > +			[[ ${esedexps[i]} ]] &&
> > +				esedexps= esed "${@:1:pos-1}" -e "${esedexps[i]}" "${@:pos}"
> > +		done
> > +
> > +		unset esedexps
> > +		return 0
> > +	fi
> > +
> > +	# Roughly attempt to find files in arguments by checking if it's a
> > +	# readable file (aka s/// is not a file) and does not start with -
> > +	# (unless after --), then store contents for comparing after sed.
> > +	local contents=() endopts files=()
> > +	for ((i=1; i<=${#}; i++)); do
> > +		if [[ ${!i} == -- && ! -v endopts ]]; then
> > +			endopts=1
> > +		elif [[ ${!i} =~ ^(-i|--in-place)$ && ! -v endopts ]]; then
> > +			# detect rushed sed -i -> esed -i, -i also silently breaks enewsed
> > +			die "passing ${!i} to ${FUNCNAME[0]} is invalid"
> > +		elif [[ ${!i} =~ ^(-f|--file)$ && ! -v endopts ]]; then
> > +			i+=1 # ignore script files
> > +		elif [[ ( ${!i} != -* || -v endopts ) && -f ${!i} && -r ${!i} ]]; then
> > +			files+=( "${!i}" )
> > +
> > +			# eval 2>/dev/null to silence \0 warnings if sed binary files
> > +			eval 'contents+=( "$(<"${!i}")" )' 2>/dev/null \
> > +				|| die "failed to read: ${!i}"
> > +		fi
> > +	done
> > +	(( ${#files[@]} )) || die "no readable files found from '${*}' arguments"
> > +
> > +	local verbose
> > +	[[ ${ESED_VERBOSE} ]] && type diff &>/dev/null && verbose=1
> > +
> > +	local changed newcontents
> > +	if [[ -v _esed_output ]]; then
> > +		[[ -v verbose ]] &&
> > +			einfo "${FUNCNAME[0]}: sed ${*} > ${_esed_output} ..."
> > +
> > +		sed "${@}" > "${_esed_output}" \
> > +			|| die "failed to run: sed ${*} > ${_esed_output}"
> > +
> > +		eval 'newcontents=$(<${_esed_output})' 2>/dev/null \
> > +			|| die "failed to read: ${_esed_output}"
> > +
> > +		local IFS=$'\n' # sed concats with newline even if none at EOF
> > +		contents=${contents[*]}
> > +		unset IFS
> > +
> > +		[[ ${contents} != "${newcontents}" ]] && changed=1
> > +
> > +		[[ -v verbose ]] &&
> > +			diff -u --color --label="${files[*]}" --label="${_esed_output}" \
> > +				<(echo "${contents}") <(echo "${newcontents}")
> > +	else
> > +		[[ -v verbose ]] && einfo "${FUNCNAME[0]}: sed -i ${*} ..."
> > +
> > +		sed -i "${@}" || die "failed to run: sed -i ${*}"
> > +
> > +		for ((i=0; i<${#files[@]}; i++)); do
> > +			eval 'newcontents=$(<"${files[i]}")' 2>/dev/null \
> > +				|| die "failed to read: ${files[i]}"
> > +
> > +			if [[ ${contents[i]} != "${newcontents}" ]]; then
> > +				changed=1
> > +				[[ -v verbose ]] || break
> > +			fi
> > +
> > +			[[ -v verbose ]] &&
> > +				diff -u --color --label="${files[i]}"{,} \
> > +					<(echo "${contents[i]}") <(echo "${newcontents}")
> > +		done
> > +	fi
> > +
> > +	[[ -v changed ]] \
> > +		|| die "no-op: ${FUNCNAME[0]} ${*}${_esed_command:+ (from: ${_esed_command})}"
> > +}
> 
> ... but then it's almost 100 lines of shell code, including very
> convoluted parsing of parameters. The code for detection whether a
> parameter is actually a file is a heuristic at best and looks rather

Even if it does pickup sed -e "s_im-a_real-file_", it's a semi
non-issue given the file won't change before and after and it'll
evaluate the others for difference instead.

Where it gets a bit more complicated is enewsed given this is valid:
sed s/// file1 file2 > file3
Of course, could always ban multiple files on enewsed if felt really
needed, albeit fwiw it's a limitation for a very unlikely scenario.

"--" support is probably what adds the most complexity here, but
will have a problem if any ebuild ever needs to sed a -file
(-- is also useful in esedfind fwiw)

Also I did write tests so the minor complexity hopefully doesn't
lead to regressions.

iwdevtools' qa-sed uses getopt(long) instead, but even that is not
perfect given sed has special rules (plus wouldn't want to depend
on || ( getopt util-linux ) here).

> brittle. Also, don't use eval because it is evil.

This is static evals, they're just evaluating a flat string and it's
no different than.. well just running it as-is except it works around
a noise issue (the 2>/dev/null doesn't register without it).

eval is mostly evil when it's:
eval "${expanded_variable}=${what_is_this_even}"

Seeing eval "var=\${not_expanded}" as "evil" makes no sense to me,
it's a harmless warning silencer. I feel this is just overreaction
to the eval keyword (also in other dev ML post).

To reproduce the warning:
$ var=$(printf "\0") # var=$(<file-with-null-bytes)
bash: warning: command substitution: ignored null byte in input

doesn't work: var=$(printf "\0") 2>/dev/null
works: eval 'var=$(printf "\0")' 2>/dev/null

> 
> So IMHO this isn't the right approach to the problem. If anything, make
> it a simple function with well defined arguments, e.g. exactly one
> expression and exactly one file, and call "sed -i" on it.

I was hoping it wouldn't be a limitation to using sed, aka just add
checks and take very little away. Would've even liked to do 100% compat
like qa-sed (transparent stdin/out support), but that one is more
complicated and drew the line.

Lacking multiple files on esed (not enewsed) would be kinda annoying
I think. esedfind also makes use of the fact it can take multiple
files for when people need things like:

find . -name '*.c' -exec sed -i s/__AVX__/__AVX2__/ {} + || die
(esedfind would die if not one macro was replaced)

I tried to use esed in several ebuilds and see what felt needed or
is annoying without hoping people wouldn't mind using it over sed.

Goal here is not to replace sed nor encourage using it, but just add
a guard to ensure changes happened if it does get used. I've seen
plenty of ebuilds with a sed that's been broken for >5-10 years.

> 
> Then again, we had something similar in the past ("dosed" as a package
> manager command) but later banned it for good reason.

Well, dosed /used/ to be kinda useful to avoid needing:

mv file "${T}"/tmpfile || die
sed s/// "${T}"/tmpfile > file || die

It's only later that dosed started using -i itself and mostly didn't
have a purpose anymore given ebuilds could now use -i too, see:

https://bugs.gentoo.org/152017

> 
> Detection whether the file contents changed also seems complicated.
> Why not compute a hash of the file before and after sed operated on it?
> md5sum or even cksum should be good enough, since security isn't an
> issue here.

I don't think this would simplify beside now calling more external
commands twice then comparing the outputs (capturing command output vs
capturing file is not particularly different). May be faster on large
amount of files but probably not meaningful for single/few files.

This may make sed concatenation more complicated to handle too (cat
doesn't concatenate the same way sed does wrt newlines at EOF). Not
that couldn't ban that on enewsed, although it's more limitations
over not much.

>

All this aside, I'm not adamant about adding this. If not enough
interest then I'll just drop the idea. May try to implement multiple
expression support in iwdevtools' qa-sed instead, albeit being a
separate QA thing that doesn't know the maintainer's intend (aka
which seds are deterministic or not) and may also be missed, it
doesn't really ensure ebuilds' seds are kept up to date.

-- 
ionen

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [gentoo-dev] [PATCH 1/2] esed.eclass: new eclass

[Ionen Wolkens]

[-- Attachment #1: Type: text/plain, Size: 1304 bytes --]

On Fri, Jun 03, 2022 at 05:26:22AM -0400, Ionen Wolkens wrote:
> On Fri, Jun 03, 2022 at 09:14:05AM +0200, Ulrich Mueller wrote:
[...]
> > brittle. Also, don't use eval because it is evil.
> 
> This is static evals, they're just evaluating a flat string and it's
> no different than.. well just running it as-is except it works around
> a noise issue (the 2>/dev/null doesn't register without it).
> 
> eval is mostly evil when it's:
> eval "${expanded_variable}=${what_is_this_even}"
> 
> Seeing eval "var=\${not_expanded}" as "evil" makes no sense to me,
> it's a harmless warning silencer. I feel this is just overreaction
> to the eval keyword (also in other dev ML post).
> 
> To reproduce the warning:
> $ var=$(printf "\0") # var=$(<file-with-null-bytes)
> bash: warning: command substitution: ignored null byte in input
> 
> doesn't work: var=$(printf "\0") 2>/dev/null
> works: eval 'var=$(printf "\0")' 2>/dev/null

Was experimenting for what else works just now.

{ var=$(printf "\0"); } 2>/dev/null

I feel like I remember trying this before for qa-sed but don't remember
why I didn't use it (unlike subshells it does keep the var readable).

I still don't think that a flat eval was "evil" but if I have a way
to avoid it I'll use that instead.

-- 
ionen

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[gentoo-dev] [PATCH 2/2] eclass/tests/esed.sh: basic tests for esed.eclass

[Ionen Wolkens]

Bit sloppy, but should pickup most regressions.

Signed-off-by: Ionen Wolkens <ionen@gentoo.org>
---
 eclass/tests/esed.sh | 173 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 173 insertions(+)
 create mode 100755 eclass/tests/esed.sh

diff --git a/eclass/tests/esed.sh b/eclass/tests/esed.sh
new file mode 100755
index 00000000000..fad1319ea13
--- /dev/null
+++ b/eclass/tests/esed.sh
@@ -0,0 +1,173 @@
+#!/usr/bin/env bash
+# Copyright 2022 Gentoo Authors
+# Distributed under the terms of the GNU General Public License v2
+
+EAPI=8
+source tests-common.sh || exit
+
+inherit esed
+
+cd "${WORKDIR:-/dev/null}" || exit 1
+
+tsddied=n
+tsddie() {
+	tsddied=y
+	echo "would die: $*" >&2
+	# silence further errors given didn't actually die
+	sed() { :; }
+	die() { :; }
+}
+
+tsdbegin() {
+	tbegin "${1}"
+	tsddied=n
+	unset -f sed
+	die() { tsddie "${@}"; }
+}
+
+tsdend() {
+	if [[ ${1} == fatal && ${tsddied} == n ]]; then
+		tend 127 "should have died"
+	elif [[ ${1} == nonfatal && ${tsddied} == y ]]; then
+		tend 128 "should not have died"
+	else
+		tend ${2:-0} "${3:-something went wrong(tm)}"
+	fi
+}
+
+tsdfile() {
+	local file
+	for file in "${@}"; do
+		if [[ ${file%:*} ]]; then
+			echo "${file%:*}" > "${file#*:}" || exit 1
+		elif [[ -e ${file#*:} ]]; then
+			rm -- "${file#*:}" || exit 1
+		fi
+	done
+}
+
+tsdcmp() {
+	local contents
+	contents=$(<"${1}") || exit 1
+	[[ ${contents} == "${2}" ]]
+}
+
+tsdbegin "esed: change on single file"
+tsdfile replace:file
+esed s/replace/new/ file
+tsdcmp file new
+tsdend nonfatal
+
+tsdbegin "esed: die due to no change on a single file"
+tsdfile keep:file
+esed s/replace/new/ file
+tsdcmp file keep
+tsdend fatal ${?}
+
+tsdbegin "esed: change on at least one of two files with ESED_VERBOSE=1"
+tsdfile keep:file1 replace:file2
+ESED_VERBOSE=1 esed s/replace/new/ file1 file2
+tsdcmp file1 keep &&
+	tsdcmp file2 new
+tsdend nonfatal ${?}
+
+tsdbegin "esed: die due to no change on two files with ESED_VERBOSE=1"
+tsdfile keep:file{1..2}
+ESED_VERBOSE=1 esed s/replace/new/ file1 file2
+tsdcmp file1 keep &&
+	tsdcmp file2 keep
+tsdend fatal ${?}
+
+tsdbegin "esed: change using esedexps"
+tsdfile replace1-replace2:file
+esedexps=(
+	s/replace1/new1/
+	s/replace2/new2/
+)
+esed file
+tsdcmp file new1-new2
+tsdend nonfatal ${?}
+
+tsdbegin "esed: don't call sed with empty esedexps"
+tsdfile keep:file
+esedexps=( '' )
+esed file
+[[ ! -v esedexps ]]
+tsdend nonfatal ${?}
+
+tsdbegin "esed: die due to passing -i"
+esed -i s/replace/new/
+tsdend fatal
+
+tsdbegin "esed: change files with one nicely named '-- -i' using esedexp extended regex"
+tsdfile replace:"-- -i" replace:file
+esedexps=(
+	's/(replace)/\1-new/'
+)
+esed -E file -- "-- -i"
+tsdcmp file replace-new &&
+	tsdcmp "-- -i" replace-new
+tsdend nonfatal ${?}
+
+tsdbegin "esed: die due to no files found"
+esed s/replace/new/
+tsdend fatal
+
+tsdbegin "esed: die due to invalid sed use"
+tsdfile keep:file
+esed s/replace/new file
+tsdend fatal
+
+tsdbegin "enewsed: change on a new file"
+tsdfile replace:file :newfile
+enewsed s/replace/new/ file newfile
+tsdcmp file replace &&
+	tsdcmp newfile new
+tsdend nonfatal ${?}
+
+tsdbegin "enewsed: die due to no changes on a newfile"
+tsdfile keep:file :newfile
+enewsed s/replace/new/ file newfile
+tsdcmp file keep &&
+	tsdcmp newfile keep
+tsdend fatal ${?}
+
+tsdbegin "enewsed: concatenating changed files to a newfile"
+tsdfile keep:file1 replace:file2
+enewsed s/replace/new/ file1 file2 newfile
+tsdcmp file1 keep &&
+	tsdcmp file2 replace &&
+	tsdcmp newfile $'keep\nnew'
+tsdend nonfatal ${?}
+
+tsdbegin "enewsed: script-file not being concatenated"
+tsdfile 's/missing/new/':script keep:file :newfile
+enewsed file -f script newfile
+tsdcmp script 's/missing/new/' &&
+	tsdcmp file keep &&
+	tsdcmp newfile keep
+tsdend fatal ${?}
+
+tsdbegin "esedfind: change found files"
+tsdfile keep:file1.find replace:file2.find
+esedfind . -type f -name '*.find' -esed s/replace/new/
+tsdcmp file1.find keep &&
+	tsdcmp file2.find new
+tsdend nonfatal ${?}
+
+tsdbegin "esedfind: die due to no files found"
+esedfind . -type f -name '*.missing' -esed s/replace/new/
+tsdend fatal
+
+tsdbegin "esedfind: die due to no changes from one esedexps without -esed"
+tsdfile keep:file1.find2 replace:file2.find2
+esedexps=(
+	s/replace/new/
+	s/missing/new/
+)
+esedfind . -type f -name '*.find2'
+tsdcmp file1.find2 keep &&
+	tsdcmp file2.find2 new
+tsdend fatal ${?}
+
+texit
-- 
2.35.1

Re: [gentoo-dev] [PATCH 0/2] Add esed.eclass for sed that dies if caused no changes

[Anna]

On 2022-05-31 07:23, Ionen Wolkens wrote:
> Implementation / available wrappers / usefulness still up for debate,
> but without further comments I consider this ready (albeit first time
> touching / making an eclass, so I could be overlooking simple things).
> Also partly uses >=bash-4.4, so EAPI-7 is not considered.

From ebuild(5):
> Beginning with EAPI 4, the dosed helper no longer exists
> Ebuilds should call sed(1) directly (and assume that it is GNU
> sed).

What were the reasons to remove this helper? Do they still apply to esed
as an eclass?

Re: [gentoo-dev] [PATCH 0/2] Add esed.eclass for sed that dies if caused no changes

[Ionen Wolkens]

[-- Attachment #1: Type: text/plain, Size: 1156 bytes --]

On Tue, May 31, 2022 at 06:54:21PM +0500, Anna wrote:
> On 2022-05-31 07:23, Ionen Wolkens wrote:
> > Implementation / available wrappers / usefulness still up for debate,
> > but without further comments I consider this ready (albeit first time
> > touching / making an eclass, so I could be overlooking simple things).
> > Also partly uses >=bash-4.4, so EAPI-7 is not considered.
> 
> From ebuild(5):
> > Beginning with EAPI 4, the dosed helper no longer exists
> > Ebuilds should call sed(1) directly (and assume that it is GNU
> > sed).
> 
> What were the reasons to remove this helper? Do they still apply to esed
> as an eclass?

Haven't looked up exact history, but afaik dosed existed because of sed
formerly not having -i. And editing files in-place was tedious in
ebuilds with cp/mv. With -i becoming commonplace (PMS also requires
GNU sed) it lost its purpose beside just allowing to skip -i/||die.

esed does bring back the -i/die skipping but that's not its inherent
purpose and GNU sed currently does not support a mean to report if
changes occurred (if this happens, esed may well become obsolete too).

-- 
ionen

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [gentoo-dev] [PATCH 0/2] Add esed.eclass for sed that dies if caused no changes

[Jaco Kroon]

Hi,

On 2022/05/31 16:29, Ionen Wolkens wrote:
> esed does bring back the -i/die skipping but that's not its inherent
> purpose and GNU sed currently does not support a mean to report if
> changes occurred (if this happens, esed may well become obsolete too).
>
Haven't checked the code to validate.  But I'm in support of esed eclass
for the sole reason it ensures the sed actually still changes
something.  Not that it verifies the change is actually intended mind
you, so it doesn't completely take away the due diligence checks, but
the verbose options should help devs to ensure the changes are the
intended at testing time.

One *could* also add a change-counter check, eg, count the number of
lines starting with + and - (but not +++ and ---) from diff, and ensure
they match the expected count.  Just to catch further possible cases.

Kind Regards,
Jaco

Re: [gentoo-dev] [PATCH 0/2] Add esed.eclass for sed that dies if caused no changes

[Michał Górny]

On Tue, 2022-05-31 at 07:23 -0400, Ionen Wolkens wrote:
> Often preferable to use patches so this happens, but sed have its
> uses/convenience and this intend to help reduce the amount of old
> broken seds causing issues that go unnoticed on bumps.
> 
> Inspired by app-portage/iwdevtools' qa-sed (warns on any seds), but
> this is for more deterministic use in ebuilds.
> 
> Also slightly shortens sed use, -i is default, and no need to || die.
> (see @EXAMPLE in eclass for a quick usage overview).
> 

To be honest, I strongly dislike this.  It really feels like trying to
make an adapter for a square wheel, while the right solution would be to
replace the wheel.  On top of that, ton of evals which are pretty much
a huge "no-no".

Perhaps it would be better to forget about trying to work miracles with
sed and instead write a trivial shell replacement for the most common
use cases.  One thing I'd love to see is a simple substitution command
that would work for paths/CFLAGS on RHS without having to worry about
them conflicting with the pattern delimiter.

-- 
Best regards,
Michał Górny

Re: [gentoo-dev] [PATCH 0/2] Add esed.eclass for sed that dies if caused no changes

[Ionen Wolkens]

[-- Attachment #1: Type: text/plain, Size: 1612 bytes --]

On Fri, Jun 03, 2022 at 06:09:38AM +0200, Michał Górny wrote:
> On Tue, 2022-05-31 at 07:23 -0400, Ionen Wolkens wrote:
> > Often preferable to use patches so this happens, but sed have its
> > uses/convenience and this intend to help reduce the amount of old
> > broken seds causing issues that go unnoticed on bumps.
> > 
> > Inspired by app-portage/iwdevtools' qa-sed (warns on any seds), but
> > this is for more deterministic use in ebuilds.
> > 
> > Also slightly shortens sed use, -i is default, and no need to || die.
> > (see @EXAMPLE in eclass for a quick usage overview).
> > 
> 
> To be honest, I strongly dislike this.  It really feels like trying to
> make an adapter for a square wheel, while the right solution would be to
> replace the wheel.  On top of that, ton of evals which are pretty much
> a huge "no-no".

About evals, the two eval is just to silence this:

    var=$(printf "\0")

The 2>/dev/null doesn't work without wrapping it, aka

    eval 'var=$(printf "\0")' 2>/dev/null

No variables are expanded pre-eval so it's just evaluating a
static statement. eval can be removed, but it'll be noisy
if someone happens to sed binary files.

> 
> Perhaps it would be better to forget about trying to work miracles with
> sed and instead write a trivial shell replacement for the most common
> use cases.  One thing I'd love to see is a simple substitution command
> that would work for paths/CFLAGS on RHS without having to worry about
> them conflicting with the pattern delimiter.
> 
> -- 
> Best regards,
> Michał Górny
> 
> 

-- 
ionen

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [gentoo-dev] [PATCH 0/2] Add esed.eclass for sed that dies if caused no changes

[Anna V]

On 2022-06-03 00:45, Ionen Wolkens wrote:
> On Fri, Jun 03, 2022 at 06:09:38AM +0200, Michał Górny wrote:
> > On Tue, 2022-05-31 at 07:23 -0400, Ionen Wolkens wrote:
> > > Often preferable to use patches so this happens, but sed have its
> > > uses/convenience and this intend to help reduce the amount of old
> > > broken seds causing issues that go unnoticed on bumps.
> > > 
> > > Inspired by app-portage/iwdevtools' qa-sed (warns on any seds), but
> > > this is for more deterministic use in ebuilds.
> > > 
> > > Also slightly shortens sed use, -i is default, and no need to || die.
> > > (see @EXAMPLE in eclass for a quick usage overview).
> > > 
> > 
> > To be honest, I strongly dislike this.  It really feels like trying to
> > make an adapter for a square wheel, while the right solution would be to
> > replace the wheel.  On top of that, ton of evals which are pretty much
> > a huge "no-no".
> 
> About evals, the two eval is just to silence this:
> 
>     var=$(printf "\0")

printf -v var "\0"

> The 2>/dev/null doesn't work without wrapping it, aka
> 
>     eval 'var=$(printf "\0")' 2>/dev/null
> 
> No variables are expanded pre-eval so it's just evaluating a
> static statement. eval can be removed, but it'll be noisy
> if someone happens to sed binary files.
> 
> > 
> > Perhaps it would be better to forget about trying to work miracles with
> > sed and instead write a trivial shell replacement for the most common
> > use cases.  One thing I'd love to see is a simple substitution command
> > that would work for paths/CFLAGS on RHS without having to worry about
> > them conflicting with the pattern delimiter.
> > 
> > -- 
> > Best regards,
> > Michał Górny
> > 
> > 
> 
> -- 
> ionen

Re: [gentoo-dev] [PATCH 0/2] Add esed.eclass for sed that dies if caused no changes

[Ionen Wolkens]

[-- Attachment #1: Type: text/plain, Size: 2073 bytes --]

On Fri, Jun 03, 2022 at 09:47:51AM +0500, Anna V wrote:
> On 2022-06-03 00:45, Ionen Wolkens wrote:
> > On Fri, Jun 03, 2022 at 06:09:38AM +0200, Michał Górny wrote:
> > > On Tue, 2022-05-31 at 07:23 -0400, Ionen Wolkens wrote:
> > > > Often preferable to use patches so this happens, but sed have its
> > > > uses/convenience and this intend to help reduce the amount of old
> > > > broken seds causing issues that go unnoticed on bumps.
> > > > 
> > > > Inspired by app-portage/iwdevtools' qa-sed (warns on any seds), but
> > > > this is for more deterministic use in ebuilds.
> > > > 
> > > > Also slightly shortens sed use, -i is default, and no need to || die.
> > > > (see @EXAMPLE in eclass for a quick usage overview).
> > > > 
> > > 
> > > To be honest, I strongly dislike this.  It really feels like trying to
> > > make an adapter for a square wheel, while the right solution would be to
> > > replace the wheel.  On top of that, ton of evals which are pretty much
> > > a huge "no-no".
> > 
> > About evals, the two eval is just to silence this:
> > 
> >     var=$(printf "\0")
> 
> printf -v var "\0"

That was just for the sake of showing what happens, the eclass is
reading a file and isn't using printf. aka: var=$(<file)

> 
> > The 2>/dev/null doesn't work without wrapping it, aka
> > 
> >     eval 'var=$(printf "\0")' 2>/dev/null
> > 
> > No variables are expanded pre-eval so it's just evaluating a
> > static statement. eval can be removed, but it'll be noisy
> > if someone happens to sed binary files.
> > 
> > > 
> > > Perhaps it would be better to forget about trying to work miracles with
> > > sed and instead write a trivial shell replacement for the most common
> > > use cases.  One thing I'd love to see is a simple substitution command
> > > that would work for paths/CFLAGS on RHS without having to worry about
> > > them conflicting with the pattern delimiter.
> > > 
> > > -- 
> > > Best regards,
> > > Michał Górny
> > > 
> > > 
> > 
> > -- 
> > ionen
> 
> 
> 

-- 
ionen

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [gentoo-dev] [PATCH 0/2] Add esed.eclass for sed that dies if caused no changes

[Anna V]

On 2022-06-03 00:45, Ionen Wolkens wrote:
> On Fri, Jun 03, 2022 at 06:09:38AM +0200, Michał Górny wrote:
> > On Tue, 2022-05-31 at 07:23 -0400, Ionen Wolkens wrote:
> > > Often preferable to use patches so this happens, but sed have its
> > > uses/convenience and this intend to help reduce the amount of old
> > > broken seds causing issues that go unnoticed on bumps.
> > > 
> > > Inspired by app-portage/iwdevtools' qa-sed (warns on any seds), but
> > > this is for more deterministic use in ebuilds.
> > > 
> > > Also slightly shortens sed use, -i is default, and no need to || die.
> > > (see @EXAMPLE in eclass for a quick usage overview).
> > > 
> > 
> > To be honest, I strongly dislike this.  It really feels like trying to
> > make an adapter for a square wheel, while the right solution would be to
> > replace the wheel.  On top of that, ton of evals which are pretty much
> > a huge "no-no".
> 
> About evals, the two eval is just to silence this:
> 
>     var=$(printf "\0")
 
Or in ksh93 syntax:

var=$'\0'

> The 2>/dev/null doesn't work without wrapping it, aka
> 
>     eval 'var=$(printf "\0")' 2>/dev/null
> 
> No variables are expanded pre-eval so it's just evaluating a
> static statement. eval can be removed, but it'll be noisy
> if someone happens to sed binary files.
> 
> > 
> > Perhaps it would be better to forget about trying to work miracles with
> > sed and instead write a trivial shell replacement for the most common
> > use cases.  One thing I'd love to see is a simple substitution command
> > that would work for paths/CFLAGS on RHS without having to worry about
> > them conflicting with the pattern delimiter.
> > 
> > -- 
> > Best regards,
> > Michał Górny
> > 
> > 
> 
> -- 
> ionen

Re: [gentoo-dev] [PATCH 0/2] Add esed.eclass for sed that dies if caused no changes

[Ionen Wolkens]

[-- Attachment #1: Type: text/plain, Size: 1484 bytes --]

On Fri, Jun 03, 2022 at 06:09:38AM +0200, Michał Górny wrote:
> On Tue, 2022-05-31 at 07:23 -0400, Ionen Wolkens wrote:
> > Often preferable to use patches so this happens, but sed have its
> > uses/convenience and this intend to help reduce the amount of old
> > broken seds causing issues that go unnoticed on bumps.
> > 
> > Inspired by app-portage/iwdevtools' qa-sed (warns on any seds), but
> > this is for more deterministic use in ebuilds.
> > 
> > Also slightly shortens sed use, -i is default, and no need to || die.
> > (see @EXAMPLE in eclass for a quick usage overview).
> > 
> 
> To be honest, I strongly dislike this.  It really feels like trying to
> make an adapter for a square wheel, while the right solution would be to
> replace the wheel.  On top of that, ton of evals which are pretty much
> a huge "no-no".

As for using this or not, I don't overly mind ether way. I felt like
giving it a try given it's been requested but if there's not much
interest that's fine too (there's still qa-sed fwiw, with some
limitations).

> 
> Perhaps it would be better to forget about trying to work miracles with
> sed and instead write a trivial shell replacement for the most common
> use cases.  One thing I'd love to see is a simple substitution command
> that would work for paths/CFLAGS on RHS without having to worry about
> them conflicting with the pattern delimiter.
> 
> -- 
> Best regards,
> Michał Górny
> 
> 

-- 
ionen

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [gentoo-dev] [PATCH 0/2] Add esed.eclass for sed that dies if caused no changes

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 2148 bytes --]



> On 31 May 2022, at 12:23, Ionen Wolkens <ionen@gentoo.org> wrote:
> 
> Often preferable to use patches so this happens, but sed have its
> uses/convenience and this intend to help reduce the amount of old
> broken seds causing issues that go unnoticed on bumps.
> 
> Inspired by app-portage/iwdevtools' qa-sed (warns on any seds), but
> this is for more deterministic use in ebuilds.
> 
> Also slightly shortens sed use, -i is default, and no need to || die.
> (see @EXAMPLE in eclass for a quick usage overview).
> 
> Implementation / available wrappers / usefulness still up for debate,
> but without further comments I consider this ready (albeit first time
> touching / making an eclass, so I could be overlooking simple things).
> Also partly uses >=bash-4.4, so EAPI-7 is not considered.
> 
> See github PR[1] for old changelog background.
> 
> Up to maintainers but personally would encourage to slowly replace
> (almost) all use of sed with either this or patches. Some cases
> where it can be inconvenient like eclasses "guessing" that a package
> may or may not have something to replace, and that nothing happened
> is not an issue.
> 

I like it. I'd prefer it if GNU sed supported it by itself (exit code indicating
if any changes were made) but it doesn't right now.

Ebuilds use sed aplenty and making it easier to be "less bad" is a good
thing. It's a practical solution to a real problem we have (zombie seds,
or more rarely, overzealous-and-not-realising-it seds).

I don't want us to have to keep it forever and I wouldn't want
people to actively use this instead of patches, but they certainly should
instead of sed.

> [1] https://github.com/gentoo/gentoo/pull/25662
> 
> Ionen Wolkens (2):
>  esed.eclass: new eclass
>  eclass/tests/esed.sh: basic tests for esed.eclass
> 
> eclass/esed.eclass   | 199 +++++++++++++++++++++++++++++++++++++++++++
> eclass/tests/esed.sh | 173 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 372 insertions(+)
> create mode 100644 eclass/esed.eclass
> create mode 100755 eclass/tests/esed.sh
> 
> --
> 2.35.1
> 

best,
sam


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: [gentoo-dev] [PATCH 0/2] Add esed.eclass for sed that dies if caused no changes

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]



> On 3 Jun 2022, at 05:59, Sam James <sam@gentoo.org> wrote:
> 
> 
> 
>> On 31 May 2022, at 12:23, Ionen Wolkens <ionen@gentoo.org> wrote:
>> 
>> Often preferable to use patches so this happens, but sed have its
>> uses/convenience and this intend to help reduce the amount of old
>> broken seds causing issues that go unnoticed on bumps.
>> 
>> Inspired by app-portage/iwdevtools' qa-sed (warns on any seds), but
>> this is for more deterministic use in ebuilds.
>> 
>> Also slightly shortens sed use, -i is default, and no need to || die.
>> (see @EXAMPLE in eclass for a quick usage overview).
>> 
>> Implementation / available wrappers / usefulness still up for debate,
>> but without further comments I consider this ready (albeit first time
>> touching / making an eclass, so I could be overlooking simple things).
>> Also partly uses >=bash-4.4, so EAPI-7 is not considered.
>> 
>> See github PR[1] for old changelog background.
>> 
>> Up to maintainers but personally would encourage to slowly replace
>> (almost) all use of sed with either this or patches. Some cases
>> where it can be inconvenient like eclasses "guessing" that a package
>> may or may not have something to replace, and that nothing happened
>> is not an issue.
>> 
> 
> I like it. I'd prefer it if GNU sed supported it by itself (exit code indicating
> if any changes were made) but it doesn't right now.
> 
> Ebuilds use sed aplenty and making it easier to be "less bad" is a good
> thing. It's a practical solution to a real problem we have (zombie seds,
> or more rarely, overzealous-and-not-realising-it seds).
> 
> I don't want us to have to keep it forever and I wouldn't want
> people to actively use this instead of patches, but they certainly should
> instead of sed.
> 

Also, I like that we let ourselves experiment a bit with edo.eclass,
and I don't see this as too different from that. Unless something is
absolutely the wrong approach and we know it won't go anywhere,
I think exploring options is generally a good thing.

Plus, we've seen the actual need for this from iwdevtools usage
(warns on such zombie seds). Let's give it a go, I think?

>> [1] https://github.com/gentoo/gentoo/pull/25662
>> 
>> Ionen Wolkens (2):
>> esed.eclass: new eclass
>> eclass/tests/esed.sh: basic tests for esed.eclass
>> 
>> eclass/esed.eclass | 199 +++++++++++++++++++++++++++++++++++++++++++
>> eclass/tests/esed.sh | 173 +++++++++++++++++++++++++++++++++++++
>> 2 files changed, 372 insertions(+)
>> create mode 100644 eclass/esed.eclass
>> create mode 100755 eclass/tests/esed.sh
>> 
>> --
>> 2.35.1
>> 
> 
> best,
> sam


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

[gentoo-dev] [PATCH v2 0/1] esed

[Ionen Wolkens]

Nothing fundamentally different until further feedback,
mostly to get eval out of the way.

Changelog
v2:
 - replace use of eval (realized another way works)
 - add missing quotes around one _esed_output
 - move diff checks inside comparison blocks

Ionen Wolkens (1):
  esed.eclass: new eclass

 eclass/esed.eclass | 201 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 201 insertions(+)
 create mode 100644 eclass/esed.eclass

-- 
2.35.1

[gentoo-dev] [PATCH v2 1/1] esed.eclass: new eclass

[Ionen Wolkens]

Signed-off-by: Ionen Wolkens <ionen@gentoo.org>
---
 eclass/esed.eclass | 201 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 201 insertions(+)
 create mode 100644 eclass/esed.eclass

diff --git a/eclass/esed.eclass b/eclass/esed.eclass
new file mode 100644
index 00000000000..f327c3bbdf4
--- /dev/null
+++ b/eclass/esed.eclass
@@ -0,0 +1,201 @@
+# Copyright 2022 Gentoo Authors
+# Distributed under the terms of the GNU General Public License v2
+
+# @ECLASS: esed.eclass
+# @MAINTAINER:
+# Ionen Wolkens <ionen@gentoo.org>
+# @AUTHOR:
+# Ionen Wolkens <ionen@gentoo.org>
+# @SUPPORTED_EAPIS: 8
+# @BLURB: sed(1) wrappers that die if expressions did not modify any files
+# @EXAMPLE:
+#
+# @CODE
+# esed 's/a/b/' src/file.c # -i is default, dies if 'a' does not become 'b'
+#
+# enewsed 's/a/b/' project.pc.in "${T}"/project.pc # stdin/out not supported
+#
+# esedfind . -type f -name '*.c' -esed 's/a/b/' # dies if zero files changed
+#
+# local esedexps=(
+#     # dies if /any/ of these did nothing, -e 's/a/b/' -e 's/c/d/' would not
+#     's/a/b/'
+#     's/c/d/' # bug 000000
+#     # use quotes around "$(use..)" to avoid word splitting/globs, won't run
+#     # sed(1) for empty elements (i.e. if USE is disabled)
+#     "$(usev fnord "s/foo bar/${baz}/")"
+# )
+# esed Makefile lib/Makefile # unsets esedexps so it's not re-used
+#
+# use prefix && esed "s|^prefix=|&${EPREFIX}|" project.pc # deterministic
+# @CODE
+#
+# Migration note: be wary of non-deterministic esed() involving variables,
+# e.g. s|lib|$(get_libdir)|, s|-O3|${CFLAGS}|, and the above ${EPREFIX} one.
+# esed() dies if these do nothing, like libdir being 'lib' on x86.  Either
+# verify, keep sed(1), or ensure a change (extra space, @placeholders@).
+
+case ${EAPI} in
+	8) ;;
+	*) die "${ECLASS}: EAPI ${EAPI:-0} not supported" ;;
+esac
+
+if [[ ! -v _ESED_ECLASS ]]; then
+_ESED_ECLASS=1
+
+# @ECLASS_VARIABLE: ESED_VERBOSE
+# @DEFAULT_UNSET
+# @USER_VARIABLE
+# @DESCRIPTION:
+# If set to a non-empty value, esed() and its wrappers will use diff(1)
+# if available to display file differences.
+
+# @VARIABLE: esedexps
+# @DEFAULT_UNSET
+# @DESCRIPTION:
+# Bash array that can optionally contain sed expressions to use sequencially
+# on separate sed calls when using esed() and its wrappers.  Allows inspection
+# of modifications per-expressions.  Unset after use so it's not used in
+# subsequent calls.  Will not run sed(1) for empty array elements.
+
+# @FUNCTION: esed
+# @USAGE: <sed-argument>...
+# @DESCRIPTION:
+# sed(1) wrapper that dies if the expression(s) did not modify any files.
+# sed's -i/--in-place is forced, and so stdin/out cannot be used.
+esed() {
+	local -i i
+
+	if [[ ${esedexps@a} =~ a ]]; then
+		# expression must be before -- but after the rest for e.g. -E to work
+		local -i pos
+		for ((pos=1; pos<=${#}; pos++)); do
+			[[ ${!pos} == -- ]] && break
+		done
+
+		for ((i=0; i<${#esedexps[@]}; i++)); do
+			[[ ${esedexps[i]} ]] &&
+				esedexps= esed "${@:1:pos-1}" -e "${esedexps[i]}" "${@:pos}"
+		done
+
+		unset esedexps
+		return 0
+	fi
+
+	# Roughly attempt to find files in arguments by checking if it's a
+	# readable file (aka s/// is not a file) and does not start with -
+	# (unless after --), then store contents for comparing after sed.
+	local contents=() endopts files=()
+	for ((i=1; i<=${#}; i++)); do
+		if [[ ${!i} == -- && ! -v endopts ]]; then
+			endopts=1
+		elif [[ ${!i} =~ ^(-i|--in-place)$ && ! -v endopts ]]; then
+			# detect rushed sed -i -> esed -i, -i also silently breaks enewsed
+			die "passing ${!i} to ${FUNCNAME[0]} is invalid"
+		elif [[ ${!i} =~ ^(-f|--file)$ && ! -v endopts ]]; then
+			i+=1 # ignore script files
+		elif [[ ( ${!i} != -* || -v endopts ) && -f ${!i} && -r ${!i} ]]; then
+			files+=( "${!i}" )
+
+			# 2>/dev/null to silence null byte warnings if sed binary files
+			{ contents+=( "$(<"${!i}")" ); } 2>/dev/null \
+				|| die "failed to read: ${!i}"
+		fi
+	done
+	(( ${#files[@]} )) || die "no readable files found from '${*}' arguments"
+
+	local verbose
+	[[ ${ESED_VERBOSE} ]] && type diff &>/dev/null && verbose=1
+
+	local changed newcontents
+	if [[ -v _esed_output ]]; then
+		[[ -v verbose ]] &&
+			einfo "${FUNCNAME[0]}: sed ${*} > ${_esed_output} ..."
+
+		sed "${@}" > "${_esed_output}" \
+			|| die "failed to run: sed ${*} > ${_esed_output}"
+
+		{ newcontents=$(<"${_esed_output}"); } 2>/dev/null \
+			|| die "failed to read: ${_esed_output}"
+
+		local IFS=$'\n' # sed concats with newline even if none at EOF
+		contents=${contents[*]}
+		unset IFS
+
+		if [[ ${contents} != "${newcontents}" ]]; then
+			 changed=1
+
+			[[ -v verbose ]] &&
+				diff -u --color --label="${files[*]}" --label="${_esed_output}" \
+					<(echo "${contents}") <(echo "${newcontents}")
+		fi
+	else
+		[[ -v verbose ]] && einfo "${FUNCNAME[0]}: sed -i ${*} ..."
+
+		sed -i "${@}" || die "failed to run: sed -i ${*}"
+
+		for ((i=0; i<${#files[@]}; i++)); do
+			{ newcontents=$(<"${files[i]}"); } 2>/dev/null \
+				|| die "failed to read: ${files[i]}"
+
+			if [[ ${contents[i]} != "${newcontents}" ]]; then
+				changed=1
+
+				[[ -v verbose ]] || break
+
+				diff -u --color --label="${files[i]}"{,} \
+					<(echo "${contents[i]}") <(echo "${newcontents}")
+			fi
+		done
+	fi
+
+	[[ -v changed ]] \
+		|| die "no-op: ${FUNCNAME[0]} ${*}${_esed_command:+ (from: ${_esed_command})}"
+}
+
+# @FUNCTION: enewsed
+# @USAGE: <esed-argument>... <output-file>
+# @DESCRIPTION:
+# esed() wrapper to save the result to <output-file>.  Same as using
+# `sed ... input > output` given esed() does not support stdin/out.
+enewsed() {
+	local _esed_command="${FUNCNAME[0]} ${*}"
+	local _esed_output=${*: -1:1}
+	esed "${@:1:${#}-1}"
+}
+
+# @FUNCTION: esedfind
+# @USAGE: <find-argument>... [-esed [<esed-argument>...]]
+# @DESCRIPTION:
+# esed() wrapper to ease use with find(1) given -exec wouldn't see a shell
+# function.  Will die if find(1) found no files, or if not a single file
+# was changed.  -esed is optional with the esedexps=( .. ) array.  -print0
+# will be appended to <find-arguments>.
+#
+# Requires that the found list not exceed args limit for file changes to be
+# evaluated together in a single esed() call.  Use is discouraged if modifying
+# files with a large total size (50+MB), as they will be loaded in memory
+# and compared ineffectively by the shell.
+esedfind() {
+	local _esed_command="${FUNCNAME[0]} ${*}"
+
+	local find=( find )
+	while (( ${#} )); do
+		if [[ ${1} == -esed ]]; then
+			shift
+			break
+		fi
+		find+=( "${1}" )
+		shift
+	done
+	find+=( -print0 )
+
+	local files
+	mapfile -d '' -t files < <("${find[@]}" || die "failed to run: ${find[*]}")
+
+	(( ${#files[@]} )) || die "no files found from: ${find[*]}"
+
+	esed "${@}" -- "${files[@]}"
+}
+
+fi
-- 
2.35.1

Re: [gentoo-dev] [PATCH v2 1/1] esed.eclass: new eclass

[Oskari Pirhonen]

[-- Attachment #1: Type: text/plain, Size: 3683 bytes --]

On Fri, Jun 03, 2022 at 07:36:46AM -0400, Ionen Wolkens wrote:
> ... snip ...
>
> +	# Roughly attempt to find files in arguments by checking if it's a
> +	# readable file (aka s/// is not a file) and does not start with -
> +	# (unless after --), then store contents for comparing after sed.
> +	local contents=() endopts files=()
> +	for ((i=1; i<=${#}; i++)); do
> +		if [[ ${!i} == -- && ! -v endopts ]]; then
> +			endopts=1
> +		elif [[ ${!i} =~ ^(-i|--in-place)$ && ! -v endopts ]]; then
> +			# detect rushed sed -i -> esed -i, -i also silently breaks enewsed
> +			die "passing ${!i} to ${FUNCNAME[0]} is invalid"
> +		elif [[ ${!i} =~ ^(-f|--file)$ && ! -v endopts ]]; then
> +			i+=1 # ignore script files
> +		elif [[ ( ${!i} != -* || -v endopts ) && -f ${!i} && -r ${!i} ]]; then
> +			files+=( "${!i}" )
> +
> +			# 2>/dev/null to silence null byte warnings if sed binary files
> +			{ contents+=( "$(<"${!i}")" ); } 2>/dev/null \
> +				|| die "failed to read: ${!i}"
> +		fi
> +	done
> +	(( ${#files[@]} )) || die "no readable files found from '${*}' arguments"
> +
> +	local verbose
> +	[[ ${ESED_VERBOSE} ]] && type diff &>/dev/null && verbose=1
> +
> +	local changed newcontents
> +	if [[ -v _esed_output ]]; then
> +		[[ -v verbose ]] &&
> +			einfo "${FUNCNAME[0]}: sed ${*} > ${_esed_output} ..."
> +
> +		sed "${@}" > "${_esed_output}" \
> +			|| die "failed to run: sed ${*} > ${_esed_output}"
> +
> +		{ newcontents=$(<"${_esed_output}"); } 2>/dev/null \
> +			|| die "failed to read: ${_esed_output}"
> +
> +		local IFS=$'\n' # sed concats with newline even if none at EOF
> +		contents=${contents[*]}
> +		unset IFS
> +
> +		if [[ ${contents} != "${newcontents}" ]]; then
> +			 changed=1
> +
> +			[[ -v verbose ]] &&
> +				diff -u --color --label="${files[*]}" --label="${_esed_output}" \
> +					<(echo "${contents}") <(echo "${newcontents}")
> +		fi
>
> ... snip ...

I'm not 100% convinced that it will give you anything meaningful. The
warning about ignoring NULL is not so much noise as it is bash warning
you that you're probably not doing something correctly. In this case,
you're not pulling _all_ the contents of the file:
    
    [ /tmp ]
    oskari@dj3ntoo λ printf "ab\0cd" >test.dat
    [ /tmp ]
    oskari@dj3ntoo λ hd test.dat
    00000000  61 62 00 63 64                                    |ab.cd|
    00000005
    [ /tmp ]
    oskari@dj3ntoo λ var=$(< test.dat)
    bash: warning: command substitution: ignored null byte in input
    [ /tmp ]
    oskari@dj3ntoo λ printf "$var" | hd
    00000000  61 62 63 64                                       |abcd|
    00000004

If it's a binary file, there's a decent chance the NULL's are
significant. Now, consider the following hypothetical example where we
want to remove the NULL's:
    
    [ /tmp ]
    oskari@dj3ntoo λ printf "ab\0cd" | sed -e 's/\x00//' | hd
    00000000  61 62 63 64                                       |abcd|
    00000004

Testing for (in)equality between pre- and post-sed contents is
reasonable enough in most cases. This time, though, it would fail to
detect anything has changed since the pre-sed contents have their NULL's
unintentionally stripped, whereas the post-sed contents have them
intentionally stripped.

While I personally don't think that running sed on binary files is a
good idea in the first place, it's still relevant since the end result
would be an incorrect answer to the question of "Did sed actually do
anything?".

On the other hand, saving a set of pre- and post-sed hashes like Ulrich
suggested would give the expected result.

- Oskari

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [gentoo-dev] [PATCH v2 1/1] esed.eclass: new eclass

[Ionen Wolkens]

[-- Attachment #1: Type: text/plain, Size: 1790 bytes --]

On Fri, Jun 03, 2022 at 07:15:17PM -0500, Oskari Pirhonen wrote:
[snip[
> Testing for (in)equality between pre- and post-sed contents is
> reasonable enough in most cases. This time, though, it would fail to
> detect anything has changed since the pre-sed contents have their NULL's
> unintentionally stripped, whereas the post-sed contents have them
> intentionally stripped.
>
> While I personally don't think that running sed on binary files is a
> good idea in the first place, it's still relevant since the end result
> would be an incorrect answer to the question of "Did sed actually do
> anything?".

Yeah one of the primary motivation to silence this was elsewhere, I
don't think silencing matters for esed and if binary files are being
modified may as well use sed(1) directly instead (this is something
that'd need to be more intensively verified on bumps than with esed
anyway).

Use is also very uncommon, although sed is still "handy" when changing
just 1 instruction and want to avoid an extra dependency on a proper
binary patching method.
 
> On the other hand, saving a set of pre- and post-sed hashes like Ulrich
> suggested would give the expected result.

If really wanted to solve this yes, although it may make sense to say
this eclass is not for binary files. The talk to add bash-only "erepl"
makes it rather difficult to preserve nulls (mapfile silently strip \0
without even a warning). mapfile -d '' could allow to restore them
but ideally want to iterate on lines to do per-line pattern matches.
Is it possible to hack away something to preserve? Probably.. but it's
going to make this messy and I'm not sure it's worth it.

erepl is also worse because they'll be lost in output and not just
during comparison.

-- 
ionen

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [gentoo-dev] [PATCH v2 0/1] esed

[Ionen Wolkens]

[-- Attachment #1: Type: text/plain, Size: 1328 bytes --]

On Fri, Jun 03, 2022 at 07:36:45AM -0400, Ionen Wolkens wrote:
> Nothing fundamentally different until further feedback,
> mostly to get eval out of the way.
> 
> Changelog
> v2:
>  - replace use of eval (realized another way works)
>  - add missing quotes around one _esed_output
>  - move diff checks inside comparison blocks

For the record, more work planned so please wait for v3 for
further review (may be a bit), albeit any feedback / ideas welcome.

Right now looking at:
- some enewsed fixes or perhaps reimplementation (noticed a meh flaw)
- adding a pure bash non-sed replacer suggested by mgorny, e.g. erepl
  this would be useful to use for simple operations not crippled by sed
  delimiters, and generally preferable unless need fancy sed stuff
- perhaps cut support for some unlikely-anyone-will-need features,
  like at least `sed s/// file1 file2 > file3` concat, and perhaps
  requiring files to be always at end of command line or others
  things which may help simplify (without going full minimal, erepl
  will be for that).

> 
> Ionen Wolkens (1):
>   esed.eclass: new eclass
> 
>  eclass/esed.eclass | 201 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 201 insertions(+)
>  create mode 100644 eclass/esed.eclass
> 
> -- 
> 2.35.1
> 
> 

-- 
ionen

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [gentoo-dev] [PATCH 0/2] Add esed.eclass for sed that dies if caused no changes

[Alessandro Barbieri]

[-- Attachment #1: Type: text/plain, Size: 1842 bytes --]

Il giorno mar 31 mag 2022 alle ore 13:23 Ionen Wolkens <ionen@gentoo.org>
ha scritto:

> Often preferable to use patches so this happens, but sed have its
> uses/convenience and this intend to help reduce the amount of old
> broken seds causing issues that go unnoticed on bumps.
>
> Inspired by app-portage/iwdevtools' qa-sed (warns on any seds), but
> this is for more deterministic use in ebuilds.
>
> Also slightly shortens sed use, -i is default, and no need to || die.
> (see @EXAMPLE in eclass for a quick usage overview).
>
> Implementation / available wrappers / usefulness still up for debate,
> but without further comments I consider this ready (albeit first time
> touching / making an eclass, so I could be overlooking simple things).
> Also partly uses >=bash-4.4, so EAPI-7 is not considered.
>
> See github PR[1] for old changelog background.
>
> Up to maintainers but personally would encourage to slowly replace
> (almost) all use of sed with either this or patches. Some cases
> where it can be inconvenient like eclasses "guessing" that a package
> may or may not have something to replace, and that nothing happened
> is not an issue.
>
> [1] https://github.com/gentoo/gentoo/pull/25662
>
> Ionen Wolkens (2):
>   esed.eclass: new eclass
>   eclass/tests/esed.sh: basic tests for esed.eclass
>
>  eclass/esed.eclass   | 199 +++++++++++++++++++++++++++++++++++++++++++
>  eclass/tests/esed.sh | 173 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 372 insertions(+)
>  create mode 100644 eclass/esed.eclass
>  create mode 100755 eclass/tests/esed.sh
>
> --
> 2.35.1
>
>
>
I use patches for static content, not a big fan of wall of sed in ebuilds
When I use sed is for dynamic content and mostly like to do this:
s|lib|$(get_libdir)|g
In this case esed would be deleterious because it would fail on 32 bit
arches

[-- Attachment #2: Type: text/html, Size: 2388 bytes --]

Re: [gentoo-dev] [PATCH 0/2] Add esed.eclass for sed that dies if caused no changes

[Ionen Wolkens]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

On Sat, Jun 04, 2022 at 06:19:30PM +0200, Alessandro Barbieri wrote:
> When I use sed is for dynamic content and mostly like to do this:
> s|lib|$(get_libdir)|g
> In this case esed would be deleterious because it would fail on 32 bit
> arches

This case is noted in the docs fwiw. How to handle will depend on
preference but best route to ensure the change is always right
would be either to patch to replace lib by e.g. @GENTOO_LIBDIR@,
/then/ sed, or insert a variable/option that could be passed and
perhaps even upstreamed.

Lazier approach that use esed (or upcoming erepl) could be:

   [[ $(get_libdir) != lib ]] && erepl /lib /$(get_libdir) file

This also avoids unnecessarily editing files when there's nothing,
albeit more invasive and runs get_libdir twice. EPREFIX is a bit
nicer given we can use `use prefix`:

   use prefix && erepl /usr "${EPREFIX}"/usr file

This is a bit like replacing
   rm -f exists-with-USE-gui-only
by
   use !gui || rm exists-with-USE-gui-only || die
(bash exit code logic always fun wrt that double negation...)

Both are going to do the job at first, but one may end up missing
that the file was moved in another directory on bump and is now
getting wrongly installed.

-- 
ionen

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]