On Fri, 01 Sep 2017 18:53:12 -0400, Eli Schwartz wrote:
Fixes regression in 49088b0860276c664933c2b3e36a2fef714b7a07
Are you sure it was there? Looking at the code (but without actually testing it) (and being the one responsible for the bug), I think it was actually in 2fd5931a8c67289a8a4acd327b3ce99a5d64c8c7. Maybe `git blame` told you the erroneous line was introduced in 49088b0--it was actually just re-indented there; it originally appeared (un-indented) in 2fd5931.
$run_namcap will always be set to "" `if $not_a_var; then ...; fi` is always truthful when $not_a_var is unset or equal to "" and the `then` clause will always be run.
I'm not entirely sure why everything had to be moved into functions purely for the sake of being moved into functions, as it goes against the general theme of devtools/makepkg/etc. especially since it doesn't seem to have been thoroughly tested.
I'm the one to blame for that. In Parabola (a derivative of Arch), we have a couple of other programs that want to reuse code from makechrootpkg; putting everything into functions makes that possible; we just delete the call to `main "$@"` at the end and name it makechrootpkg.sh somewhere in /lib. However, we don't actually use the makechrootpkg program itself, we only use functions from it. Because of this, most of the functions except for `main` are tested by Parabola's fairly intense test suite for our dev tools, but main(), init_variables(), usage(), and load_vars() aren't. So I admit, Arch doesn't really benefit from 49088b0 (except for whatever arguments you want to make about code clarity); and that it's a change made for the benefit of Parabola. But hey, Jan merged it :-) But also, prepare_chroot() was a function before any code from me/Parabola was merged. The relevant change made by me was in 2fd5931 when I made it take run_namcap as an argument, instead of as a global variable. Which leads us in to...
I'm also not sure why global state variables need to be cloned locally for their sole explicit purpose.
Well, as I explained above, Parabola has another program that calls prepare_chroot(), and having to set some global variable (especially a lower_case one) is a weird calling convention. But also, I don't think that it's a bad idea to slowly get rid of global variables in programs (of course in Bash it's not quite a good distinction because it's dynamically scoped instead of lexically scoped, and even if it's a local variable in a function, in anything that you call it might as well be global). And this is a step toward that. Think of all the globals as just local variables to main()--other functions (except for init_variables(), which is really just "main() part 1") shouldn't be touching them.
But for now this patch implements the minimum necessary work to properly pass the "do I want namcap" variable into prepare_chroot() according to the current logic flow. Note that I have still not thorougly tested makechrootpkg.
I actually think that this mistake originated in Parabola's libretools commit 6eddc77 way back on 2013-09-11, when I made a mistake when resolving merge conflicts when merging devtools commits 7ca4eb82 (2013-05-02) and 4937422 (2013-05-03) into libretools. Jan and I had made many of the same changes, but in subtly different ways.
Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- makechrootpkg.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/makechrootpkg.in b/makechrootpkg.in index 664f9f6..b1c9545 100644 --- a/makechrootpkg.in +++ b/makechrootpkg.in @@ -403,7 +403,7 @@ main() {
download_sources "$copydir" "$makepkg_user"
- prepare_chroot "$copydir" "$USER_HOME" "$keepbuilddir" + prepare_chroot "$copydir" "$USER_HOME" "$keepbuilddir" "$run_namcap"
if arch-nspawn "$copydir" \ --bind="$PWD:/startdir" \ -- 2.14.1
Not that I'm in a position within Arch for it to carry too much weight, but: LGTM. -- Happy hacking, ~ Luke Shumaker