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. inbd1a5ddc82
(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. inc68c0a2708
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 ind54990695f
. 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 inbd1a5ddc82
, it does not seem to have been until0cbbde2b96
(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,6204dc0e6d
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)
This commit is contained in:
parent
1b09b15277
commit
ea0f6b7810
|
@ -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
|
||||
|
|
Loading…
Reference in New Issue