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 <eschwa...@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
