[arch-projects] [devtools] [PATCH 1/2] makechrootpkg: Fix unconditionally running namcap

Luke Shumaker lukeshu at lukeshu.com
Sat Sep 2 03:34:06 UTC 2017


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 at 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


More information about the arch-projects mailing list