Add 'quick-check' option to install-build-deps.sh

Add 'quick-check' option to install-build-deps.sh

This is a lightweight check to alert developers when their build
dependencies are out-of-date, and hopefully avoid confusing compile
errors. It takes < 1 second to run, so can be checked whenever sources
are synced, and might even by worth including as a default step in
gyp_chromium or DEPS hooks.

By definition, this is not a complete check, and in particular, will not
detect version updates of installed dependencies. It's mostly intended
to detect when entirely new dependencies have been added.

R=markus@chromium.org, sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231030

[sky, 2013-10-24 20:46:03]

Can you find someone that has reviewed or modified install-build-deps in the
past?

[Michael Moss, 2013-10-24 20:57:04]

On 2013/10/24 20:46:03, sky wrote:
> Can you find someone that has reviewed or modified install-build-deps in the
> past?

That's markus. Just thought you might also have an opinion since you mentioned
the problem with the new deps.

[sky, 2013-10-24 21:13:48]

I like it, but I'll let Markus review:)

-sky

[Markus (顧孟勤), 2013-10-25 00:40:49]

Generally, all of this looks good. Just a couple of nitpicks about shell style.

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps.sh
File build/install-build-deps.sh (right):

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:63: if [ 0 -eq "${do_unsupported-0}" ] && [ 0 -eq
"${do_quick_check-0}" ] ; then
I generally prefer "${...:-...}" over "${...-...}", but it probably doesn't
matter much.

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:83: chromeos_dev_list="libbluetooth-dev"
This sounds as if it should be in an unrelated CL. But maybe, I am missing
something non-obvious

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:108: chromeos_lib_list="libpulse0 libbz2-1.0"
Same here

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:211: if test "$do_inst_syms" = "" && test 0 -eq
${do_quick_check-0}
We should really clean up the code and either use "test" everywhere or use "[ ]"
everywhere. Oh well, probably not something that you want to do right now.

They are all sorts of other style issues with this script anyway.

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:240: test 0 -eq "${do_unsupported-0}"; then
You do realize that you can write "-o" and "-a", do you? Just checking. Either
option should be OK in general.

For some reason, I find the double-negation hard to read. Would it be more
readable to change the logic:

  if ! [ "${DISTRIB_CODENAME}" = "precise" -o 1 -eq "${do_unsupported-0}" ];
then ...

It's really your call. I am probably just too dense to read the existing code
without having the scratch my head.

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:252: tr " " "\n" | sort -u | tr "\n" " ")"
Minor nit-pick. You can avoid using "tr", if you use
${parameter//pattern/string}. This is generally a good idea, as it is faster
than spawning an external process. But it makes the code less readable. So, it's
probably a premature optimization.

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:256: ${packages} 2>&1 | grep -v "ok installed" ||
/bin/true)"
You should use ":" instead of "/bin/true". That's the more common pattern for
this type of thing, and it is more efficient than spawning an external process.

Think of ":" as being roughly equivalent to "pass" in Python.

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:257: if [ "$failed_check" ]; then
Shouldn't that read "[ -n "${failed_check}" ]". You don't want to confuse "[" by
whatever happens to be the contents of "${failed_check}". The heuristics that
"[" needs to apply when parsing arguments are crazy enough as is; let's try to
not make things more fragile.

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:259: grep "No packages found matching" | sed -e
"s/^.* \(.*\).$/\1/")"
You can probably combine "grep" and "sed" into a single command.

This is untested, but I think it should work:

sed -e '/No packages found matching/s/^.* \(.*\)[.]$/\1/;t;d'

For the "missing" case, it would then be:

sed -e '/No packages found matching/d;s/^\([^:]*\):.*$/\1/'

More importantly though, I can't trigger any dpkg-query that would match the
"missing" pattern. Can you give an example of what you would expect to see here,
and how you tested this? My machine is probably just not set up the way you
expect it (i.e. everything is managed by apt-get).

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:268: if apt-cache show ${p} > /dev/null 2>&1; then
This is a bash script, so you could write ">&/dev/null", if you prefer the more
concise form.

[Michael Moss, 2013-10-25 02:45:31]

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps.sh
File build/install-build-deps.sh (right):

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:63: if [ 0 -eq "${do_unsupported-0}" ] && [ 0 -eq
"${do_quick_check-0}" ] ; then
On 2013/10/25 00:40:50, Markus (顧孟勤) wrote:
> I generally prefer "${...:-...}" over "${...-...}", but it probably doesn't
> matter much.

Yeah, I was just staying consistent with other uses nearby, though the file as a
whole isn't terribly consistent about much :/

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:83: chromeos_dev_list="libbluetooth-dev"
On 2013/10/25 00:40:50, Markus (顧孟勤) wrote:
> This sounds as if it should be in an unrelated CL. But maybe, I am missing
> something non-obvious

I was just cleaning up dupes (that package is also in dev_list, so it's not
chromeos-only).

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:108: chromeos_lib_list="libpulse0 libbz2-1.0"
On 2013/10/25 00:40:50, Markus (顧孟勤) wrote:
> Same here

Another dev_list dupe.

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:211: if test "$do_inst_syms" = "" && test 0 -eq
${do_quick_check-0}
On 2013/10/25 00:40:50, Markus (顧孟勤) wrote:
> We should really clean up the code and either use "test" everywhere or use "[
]"
> everywhere. Oh well, probably not something that you want to do right now.
> 
> They are all sorts of other style issues with this script anyway.

Agreed, but probably better as a dedicated clean up CL.

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:240: test 0 -eq "${do_unsupported-0}"; then
On 2013/10/25 00:40:50, Markus (顧孟勤) wrote:
> You do realize that you can write "-o" and "-a", do you? Just checking. Either
> option should be OK in general.
> 
> For some reason, I find the double-negation hard to read. Would it be more
> readable to change the logic:
> 
>   if ! [ "${DISTRIB_CODENAME}" = "precise" -o 1 -eq "${do_unsupported-0}" ];
> then ...
> 
> It's really your call. I am probably just too dense to read the existing code
> without having the scratch my head.

Done.

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:252: tr " " "\n" | sort -u | tr "\n" " ")"
On 2013/10/25 00:40:50, Markus (顧孟勤) wrote:
> Minor nit-pick. You can avoid using "tr", if you use
> ${parameter//pattern/string}. This is generally a good idea, as it is faster
> than spawning an external process. But it makes the code less readable. So,
it's
> probably a premature optimization.

Yeah, since I still have to spawn for sort, I think keeping the 'tr's in the
pipeline is a bit more clear.

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:256: ${packages} 2>&1 | grep -v "ok installed" ||
/bin/true)"
On 2013/10/25 00:40:50, Markus (顧孟勤) wrote:
> You should use ":" instead of "/bin/true". That's the more common pattern for
> this type of thing, and it is more efficient than spawning an external
process.
> 
> Think of ":" as being roughly equivalent to "pass" in Python.

'pass' is exactly what I was looking for :)
Done.

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:257: if [ "$failed_check" ]; then
On 2013/10/25 00:40:50, Markus (顧孟勤) wrote:
> Shouldn't that read "[ -n "${failed_check}" ]". You don't want to confuse "["
by
> whatever happens to be the contents of "${failed_check}". The heuristics that
> "[" needs to apply when parsing arguments are crazy enough as is; let's try to
> not make things more fragile.

Done.

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:259: grep "No packages found matching" | sed -e
"s/^.* \(.*\).$/\1/")"
On 2013/10/25 00:40:50, Markus (顧孟勤) wrote:
> You can probably combine "grep" and "sed" into a single command.

Ugh, yeah. I actually had these in different areas initially, then combined them
but didn't rewrite it.

> This is untested, but I think it should work:
> 
> sed -e '/No packages found matching/s/^.* \(.*\)[.]$/\1/;t;d'
> 
> For the "missing" case, it would then be:
> 
> sed -e '/No packages found matching/d;s/^\([^:]*\):.*$/\1/'
> 
> More importantly though, I can't trigger any dpkg-query that would match the
> "missing" pattern. Can you give an example of what you would expect to see
here,
> and how you tested this? My machine is probably just not set up the way you
> expect it (i.e. everything is managed by apt-get).

If I do something like:

dpkg-query -W -f '${PackageSpec}:${Status}\n' libbrlapi-dev libc6-armel-cross

I get:

libbrlapi-dev:unknown ok not-installed
No packages found matching libc6-armel-cross.

'missing' matches the first and 'nomatch' matches the second.

https://codereview.chromium.org/40603004/diff/90001/build/install-build-deps....
build/install-build-deps.sh:268: if apt-cache show ${p} > /dev/null 2>&1; then
On 2013/10/25 00:40:50, Markus (顧孟勤) wrote:
> This is a bash script, so you could write ">&/dev/null", if you prefer the
more
> concise form.

2>&1 is used elsewhere, so I'll leave it consistent for now. That can be a clean
up CL change.

[Markus (顧孟勤), 2013-10-25 07:25:14]

lgtm

[Michael Moss, 2013-10-25 16:11:51]

Committed patchset #5 manually as r231030 (presubmit successful).