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

[Luke Shumaker]

On Sun, 03 Sep 2017 03:12:19 -0400,
Eli Schwartz wrote:
> > 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.
> 
> "it's not quite a good distinction" indeed.
> 
> And init_variables() is most emphatically *not* main() part 1, whether
> it is meant to be or not in practice it doesn't even attempt to be.

From the commit that put the code in init_variables() and main() into
functions instead of floating around in the top-level:

    It [may] make sense to move init_variables() down into the main()
    function, instead of having it as a separate function up top (if this
    done, then the `-g` flag passed to `declare` in init_variables() can
    be dropped).  However, in interest of keeping the `diff -w` small, and
    merges/rebases simpler, this isn't done here.

If you cut/paste the code from init_variables() into the top of
main(), it works fine.  The sole reason that it isn't part of main()
is that if Jan didn't apply that patch, then it meant that all of my
work on commits after that would be harder to merge/rebase to new
versions of devtools.

> Global variables aren't automatically a sin. Imperative programming
> isn't automatically a sin either. When used properly it can be both very
> nice-looking and descriptive of what is going on.

Oh, definitely.  But many of the functions that were already there
before I made any changes weren't really functions, they were just
labels serving as comments.  That is understandable when you look at
the history and see them as incrementally improving the legibility of
the codebase from relative mess that it was circa early 2013.  I was
just taking it a step farther; I made it so that if a function was
touching global state, then it was explicit.

> Figuring out where the state is at any given time can only ever be
> harder when scoping it (complexity is hard...), but usually that is a
> negligible cost offset by the reusability of functions that are going to
> be reused a lot. Which brings us back to "this is only useful as a lib".

Splitting things in to functions (real functions, that stand alone
from the calling code) increases readability because when reading that
function, you see the signature, and know the inputs, the outputs, and
you don't have to know everything that's going on in the entire file,
you can read that one function and understand the function.  When it
starts dealing with global state, well then you start to lose that, as
you have to start worrying about the entire codebase again.

> Also the existence of this bug testifies that you weren't able to follow
> what it was doing. :p

Oh, I followed what it was doing.  The code in libremakepkg calls
prepare_chroot() correctly :P

It was just a simple mistake of missing an argument when I called it.
As I said, I hadn't generally been looking at main() as closely as I
do the other functions (as on Parabola it never gets called), so it
slipped through the cracks.

> And if it were so advantageous to stick everything inside a main()
> function in bash scripts, maybe the rest of devtools should be converted
> as well. Thing is, that doesn't make a whole lot of sense in either
> makechrootpkg or any other tool if you aren't actually doing much other
> than sticking the entire contents of a script into a single function and
> then calling the function.
> 
> I kind of feel that makechrootpkg had struck a pretty good balance
> already.


>          I mean, I guess an argument could be made that on a strictly
> "make this more reusable for parabola" level prepare_chroot() should be
> able to scope run_namcap, which I guess was neatly accomplished (and
> broken) in the commit you mentioned... but tacking on a bunch of
> questionable changes and claiming that it was done for code readability
> purposes is IMHO fallacious.

Who claimed it was done for readability?  The commit message said

    makechrootpkg: Have functions be more function-y.

    Rather than them simply being named blocks of code with braces around
    them.

    That is: have them take things via arguments rather than global
    variables.

I made it known that I was backporting changes made in Parabola, but I
never gave any justification for this change more than what is said in
the commit message.

There's an argument to be made that making them more function-y
improves readability (it's explicit what affects that bit of code),
though I didn't make that argument.

Of course, there was the original motivating argument that them being
more function-y makes them easier to call from places other than (what
I indented to become) main().  Though I did not discuss that
motivation when contributing it upstream.

> Also main() functions in bash scripts are stupid.

Eh, it's similar to the idiom of wrapping everything in { ... },
except that it isn't confusing to people who haven't seen that idiom
before.  For reference, this serves the purpose of making it so that
Bash doesn't begin executing anything until the entire script has been
parsed.  This is important for scripts that aren't real files (maybe
piped from curl, a terrible idea, but a thing people do), but regular
programs benefit because if the file is edited (via an upgrade) while
the program is running, then it probably hasn't had the entire thing
read yet, and that can lead to real weirdness.

>                                                   As are functions whose
> only purpose is to set a bunch of global (!!!) variables immediately
> after the function definition.

Eh, I'd have moved it to be at the top of main(), but I was trying to
avoid moving code around in the file.

-- 
Happy hacking,
~ Luke Shumaker