Report apt-get failures in install-build-deps.sh

[Evan Martin, 2009-08-05 04:26:21]

Dan, I guess this script is your domain, right?

[dank, 2009-08-05 04:31:51]

Yeah.  This tries to fix a long-standing problem,
I applaud the author.  Two notes:
1) I'd rather just abort with a helpful error message
than ask another question.  (I'm already annoyed
with the questions this script asks.)
2) If you can use case foo) rather than echo | grep foo, please do;
it's the sh way.

[gmatht, 2009-08-05 15:43:24]

This now uses elif in place of "echo | grep foo", and avoids use of yes_no.

However, I was thinking that maybe we could replace occurrences of "yes_no 1"
with "yes_no 1 opt" such that yes_no will return true if --opt was set on the
command line, false if --no-opt was set, default if --non-interactive was set
and prompt the user otherwise.

[dank, 2009-08-05 17:03:31]

On 2009/08/05 15:43:24, gmatht wrote:
> This now uses elif in place of "echo | grep foo", and avoids use of yes_no.

Looks good to me.

> However, I was thinking that maybe we could replace occurrences of "yes_no 1"
> with "yes_no 1 opt" such that yes_no will return true if --opt was set on the
> command line, false if --no-opt was set, default if --non-interactive was set
> and prompt the user otherwise.

I'm more in favor of just getting rid of all prompts
entirely, myself, but I don't feel strongly enough
to do it.  Scripts like this should not be interactive,
and the way yes_no behaves screws me up every time.

[Evan Martin, 2009-08-06 20:03:07]

Did you land this?  Should I?

[Lei Zhang, 2009-08-07 20:06:38]

http://codereview.chromium.org/160630/diff/12/13
File build/install-build-deps.sh (right):

http://codereview.chromium.org/160630/diff/12/13#newcode177
Line 177: echo "No missing packages, and the packages are up-to-date."
I realized Dan K already committed this, and it's a bit broken.

Have you ever gotten the case where you hit this line? Even with all the
packages installed, apt-get already return 1 for me, probably because of
--reinstall.

http://codereview.chromium.org/160630/diff/12/13#newcode182
Line 182: new_list=$(echo $new_list |
echo $new_list needs to be: echo "$new_list". Otherwise the multi-line text in
$new_list gets collapsed into one line and the sed command doesn't do the right
operations.

[gmatht, 2009-08-08 03:47:38]

On 2009/08/07 20:06:38, Lei Zhang wrote:
> Have you ever gotten the case where you hit this line? Even with all the
> packages installed, apt-get already return 1 for me, probably because of
> --reinstall.

No. I that particular if was mainly to trap the error rather than because I
really cared if that line was hit. I have replaced the if with an || which may
be cleaner.

> Line 182: new_list=$(echo $new_list |
> echo $new_list needs to be: echo "$new_list". Otherwise the multi-line text in
> $new_list gets collapsed into one line and the sed command doesn't do the
right

Agreed. I have submitted another patch set.

[Evan Martin, 2009-11-05 01:44:42]

ping.  should we commit this?

[gmatht, 2009-11-05 04:24:27]

On 2009/11/05 01:44:42, Evan Martin wrote:
> ping.  should we commit this?

As I recall that the missing "'s in line 182 ended up being fixed by someone
else, probably in a similar way. If not, it would be a good idea to commit this.

I don't think the code clean up relating to if/|| was committed, but I  think
we'd need to split the patch up to submit that, so we don't get conflicts with
the already committed fix for the missing "'s