Add symbol uploader

Add symbol uploader

[petkov, 2010-06-21 18:01:55]

A few comments that you might want to address...

http://codereview.chromium.org/2812012/diff/4001/5002
File upload_symbols (right):

http://codereview.chromium.org/2812012/diff/4001/5002#newcode6
upload_symbols:6: # Script to build the set of binary packages needed by Chrome
OS. It will
Update doc.

http://codereview.chromium.org/2812012/diff/4001/5002#newcode27
upload_symbols:27: "Run without actually uploading."
Do we want to make this the default, maybe if running on an official build? Is
it easy for a developer to build an official build with some local changes
(e.g., for debugging) and maybe mess up the symbols on the servers?

Also, at the beginning of main warn that this is a dry-run only if this flag is
enabled.

http://codereview.chromium.org/2812012/diff/4001/5002#newcode68
upload_symbols:68: function upload_file {
It might make sense to split this routine into 2 or 3 separate parts (generally
-- prepare and upload).

http://codereview.chromium.org/2812012/diff/4001/5002#newcode86
upload_symbols:86: 2>"${ERR_FILE}"; then
space after >

http://codereview.chromium.org/2812012/diff/4001/5002#newcode114
upload_symbols:114: 2>"${ERR_FILE}"; then
space after >

http://codereview.chromium.org/2812012/diff/4001/5002#newcode139
upload_symbols:139: error "--board is required."
use "die" instead. And you could make it a one line:
[ -n ...] || die ...

http://codereview.chromium.org/2812012/diff/4001/5002#newcode150
upload_symbols:150: warn "This is an unofficial build, uploading to staging
server"
the other URL says "staging"?

http://codereview.chromium.org/2812012/diff/4001/5002#newcode158
upload_symbols:158: if [ "${FLAGS_ARGV}" == "" ]; then
Use -z instead?

http://codereview.chromium.org/2812012/diff/4001/5002#newcode159
upload_symbols:159: local debug_file=""
Do you need this declaration?

http://codereview.chromium.org/2812012/diff/4001/5002#newcode182
upload_symbols:182: error "Encountered problems during uploads"
Use die instead.

http://codereview.chromium.org/2812012/diff/4001/5002#newcode184
upload_symbols:184: fi
Do you want to print in green an all went OK message (e.g., through "info") if
no errors were encountered?

[kmixter1, 2010-06-21 21:34:55]

PTAL

http://codereview.chromium.org/2812012/diff/4001/5002
File upload_symbols (right):

http://codereview.chromium.org/2812012/diff/4001/5002#newcode6
upload_symbols:6: # Script to build the set of binary packages needed by Chrome
OS. It will
On 2010/06/21 18:01:55, petkov wrote:
> Update doc.

Done.

http://codereview.chromium.org/2812012/diff/4001/5002#newcode27
upload_symbols:27: "Run without actually uploading."
On 2010/06/21 18:01:55, petkov wrote:
> Do we want to make this the default, maybe if running on an official build? Is
> it easy for a developer to build an official build with some local changes
> (e.g., for debugging) and maybe mess up the symbols on the servers?

Not a bad idea, though for a different reason.  A developer can build their own
official build and upload symbols without having to worry about messing up other
official builds (unless there are fairly unlikely hash collisions). The issue is
more to make sure we avoid having people unnecessarily burn server resources by
uploading every build.  So I added a big prompt to notify them of that if they
signal to upload everything.

> Also, at the beginning of main warn that this is a dry-run only if this flag
is
> enabled.

Added a warning at the beginning and end.

http://codereview.chromium.org/2812012/diff/4001/5002#newcode68
upload_symbols:68: function upload_file {
On 2010/06/21 18:01:55, petkov wrote:
> It might make sense to split this routine into 2 or 3 separate parts
(generally
> -- prepare and upload).
> 

Done.

http://codereview.chromium.org/2812012/diff/4001/5002#newcode86
upload_symbols:86: 2>"${ERR_FILE}"; then
On 2010/06/21 18:01:55, petkov wrote:
> space after >

Done.

http://codereview.chromium.org/2812012/diff/4001/5002#newcode114
upload_symbols:114: 2>"${ERR_FILE}"; then
On 2010/06/21 18:01:55, petkov wrote:
> space after >

Done.

http://codereview.chromium.org/2812012/diff/4001/5002#newcode139
upload_symbols:139: error "--board is required."
On 2010/06/21 18:01:55, petkov wrote:
> use "die" instead. And you could make it a one line:
> [ -n ...] || die ...
> 

Done.

http://codereview.chromium.org/2812012/diff/4001/5002#newcode150
upload_symbols:150: warn "This is an unofficial build, uploading to staging
server"
On 2010/06/21 18:01:55, petkov wrote:
> the other URL says "staging"?
> 

Thanks - I should have verified the symbols went where I expected.  Fixed, and I
have now verified as well.

http://codereview.chromium.org/2812012/diff/4001/5002#newcode158
upload_symbols:158: if [ "${FLAGS_ARGV}" == "" ]; then
On 2010/06/21 18:01:55, petkov wrote:
> Use -z instead?

Done.

http://codereview.chromium.org/2812012/diff/4001/5002#newcode159
upload_symbols:159: local debug_file=""
On 2010/06/21 18:01:55, petkov wrote:
> Do you need this declaration?
> 

Just was trying to limit scope, but no it's not necessary.

http://codereview.chromium.org/2812012/diff/4001/5002#newcode182
upload_symbols:182: error "Encountered problems during uploads"
On 2010/06/21 18:01:55, petkov wrote:
> Use die instead.

Done.

http://codereview.chromium.org/2812012/diff/4001/5002#newcode184
upload_symbols:184: fi
On 2010/06/21 18:01:55, petkov wrote:
> Do you want to print in green an all went OK message (e.g., through "info") if
> no errors were encountered?

Done.

[petkov, 2010-06-21 22:05:36]

A few more nits, typos, etc. One bu with the info/fi order.

LGTM otherwise.

http://codereview.chromium.org/2812012/diff/10001/11002
File upload_symbols (right):

http://codereview.chromium.org/2812012/diff/10001/11002#newcode6
upload_symbols:6: # Script to run upload all debug symbols required for crash
reporting
"run upload"

http://codereview.chromium.org/2812012/diff/10001/11002#newcode26
upload_symbols:26: DEFINE_boolean yes ${FLAGS_FALSE} "Answer yes to all
prompts."
Keep flags sorted?

http://codereview.chromium.org/2812012/diff/10001/11002#newcode27
upload_symbols:27: DEFINE_boolean verbose ${FLAGS_FALSE} "Be verbose"
Period at the end for consistency.

http://codereview.chromium.org/2812012/diff/10001/11002#newcode72
upload_symbols:72: echo "develoer debugging purposes, consider instead passing
specific files "
typo: developer

http://codereview.chromium.org/2812012/diff/10001/11002#newcode77
upload_symbols:77: then
Move then up on the same line as if

http://codereview.chromium.org/2812012/diff/10001/11002#newcode194
upload_symbols:194: info "Uploading symbols to ${upload_url} from ${SYSROOT}."
This "info" should be after "fi" ? Or indentation is wrong?

http://codereview.chromium.org/2812012/diff/10001/11002#newcode207
upload_symbols:207: if ! really_upload; then
I get easily lost in shell scripts and prefer short hands:

really_upload || exit 1

This could be done in a few other places (basically, all ifs with a single-line
body).

But it's up to you.

[kmixter1, 2010-06-21 23:11:56]

Pushing...

http://codereview.chromium.org/2812012/diff/10001/11002
File upload_symbols (right):

http://codereview.chromium.org/2812012/diff/10001/11002#newcode6
upload_symbols:6: # Script to run upload all debug symbols required for crash
reporting
On 2010/06/21 22:05:36, petkov wrote:
> "run upload"

Done.

http://codereview.chromium.org/2812012/diff/10001/11002#newcode26
upload_symbols:26: DEFINE_boolean yes ${FLAGS_FALSE} "Answer yes to all
prompts."
On 2010/06/21 22:05:36, petkov wrote:
> Keep flags sorted?
> 

Done.

http://codereview.chromium.org/2812012/diff/10001/11002#newcode27
upload_symbols:27: DEFINE_boolean verbose ${FLAGS_FALSE} "Be verbose"
On 2010/06/21 22:05:36, petkov wrote:
> Period at the end for consistency.
> 

Done.

http://codereview.chromium.org/2812012/diff/10001/11002#newcode72
upload_symbols:72: echo "develoer debugging purposes, consider instead passing
specific files "
On 2010/06/21 22:05:36, petkov wrote:
> typo: developer

Done.

http://codereview.chromium.org/2812012/diff/10001/11002#newcode77
upload_symbols:77: then
On 2010/06/21 22:05:36, petkov wrote:
> Move then up on the same line as if
> 

Done.

http://codereview.chromium.org/2812012/diff/10001/11002#newcode194
upload_symbols:194: info "Uploading symbols to ${upload_url} from ${SYSROOT}."
On 2010/06/21 22:05:36, petkov wrote:
> This "info" should be after "fi" ? Or indentation is wrong?
> 

Done.

http://codereview.chromium.org/2812012/diff/10001/11002#newcode207
upload_symbols:207: if ! really_upload; then
On 2010/06/21 22:05:36, petkov wrote:
> I get easily lost in shell scripts and prefer short hands:
> 
> really_upload || exit 1
> 
> This could be done in a few other places (basically, all ifs with a
single-line
> body).
> 
> But it's up to you.
> 

I did a few though honestly I find the full if more easily readable.  I'll
switch it in a few cases though where I feel like the silly overhead of writing
a shell if block outweighs the extra readability.