[pacman-dev] [PATCH 1/3] makepkg: restrict usage of errexit to user functions

[Allan McRae]

On 16/02/12 02:26, Dave Reisner wrote:
> On Wed, Feb 15, 2012 at 7:05 AM, Allan McRae <allan at archlinux.org> wrote:
> 
>> On 15/02/12 04:57, Dave Reisner wrote:
>>> It's expected that this will lead to unwanted behavior, and needs
>>> widespread testing. It's desirable to commit this for a few reasons:
>>>
>>> - there's no reason we can't do our own error checking for code that we
>>>   write.
>>> - it avoids the need for ||true hacks scattered about in the code.
>>> - it makes us immune to upstream changes in exit codes (FS#28248)
>>>
>>> Signed-off-by: Dave Reisner <dreisner at archlinux.org>
>>> ---
>>> Allan, just making sure we're on the same page -- this _will_ cause
>> breakage,
>>> and the next patch in this series addresses one specific case. I figure
>> getting
>>> this patch in now gives us "ample time" to uncover and fix all these
>> before the
>>> next release.
>>
>>
>> OK...  I like this idea somewhat as I have seen the issues this can
>> cause.  But I have also seen it validly stop the scripts execution due
>> to errors that would have been non-obvious.
>>
>>
>> Here goes a few concerns you might help me alleviate:
>>
>> 1) does it really make us immune to upstream changes in exit codes?  In
>> the particular example of mercurial, would we not be checking the exit
>> code of the "hg pull" part anyway?
>>
> 
> Yeah, this is probably accurate. I feel like there's always going to be
> commands which can exit non-zero for which we still expect that they did
> what we told them to, I just picked a bad example.
> 
> 
>> 2) how much extra error checking are we going to need to do? e.g. if I
>> look at create_srcpackage() would we only need to check the mkdir and ln
>> lines?
> 
> 
> So, the big stuff is: file/directory creation, directory changes, and
> executing arbitrary code (e.g. sourcing files). There's no reason we
> couldn't create a "lighter weight" ERR trap with errtrace if we expect to
> go through an entire function that needs to succeed, rather than checking
> everything. create_srcpackage() looks like a good candidate for this sort
> of thing. We can do something like this:
> 
> somefunc() { true; false; true; echo 'shouldnt be here' }
> set -E
> trap -- 'return 1 2>/dev/null' ERR
> somefunc
> set +E
> trap -- ERR
> echo 'success'
> 
> The stderr redirect is due to what I suspect is a bug in bash (exists in
> bash3 as well) -- it errors out saying you can only return from a function,
> but it gives us the behavior we want (evidenced by the debug echo's). I'm
> going to followup with bug-bash at gnu to see if this is really the case,
> though.
> 

You are not going to convince anyone with that code!  :D

Would it be useful to have wrapper functions around cd, ln, mkdir that
would take most of the additional testing burden away?

Anyway, I am happy for this change to go ahead.  But I would hold off
committing it master until a few more of the obvious checks that will
now be needed have been added.

Allan