stagefiles: fix completely wrong require-stages logic

now having investigated my suspicions of the functionality and use of
Require_stagefile(), i conclude that it has been fundamentally broken
all the way back to v1.0~a8-1 (or at least usage of it since v1.0.1-2).

gah. (╯°□°)╯︵ ┻━┻

----

very early on in the history of live-build this function took the name of
a _single_ stage file only and did `exit 1` should the file not be found.
this was simple and clearly accomplished "what was on the tin", so to
speak.

in bd1a5ddc8203907eb40135303bea5488397ec5d0 (2007, 1.0~a8-1) things got
weird. it was modified to support a list of multiple files. but instead of
being written to cause failure if _any_ of the listed files were missing
as perhaps one might expect, it was instead written to fail only if all
files were missing!

if you jump to the conclusion that i'm talking about a simple flipped
logic from a lack or otherwise of a `!` here, you'd be mistaken; there is
a comment inside the function that could not be more clear about what was
intended by the author - "Find at least one of the required stages"! this
makes me thoroughly confused about what they were thinking.

as we'll get to, this was fundamentally flawed (or at least its later use
was), but furthermore there were other notable issues introduced at this
point (but don't worry too much about these, they've all been addressed):
 - `NAME` was modified in the loop, using the existing value, but nothing
   initially set it...
 - the setting of `NAME` seems related to its use in the subsequent error
   output, yet they are logically separated; it is only set if a file
   exists, while the error is only printed if none exist.
 - it is pointlessly using a messy `CONTINUE="true"` based mechanism,
   when it could just `return 0`.
 - it did not handle correctly the bad use case of no params having been
   supplied.

it doesn't seem to have been entirely thought through, despite its
pervasive use throughout the build system.

note that no change was made in that commit to make actual use of the
new multi-param support. it would not be used until about a year later.

the function has remained largely untouched since then. in
c68c0a270832ca340429878ce6a0ab606d435b06 a notable change was made to add
an initial setting of `NAME`, which partially addressed one of the above
issues. but it did not really address the issue the change was meant to
solve, since the `NAME` as printed in the error was now the name of the
script when what was really wanted was the name of the stagefile. this was
finally fixed properly in d54990695f334d205fa846c42b6e0f2afd3c47f5.
however the weirdly pointless setting of `NAME` persisted in the loop.

finally i personally just refactored the function in the commit prior to
this one, retaining the same functionality but addressing the remaining
of the above minor implementation issues.

looking at usage of the new functionality introduced in
bd1a5ddc8203907eb40135303bea5488397ec5d0, it does not seem to have been
until 0cbbde2b9664b9fafb311f1048db25ea69952222 (2008, almost a year after
it was made possible) that changes were made to finally start making use
of the ability to pass more than one filename at a time to the function,
and it would appear that perhaps the author forgot what it actually was
that the function accomplished when used with multiple params, and failed
to double check.

in this first use of multiple parameters, this commit went from passing
single file names to individual calls to the function to passing the files
in one single call, in a commit the purpose of which was described as
simply tidying things up. it was most certainly not intended to change
stage requirements.

unfortunately, a change in requirements did occur as a result since the
new usage of the function was not accomplishing the same as before. this
change completely broke the stage requirements protection mechanism such
that only a single one of the listed stages needed to have completed for
the check to pass, rather than all as expected.

this flaw made it into release v1.0.1-2 and it has existed every since.

in the very next commit from that one,
6204dc0e6db02859a07a978d87f1a5231c0214cf things got even worse. here we
see the config stage being specified commonly as the first stage listed,
which is still the case today. this means that ever since this commit,
if you've already got a config before building (which you inevitably do,
especially after some later commits introduced automatically creating it
if missing), then all other stage requirements are simply ignored.

so it seems pretty damn clear that this function is accomplishing
completely the wrong objective. it _should_ be checking that _all_ files
given to it exist, not just one or more. ¯\_(ツ)_/¯

this FINALLY addresses this mistake.

(not that i wish to berate the author; i've made silly mistakes of my own
before)

1 files changed, 9 insertions, 4 deletions

diff --git a/functions/stagefile.sh b/functions/stagefile.sh
index 383df2c45..2fd757971 100755
--- a/functions/stagefile.sh
+++ b/functions/stagefile.sh
@@ -63,19 +63,24 @@ Remove_stagefile ()
 	rm -f "${FILE}"
 }
 
-# Find at least one of the required stages, `exit 1` if none found
+# Ensure that all specified stagefiles exist (and thus that all associated stages are complete)
 Require_stagefile ()
 {
 	if [ $# -eq 0 ]; then
 		Echo_warning "Bad `Require_stagefile` usage, no params were supplied"
 		return 0
 	fi
+
 	local FILE
+	local MISSING=false
 	for FILE in ${@}; do
-		if [ -f ".build/${FILE}" ]; then
-			return 0
+		if [ ! -f ".build/${FILE}" ]; then
+			MISSING=true
 		fi
 	done
+	if ! $MISSING; then
+		return 0
+	fi
 
 	local NAME
 	NAME="$(basename ${0})"
@@ -83,7 +88,7 @@ Require_stagefile ()
 	if [ $# -eq 1 ]; then
 		Echo_error "%s requires stage: %s" "${NAME}" "${FILE}"
 	else
-		Echo_error "%s requires one of these stages: %s" "${NAME}" "$(echo ${@})"
+		Echo_error "%s requires the following stages: %s" "${NAME}" "$(echo ${@})"
 	fi
 
 	exit 1