Make editor builders archive to gs://dart-archive/ in addition to the other GCS buckets

Make editor builders archive to gs://dart-archive/ in addition to the other GCS buckets

This is the first step towards release channels.

R=ricow@google.com

Committed: https://code.google.com/p/dart/source/detail?r=27550

[ricow1, 2013-09-16 11:51:30]

There is a few long lines as well

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py
File dart/editor/build/build.py (right):

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:49: gsutil.execute(['cp', '-a', 'public-read',
local_path, remote_path])
how about a copyPublic method in GSUtil class?

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:60: if CHANNEL == 'trunk': return
add a todo here saying why we do this

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:67: if basename.startswith('darteditor-linux'):
shouldn't we pass this info along instead of extracting it from the filename?
If the reason is that we want to make as little addition to existing code as
possible we should have a todo to remove this

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:83: DartArchiveFile(zipfile, editor_dir + '/' +
destination_zipfile,
how about a namer.editor_zipfilepath instead, that just returns the full gs://
path?

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:84: create_md5sum=True)
else:
  throw an error?

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:93: gsutil.execute(['rm', '-R', update_site_dir])
isn't this asking for a race condition?

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:105: path32 = sdk_dir + '/' +
namer.sdk_zipfilename(system, 'ia32', 'release')
the latest one will already be there right, will this not throw an error?

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:489: namer = bot_utils.GCSNamer(CHANNEL,
bot_utils.ReleaseType.RAW)
Extract this to a method as well to be consistent

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:926: namer = bot_utils.GCSNamer(CHANNEL,
bot_utils.ReleaseType.RAW)
also put this into a method

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/__init__.py
File dart/tools/bots/__init__.py (left):

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/__init__.p...
dart/tools/bots/__init__.py:6: 'sources': [
what is this and why is it here, and why are we leaving an empty file except the
copyright header

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils.py
File dart/tools/bots/bot_utils.py (right):

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils....
dart/tools/bots/bot_utils.py:51: class ReleaseType(object):
To be consistent, should we make a Mode class as well?
I know it will just be for asserting, but anyway

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils....
dart/tools/bots/bot_utils.py:93: def apidocs_zipfile(self):
we use zipfilename in other functions

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils....
dart/tools/bots/bot_utils.py:101: assert mode in ['release', 'debug']
if you create a Mode class, then use it here for the assert

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils....
dart/tools/bots/bot_utils.py:116: def dartium_variant_zipfilename(self, name,
system, arch, mode):
do we need the above functions if we have this?

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils....
dart/tools/bots/bot_utils.py:127: if p.returncode:
I would actually be specific here
if p.returncode != 0:
 ....

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils....
dart/tools/bots/bot_utils.py:135: GSUTIL_PATH = os.path.join(DART_DIR,
'third_party', 'gsutil', 'gsutil')
this will probably not work on dartium, since we don't use the normal DEPS file
there (i.e., we don't have the gsutil tool in this location)

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils....
dart/tools/bots/bot_utils.py:147: env['AWS_CREDENTIAL_FILE'] = boto_config
do we need to specify this at this point?
This will only be valid for the env used to execute this specific command right
(which is gsutil)

[kustermann, 2013-09-16 14:15:54]

PTAL

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py
File dart/editor/build/build.py (right):

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:49: gsutil.execute(['cp', '-a', 'public-read',
local_path, remote_path])
On 2013/09/16 11:51:30, ricow1 wrote:
> how about a copyPublic method in GSUtil class?

Added upload/remove with named parameters for public/recursive.

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:60: if CHANNEL == 'trunk': return
On 2013/09/16 11:51:30, ricow1 wrote:
> add a todo here saying why we do this

Done.

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:67: if basename.startswith('darteditor-linux'):
On 2013/09/16 11:51:30, ricow1 wrote:
> shouldn't we pass this info along instead of extracting it from the filename?
> If the reason is that we want to make as little addition to existing code as
> possible we should have a todo to remove this

I don't know what you mean by passing it along.

bot_utils expect to get an "system"/"arch" parameter -- and we extract these two
parts here.

I don't want to change the name of the zip file, since we want to keep doing
trunk builds/archives for a while (and I don't want to touch these pieces)

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:83: DartArchiveFile(zipfile, editor_dir + '/' +
destination_zipfile,
On 2013/09/16 11:51:30, ricow1 wrote:
> how about a namer.editor_zipfilepath instead, that just returns the full gs://
> path?

Added filepath functions in addition to filename/directory functions.

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:84: create_md5sum=True)
On 2013/09/16 11:51:30, ricow1 wrote:
> else:
>   throw an error?

I added an assert (I thought we could call this function with 'tar.gz' as well
-- now I think we don't).

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:93: gsutil.execute(['rm', '-R', update_site_dir])
On 2013/09/16 11:51:30, ricow1 wrote:
> isn't this asking for a race condition?

Not AFAIK. Please explain.

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:105: path32 = sdk_dir + '/' +
namer.sdk_zipfilename(system, 'ia32', 'release')
On 2013/09/16 11:51:30, ricow1 wrote:
> the latest one will already be there right, will this not throw an error?

Not AFAIK. Please explain.

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:489: namer = bot_utils.GCSNamer(CHANNEL,
bot_utils.ReleaseType.RAW)
On 2013/09/16 11:51:30, ricow1 wrote:
> Extract this to a method as well to be consistent

Extracted this and another one. Refactored bot_utils a bit.

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:926: namer = bot_utils.GCSNamer(CHANNEL,
bot_utils.ReleaseType.RAW)
On 2013/09/16 11:51:30, ricow1 wrote:
> also put this into a method

Done.

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/__init__.py
File dart/tools/bots/__init__.py (left):

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/__init__.p...
dart/tools/bots/__init__.py:6: 'sources': [
On 2013/09/16 11:51:30, ricow1 wrote:
> what is this and why is it here, and why are we leaving an empty file except
the
> copyright header

This red area is an artifact of retvield.

The reason to add __init__.py to dart/tools/bots enables us to 
$ cd tools
$ python
| import utils
| from bots.bot_utils import GCSNamer
| namer = GCSNamer(..)

In other words we make that directory a python package.

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils.py
File dart/tools/bots/bot_utils.py (right):

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils....
dart/tools/bots/bot_utils.py:51: class ReleaseType(object):
On 2013/09/16 11:51:30, ricow1 wrote:
> To be consistent, should we make a Mode class as well?
> I know it will just be for asserting, but anyway

Done.

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils....
dart/tools/bots/bot_utils.py:93: def apidocs_zipfile(self):
On 2013/09/16 11:51:30, ricow1 wrote:
> we use zipfilename in other functions

Done.

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils....
dart/tools/bots/bot_utils.py:101: assert mode in ['release', 'debug']
On 2013/09/16 11:51:30, ricow1 wrote:
> if you create a Mode class, then use it here for the assert

Done.

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils....
dart/tools/bots/bot_utils.py:116: def dartium_variant_zipfilename(self, name,
system, arch, mode):
On 2013/09/16 11:51:30, ricow1 wrote:
> do we need the above functions if we have this?

Probably not. I started with this function to be private, but I figured it would
be beneficial to expose it.

I'll remove the old specific functions.

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils....
dart/tools/bots/bot_utils.py:127: if p.returncode:
On 2013/09/16 11:51:30, ricow1 wrote:
> I would actually be specific here
> if p.returncode != 0:
>  ....

I changed it -- but that's not the "pythonic way"!

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils....
dart/tools/bots/bot_utils.py:135: GSUTIL_PATH = os.path.join(DART_DIR,
'third_party', 'gsutil', 'gsutil')
On 2013/09/16 11:51:30, ricow1 wrote:
> this will probably not work on dartium, since we don't use the normal DEPS
file
> there (i.e., we don't have the gsutil tool in this location)

Good point. I've added more logic now to search for gsutil inside all PATH
directories in addition to the dart-gsutil and buildbot gsutil.

https://codereview.chromium.org/24175002/diff/3001/dart/tools/bots/bot_utils....
dart/tools/bots/bot_utils.py:147: env['AWS_CREDENTIAL_FILE'] = boto_config
On 2013/09/16 11:51:30, ricow1 wrote:
> do we need to specify this at this point?
> This will only be valid for the env used to execute this specific command
right
> (which is gsutil)

We probably don't need it. But BB contains it in it's default environment and
there is no reason not to include it here (maybe a future version of gsutil will
use it ....).

[ricow1, 2013-09-16 15:11:27]

LGTM

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py
File dart/editor/build/build.py (right):

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:93: gsutil.execute(['rm', '-R', update_site_dir])
On 2013/09/16 14:15:54, kustermann wrote:
> On 2013/09/16 11:51:30, ricow1 wrote:
> > isn't this asking for a race condition?
> 
> Not AFAIK. Please explain.

If people are using the web interface to browse the docs?

https://codereview.chromium.org/24175002/diff/3001/dart/editor/build/build.py...
dart/editor/build/build.py:105: path32 = sdk_dir + '/' +
namer.sdk_zipfilename(system, 'ia32', 'release')
On 2013/09/16 14:15:54, kustermann wrote:
> On 2013/09/16 11:51:30, ricow1 wrote:
> > the latest one will already be there right, will this not throw an error?
> 
> Not AFAIK. Please explain.

latest will already be there

[kustermann, 2013-09-17 07:52:27]

Committed patchset #3 manually as r27550 (presubmit successful).