Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1193813004:
cipd recipe_module (Closed)
|
Created:
5 years, 6 months ago by seanmccullough Modified:
5 years, 5 months ago Reviewers:
Vadim Sh. CC:
chromium-reviews, kjellander-cc_chromium.org, stip+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/build.git@master Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
Descriptioncipd recipe_module
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=295890
Patch Set 1 #Patch Set 2 : still broken, uploading only to solicit help debugging #Patch Set 3 : removed junk changes #Patch Set 4 : added ensure_installed #
Total comments: 22
Patch Set 5 : removed ensure.py, unused gce-specific code #
Total comments: 13
Patch Set 6 : removed config, re-ordered bootstrap steps #Patch Set 7 : added cipd/example.expected #
Total comments: 15
Patch Set 8 : remove unused imports, pass bin_path in as a data param #
Total comments: 4
Patch Set 9 : more bootstrap simplification #Patch Set 10 : coverage -> 100% #
Messages
Total messages: 25 (5 generated)
seanmccullough@chromium.org changed reviewers: + vadimsh@chromium.org
This surely has some unnecessary stuff included, I wasn't sure what made sense to keep from ccompute's cipd lib. Should most of this be pulled out into a shared lib so the recipe and ccompute use the same code? example recipe usage (different repo): https://chromereviews.googleplex.com/215087013
Recipe modules aren't supposed to do all heavy lifting themselves, instead they just orchestrate calls to external scripts. In this particular case bootstrap should be an external script invoked from a recipe module. For example see how zip recipe_module is organized: https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re... The rational behind this: it is possible to run recipes in a "fake" mode where all they do is to record what they would have called into a file, without actually calling it. It allows to test high level logic independently of low level implementation details. Recipes are in fact can be seen as a standalone flavor of python. Usage of many standard modules is discouraged. Even os.path usage is forbidden: recipe running on Linux must be able to simulate what's happening on Windows.
Yep, what Vadim said. Lots of calls in here are dangerous (or just plain broken)
for recipes right now (os.path, urllib, print, open, socket, logging,
subprocess, etc.).
I'm guessing you'll probably want a config which looks roughly like:
{
'cipd_server': <defaults to prod url>,
'packages': {
<name>: {'version': <version/tag>},
}
}
then the api might look like:
api.cipd.c.packages['foobar'].version = "deadbeef..."
api.cipd.c.packages['nerdwhat'].version = "badc0ffee..."
api.cipd.ensure_installed(root=api.path...) # ensures client is installed and
all packages match the configured versions.
You'll also may want some helper magic to assist in computing some of the
'conventional' package names (using api.platform), so maybe something like:
pt = api.cipd.platform_tag(style=api.cipd.NATIVE) # returns 'linux-amd64',
other styles might be PYTHON, etc.
api.cipd.c.packages['infra/monitoring/dispatcher/'+pt].version = "deadbeef....."
On 2015/06/23 02:31:12, iannucci wrote:
> Yep, what Vadim said. Lots of calls in here are dangerous (or just plain
broken)
> for recipes right now (os.path, urllib, print, open, socket, logging,
> subprocess, etc.).
>
> I'm guessing you'll probably want a config which looks roughly like:
>
> {
> 'cipd_server': <defaults to prod url>,
> 'packages': {
> <name>: {'version': <version/tag>},
> }
> }
>
> then the api might look like:
>
> api.cipd.c.packages['foobar'].version = "deadbeef..."
> api.cipd.c.packages['nerdwhat'].version = "badc0ffee..."
> api.cipd.ensure_installed(root=api.path...) # ensures client is installed and
> all packages match the configured versions.
>
> You'll also may want some helper magic to assist in computing some of the
> 'conventional' package names (using api.platform), so maybe something like:
>
> pt = api.cipd.platform_tag(style=api.cipd.NATIVE) # returns 'linux-amd64',
> other styles might be PYTHON, etc.
> api.cipd.c.packages['infra/monitoring/dispatcher/'+pt].version =
"deadbeef....."
Added a config.py with a BaseConfig declaring a packages dict in a ConfigGroup.
BUT: calling api.cipd.c.packages["foo"].version = 'bar' from a recipe (see
other CL for usage)
results in this error: "AttributeError: 'NoneType' object has no attribute
'packages'"
According to print/sys.exit debugging statements I added to BaseConfig(), it
looks like BaseConfig never even gets called.
What else do I need to do in order for api.cipd.c to be initialized to
something?
On 2015/06/24 17:05:28, seanmccullough wrote:
> On 2015/06/23 02:31:12, iannucci wrote:
> > Yep, what Vadim said. Lots of calls in here are dangerous (or just plain
> broken)
> > for recipes right now (os.path, urllib, print, open, socket, logging,
> > subprocess, etc.).
> >
> > I'm guessing you'll probably want a config which looks roughly like:
> >
> > {
> > 'cipd_server': <defaults to prod url>,
> > 'packages': {
> > <name>: {'version': <version/tag>},
> > }
> > }
> >
> > then the api might look like:
> >
> > api.cipd.c.packages['foobar'].version = "deadbeef..."
> > api.cipd.c.packages['nerdwhat'].version = "badc0ffee..."
> > api.cipd.ensure_installed(root=api.path...) # ensures client is installed
and
> > all packages match the configured versions.
> >
> > You'll also may want some helper magic to assist in computing some of the
> > 'conventional' package names (using api.platform), so maybe something like:
> >
> > pt = api.cipd.platform_tag(style=api.cipd.NATIVE) # returns 'linux-amd64',
> > other styles might be PYTHON, etc.
> > api.cipd.c.packages['infra/monitoring/dispatcher/'+pt].version =
> "deadbeef....."
>
> Added a config.py with a BaseConfig declaring a packages dict in a
ConfigGroup.
>
> BUT: calling api.cipd.c.packages["foo"].version = 'bar' from a recipe (see
> other CL for usage)
> results in this error: "AttributeError: 'NoneType' object has no attribute
> 'packages'"
>
> According to print/sys.exit debugging statements I added to BaseConfig(), it
> looks like BaseConfig never even gets called.
>
> What else do I need to do in order for api.cipd.c to be initialized to
> something?
I found this works if I call it in my recipe RunSteps before I try to set
api.cipd.c.packages['foo']:
api.cipd.c = api.cipd.make_config()
Is this the right way to initialize api.cipd.c?
On 2015/06/24 18:13:56, seanmccullough wrote:
> On 2015/06/24 17:05:28, seanmccullough wrote:
> > On 2015/06/23 02:31:12, iannucci wrote:
> > > Yep, what Vadim said. Lots of calls in here are dangerous (or just plain
> > broken)
> > > for recipes right now (os.path, urllib, print, open, socket, logging,
> > > subprocess, etc.).
> > >
> > > I'm guessing you'll probably want a config which looks roughly like:
> > >
> > > {
> > > 'cipd_server': <defaults to prod url>,
> > > 'packages': {
> > > <name>: {'version': <version/tag>},
> > > }
> > > }
> > >
> > > then the api might look like:
> > >
> > > api.cipd.c.packages['foobar'].version = "deadbeef..."
> > > api.cipd.c.packages['nerdwhat'].version = "badc0ffee..."
> > > api.cipd.ensure_installed(root=api.path...) # ensures client is installed
> and
> > > all packages match the configured versions.
> > >
> > > You'll also may want some helper magic to assist in computing some of the
> > > 'conventional' package names (using api.platform), so maybe something
like:
> > >
> > > pt = api.cipd.platform_tag(style=api.cipd.NATIVE) # returns
'linux-amd64',
> > > other styles might be PYTHON, etc.
> > > api.cipd.c.packages['infra/monitoring/dispatcher/'+pt].version =
> > "deadbeef....."
> >
> > Added a config.py with a BaseConfig declaring a packages dict in a
> ConfigGroup.
> >
> > BUT: calling api.cipd.c.packages["foo"].version = 'bar' from a recipe (see
> > other CL for usage)
> > results in this error: "AttributeError: 'NoneType' object has no attribute
> > 'packages'"
> >
> > According to print/sys.exit debugging statements I added to BaseConfig(), it
> > looks like BaseConfig never even gets called.
> >
> > What else do I need to do in order for api.cipd.c to be initialized to
> > something?
>
> I found this works if I call it in my recipe RunSteps before I try to set
> api.cipd.c.packages['foo']:
> api.cipd.c = api.cipd.make_config()
>
> Is this the right way to initialize api.cipd.c?
PTAL
Added an ensure_installed method, separated out the non-recipe-safe code out
into external files in resources/. (not sure ensure.py needs to be its own file
but there it is)
On 2015/06/25 02:06:32, seanmccullough wrote:
> On 2015/06/24 18:13:56, seanmccullough wrote:
> > On 2015/06/24 17:05:28, seanmccullough wrote:
> > > On 2015/06/23 02:31:12, iannucci wrote:
> > > > Yep, what Vadim said. Lots of calls in here are dangerous (or just plain
> > > broken)
> > > > for recipes right now (os.path, urllib, print, open, socket, logging,
> > > > subprocess, etc.).
> > > >
> > > > I'm guessing you'll probably want a config which looks roughly like:
> > > >
> > > > {
> > > > 'cipd_server': <defaults to prod url>,
> > > > 'packages': {
> > > > <name>: {'version': <version/tag>},
> > > > }
> > > > }
> > > >
> > > > then the api might look like:
> > > >
> > > > api.cipd.c.packages['foobar'].version = "deadbeef..."
> > > > api.cipd.c.packages['nerdwhat'].version = "badc0ffee..."
> > > > api.cipd.ensure_installed(root=api.path...) # ensures client is
installed
> > and
> > > > all packages match the configured versions.
> > > >
> > > > You'll also may want some helper magic to assist in computing some of
the
> > > > 'conventional' package names (using api.platform), so maybe something
> like:
> > > >
> > > > pt = api.cipd.platform_tag(style=api.cipd.NATIVE) # returns
> 'linux-amd64',
> > > > other styles might be PYTHON, etc.
> > > > api.cipd.c.packages['infra/monitoring/dispatcher/'+pt].version =
> > > "deadbeef....."
> > >
> > > Added a config.py with a BaseConfig declaring a packages dict in a
> > ConfigGroup.
> > >
> > > BUT: calling api.cipd.c.packages["foo"].version = 'bar' from a recipe
(see
> > > other CL for usage)
> > > results in this error: "AttributeError: 'NoneType' object has no attribute
> > > 'packages'"
> > >
> > > According to print/sys.exit debugging statements I added to BaseConfig(),
it
> > > looks like BaseConfig never even gets called.
> > >
> > > What else do I need to do in order for api.cipd.c to be initialized to
> > > something?
> >
> > I found this works if I call it in my recipe RunSteps before I try to set
> > api.cipd.c.packages['foo']:
> > api.cipd.c = api.cipd.make_config()
> >
> > Is this the right way to initialize api.cipd.c?
>
> PTAL
>
> Added an ensure_installed method, separated out the non-recipe-safe code out
> into external files in resources/. (not sure ensure.py needs to be its own
file
> but there it is)
Ping?
https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/cipd/api.py (right): https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:9: NATIVE = "native" nit: remove it unless it's used for real https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:15: version='9579504cec0336688292f5b0b68c3ed4e288e273'): I prefer versions to be expressed as a dict, e.g. class CIPDApi(...): CLIENT_VERSIONS = { 'infra/tools/cipd/linux-amd64': '9579....', 'infra/tools/cipd/mac-amd64': '....', .... } https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:16: self.bin_path = ("cipd_%s" % version) nit: () is unnecessary Also use self.m.path to construct an absolute path, e.g. self.m.path['slave_build'].join('cipd/cipd_%s' % version). https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:17: print ("should install cipd at %s" % self.bin_path) print is not allowed in recipes. https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:37: for pkg_name in self.c.packages: sorted(self.c.packages), for reproducibility https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:39: pkg_list.append(("%s %s" % (pkg_name, pkg_spec['version']))) () is not required https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:44: self.m.file.write("write_test", "pkg_list.txt", "\n".join(pkg_list)) this is unnecessary, use raw_io module instead (search code for raw_io.input for examples). https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:52: script=self.resource('ensure.py'), what is the purpose to ensure.py? Why not self.m.step( name="ensure_installed", [self.bin_path, "ensure", ....]) ? https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/cipd/config.py (right): https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/config.py:1: from recipe_engine.config import config_item_context I personally don't understand how this config stuff works and why it is needed in CIPD module, imho: api.cipd.ensure_installed(root, packages) is good enough. https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/cipd/resources/bootstrap.py (right): https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. this script is very GCE-specific. Please remove all GCE-specific code and code that is not actually involved in running install_cipd_client.
https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/cipd/api.py (right): https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:9: NATIVE = "native" On 2015/06/29 21:00:46, Vadim Sh. wrote: > nit: remove it unless it's used for real Done. https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:15: version='9579504cec0336688292f5b0b68c3ed4e288e273'): On 2015/06/29 21:00:46, Vadim Sh. wrote: > I prefer versions to be expressed as a dict, e.g. > > class CIPDApi(...): > > CLIENT_VERSIONS = { > 'infra/tools/cipd/linux-amd64': '9579....', > 'infra/tools/cipd/mac-amd64': '....', > .... > } Done. https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:16: self.bin_path = ("cipd_%s" % version) On 2015/06/29 21:00:46, Vadim Sh. wrote: > nit: () is unnecessary > > Also use self.m.path to construct an absolute path, e.g. > self.m.path['slave_build'].join('cipd/cipd_%s' % version). Done. https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:17: print ("should install cipd at %s" % self.bin_path) On 2015/06/29 21:00:46, Vadim Sh. wrote: > print is not allowed in recipes. Done. https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:37: for pkg_name in self.c.packages: On 2015/06/29 21:00:46, Vadim Sh. wrote: > sorted(self.c.packages), for reproducibility Done. https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:39: pkg_list.append(("%s %s" % (pkg_name, pkg_spec['version']))) On 2015/06/29 21:00:46, Vadim Sh. wrote: > () is not required Done. https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:44: self.m.file.write("write_test", "pkg_list.txt", "\n".join(pkg_list)) On 2015/06/29 21:00:46, Vadim Sh. wrote: > this is unnecessary, use raw_io module instead (search code for raw_io.input for > examples). Done. https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:52: script=self.resource('ensure.py'), On 2015/06/29 21:00:46, Vadim Sh. wrote: > what is the purpose to ensure.py? Why not > > self.m.step( > name="ensure_installed", > [self.bin_path, "ensure", ....]) > > ? Done. https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/cipd/config.py (right): https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/config.py:1: from recipe_engine.config import config_item_context On 2015/06/29 21:00:46, Vadim Sh. wrote: > I personally don't understand how this config stuff works and why it is needed > in CIPD module, imho: > > api.cipd.ensure_installed(root, packages) is good enough. IIRC iannucci@ suggested this approach. I'm fine with taking it out :) https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/cipd/resources/bootstrap.py (right): https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2015/06/29 21:00:46, Vadim Sh. wrote: > this script is very GCE-specific. Please remove all GCE-specific code and code > that is not actually involved in running install_cipd_client. Done.
Do you know about build/scripts/tools/run_recipe.py? 1) Create simple "cipd/example.py" recipe. E.g. https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re.... 2) Run it locally for real to test recipe module code: ./scripts/tools/run_recipe cipd:example https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/cipd/config.py (right): https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/config.py:1: from recipe_engine.config import config_item_context On 2015/06/30 17:39:39, seanmccullough wrote: > On 2015/06/29 21:00:46, Vadim Sh. wrote: > > I personally don't understand how this config stuff works and why it is needed > > in CIPD module, imho: > > > > api.cipd.ensure_installed(root, packages) is good enough. > > IIRC iannucci@ suggested this approach. I'm fine with taking it out :) Yeah, let's keep it simple for now and add it later if it brings any benefits. https://codereview.chromium.org/1193813004/diff/80001/masters/master.chromium... File masters/master.chromiumos/config.current.txt (right): https://codereview.chromium.org/1193813004/diff/80001/masters/master.chromium... masters/master.chromiumos/config.current.txt:1: config = { I don't think this file is meant to be included into the CL. Same for slave_pool.json. It's probably some artifact of running presubmit checks :-/ https://codereview.chromium.org/1193813004/diff/80001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/cipd/api.py (right): https://codereview.chromium.org/1193813004/diff/80001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:14: self.bin_path = self.m.path['slave_build'].join('cipd/cipd_%s' % self.CLIENT_VERSIONS[self.platform_tag()]) nit: 80 cols https://codereview.chromium.org/1193813004/diff/80001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:30: return "linux-amd64" nit: use " or ' consistently. Now some strings are '...' others are "...". I think we use '...' in python more. https://codereview.chromium.org/1193813004/diff/80001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/api.py:42: [self.m.path['slave_build'].join('cipd'), "ensure", the binary is cipd/cipd_<version> https://codereview.chromium.org/1193813004/diff/80001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/cipd/resources/bootstrap.py (right): https://codereview.chromium.org/1193813004/diff/80001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:22: from recipe_engine import recipe_api not used (and shouldn't be used) https://codereview.chromium.org/1193813004/diff/80001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:196: print ("Exception installing cipd: %s" % e) I'd prefer traceback to be printed too. https://codereview.chromium.org/1193813004/diff/80001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:197: exit_code = 1 return 1 renaming below will most probably fail anyway https://codereview.chromium.org/1193813004/diff/80001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:200: os.rename('%s/cipd' % bin_path, "cipd") bootstrap script should be idempotent: second call should do nothing if the client is already installed and have correct version. This rename breaks this: if on line 189 will not find existing client.
https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/cipd/config.py (right): https://codereview.chromium.org/1193813004/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/config.py:1: from recipe_engine.config import config_item_context On 2015/06/30 17:58:35, Vadim Sh. wrote: > On 2015/06/30 17:39:39, seanmccullough wrote: > > On 2015/06/29 21:00:46, Vadim Sh. wrote: > > > I personally don't understand how this config stuff works and why it is > needed > > > in CIPD module, imho: > > > > > > api.cipd.ensure_installed(root, packages) is good enough. > > > > IIRC iannucci@ suggested this approach. I'm fine with taking it out :) > > Yeah, let's keep it simple for now and add it later if it brings any benefits. Done. https://codereview.chromium.org/1193813004/diff/80001/masters/master.chromium... File masters/master.chromiumos/config.current.txt (right): https://codereview.chromium.org/1193813004/diff/80001/masters/master.chromium... masters/master.chromiumos/config.current.txt:1: config = { On 2015/06/30 17:58:35, Vadim Sh. wrote: > I don't think this file is meant to be included into the CL. Same for > slave_pool.json. It's probably some artifact of running presubmit checks :-/ Done. https://codereview.chromium.org/1193813004/diff/80001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/cipd/resources/bootstrap.py (right): https://codereview.chromium.org/1193813004/diff/80001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:22: from recipe_engine import recipe_api On 2015/06/30 17:58:35, Vadim Sh. wrote: > not used (and shouldn't be used) Done. https://codereview.chromium.org/1193813004/diff/80001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:196: print ("Exception installing cipd: %s" % e) On 2015/06/30 17:58:35, Vadim Sh. wrote: > I'd prefer traceback to be printed too. Done. https://codereview.chromium.org/1193813004/diff/80001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:197: exit_code = 1 On 2015/06/30 17:58:35, Vadim Sh. wrote: > return 1 > > renaming below will most probably fail anyway moved the rename to inside the try block, after the install. https://codereview.chromium.org/1193813004/diff/80001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:200: os.rename('%s/cipd' % bin_path, "cipd") On 2015/06/30 17:58:35, Vadim Sh. wrote: > bootstrap script should be idempotent: second call should do nothing if the > client is already installed and have correct version. This rename breaks this: > if on line 189 will not find existing client. Switched the order around, so that it checks for the cipd_version *dir* and installs if that is missing. What I'd really rather do is ln -s, instead of copy (then I can check for cipd_version/cipd file instead of just its parent dir). Is there a recommended way to do that in python?
https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/cipd/api.py (right): https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/api.py:14: self.bin_path = self.m.path['slave_build'].join('cipd/cipd_%s' % self.CLIENT_VERSIONS[self.platform_tag()]) 80 cols limit, split in two lines https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/api.py:18: 'version': self.CLIENT_VERSIONS[self.platform_tag()], 'bin_path': self.bin_path and make bootstrap.py install it there. Don't spread knowledge of where stuff is across many files. It should be defined in a single place. https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/api.py:34: for pkg_name in pkgs: sorted(pkgs) https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/api.py:42: [self.m.path['slave_build'].join('cipd'), "ensure", Use self.bin_path. Check that it's not None. If it's None (i.e. 'install_client' wasn't called) raise self.m.step.StepFailure("install_client wasn't called") or something. https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/cipd/resources/bootstrap.py (right): https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:5: import argparse many of these imports are not used, please clean this list https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:114: req.add_header('User-Agent', 'ccompute update-cipd-packages') it's not ccompute. 'recipes bootstrap.py' or whatever https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:195: # move the binary into the current directory. actually, don't use current directory... it can be anything at all. Let recipe decide where client should live and pass it here via data json.
https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/cipd/api.py (right): https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/api.py:14: self.bin_path = self.m.path['slave_build'].join('cipd/cipd_%s' % self.CLIENT_VERSIONS[self.platform_tag()]) On 2015/06/30 23:14:29, Vadim Sh. wrote: > 80 cols limit, split in two lines Done. https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/api.py:18: 'version': self.CLIENT_VERSIONS[self.platform_tag()], On 2015/06/30 23:14:29, Vadim Sh. wrote: > 'bin_path': self.bin_path > > and make bootstrap.py install it there. > > Don't spread knowledge of where stuff is across many files. It should be defined > in a single place. Done. https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/api.py:34: for pkg_name in pkgs: On 2015/06/30 23:14:29, Vadim Sh. wrote: > sorted(pkgs) Done. https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/api.py:42: [self.m.path['slave_build'].join('cipd'), "ensure", On 2015/06/30 23:14:29, Vadim Sh. wrote: > Use self.bin_path. Check that it's not None. If it's None (i.e. 'install_client' > wasn't called) raise self.m.step.StepFailure("install_client wasn't called") or > something. Added this, but raising StepFailure in this condition only produces the following output (no failure message, which seems wrong): @@@HONOR_ZERO_RETURN_CODE@@@ @@@SEED_STEP setup_build@@@ @@@STEP_CURSOR setup_build@@@ @@@STEP_STARTED@@@ @@@STEP_LOG_LINE@run_recipe@To repro this locally, run the following line from a build checkout:@@@ @@@STEP_LOG_LINE@run_recipe@@@@ @@@STEP_LOG_LINE@run_recipe@./scripts/tools/run_recipe.py cipd:example --properties-file - <<EOF@@@ @@@STEP_LOG_LINE@run_recipe@{u'recipe': u'cipd:example'}@@@ @@@STEP_LOG_LINE@run_recipe@EOF@@@ @@@STEP_LOG_LINE@run_recipe@@@@ @@@STEP_LOG_LINE@run_recipe@To run on Windows, you can put the JSON in a file and redirect the@@@ @@@STEP_LOG_LINE@run_recipe@contents of the file into run_recipe.py, with the < operator.@@@ @@@STEP_LOG_END@run_recipe@@@ Running recipe with {u'recipe': u'cipd:example', u'use_mirror': False} @@@STEP_TEXT@<br/>running recipe: "cipd:example"@@@ @@@STEP_CURSOR setup_build@@@ @@@STEP_CLOSED@@@ https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/cipd/resources/bootstrap.py (right): https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:5: import argparse On 2015/06/30 23:14:29, Vadim Sh. wrote: > many of these imports are not used, please clean this list Done. https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:114: req.add_header('User-Agent', 'ccompute update-cipd-packages') On 2015/06/30 23:14:29, Vadim Sh. wrote: > it's not ccompute. 'recipes bootstrap.py' or whatever Done. https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:195: # move the binary into the current directory. On 2015/06/30 23:14:29, Vadim Sh. wrote: > actually, don't use current directory... it can be anything at all. Let recipe > decide where client should live and pass it here via data json. Done.
https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/cipd/api.py (right): https://codereview.chromium.org/1193813004/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/api.py:42: [self.m.path['slave_build'].join('cipd'), "ensure", On 2015/06/30 23:48:25, seanmccullough wrote: > On 2015/06/30 23:14:29, Vadim Sh. wrote: > > Use self.bin_path. Check that it's not None. If it's None (i.e. > 'install_client' > > wasn't called) raise self.m.step.StepFailure("install_client wasn't called") > or > > something. > > Added this, but raising StepFailure in this condition only produces the > following output (no failure message, which seems wrong): > > @@@HONOR_ZERO_RETURN_CODE@@@ > > @@@SEED_STEP setup_build@@@ > > @@@STEP_CURSOR setup_build@@@ > > @@@STEP_STARTED@@@ > > @@@STEP_LOG_LINE@run_recipe@To repro this locally, run the following line from a > build checkout:@@@ > > @@@STEP_LOG_LINE@run_recipe@@@@ > > @@@STEP_LOG_LINE@run_recipe@./scripts/tools/run_recipe.py cipd:example > --properties-file - <<EOF@@@ > > @@@STEP_LOG_LINE@run_recipe@{u'recipe': u'cipd:example'}@@@ > > @@@STEP_LOG_LINE@run_recipe@EOF@@@ > > @@@STEP_LOG_LINE@run_recipe@@@@ > > @@@STEP_LOG_LINE@run_recipe@To run on Windows, you can put the JSON in a file > and redirect the@@@ > > @@@STEP_LOG_LINE@run_recipe@contents of the file into run_recipe.py, with the < > operator.@@@ > > @@@STEP_LOG_END@run_recipe@@@ > > Running recipe with {u'recipe': u'cipd:example', u'use_mirror': False} > > @@@STEP_TEXT@<br/>running recipe: "cipd:example"@@@ > > @@@STEP_CURSOR setup_build@@@ > > @@@STEP_CLOSED@@@ Dunno.. Based on this log, no steps were called at all. It should work: https://code.google.com/p/chromium/codesearch#search/&q=raise%20self.m.step.S... https://codereview.chromium.org/1193813004/diff/140001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/cipd/api.py (right): https://codereview.chromium.org/1193813004/diff/140001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/api.py:18: self.bin_path = self.m.path['slave_build'].join( actually, bin_path doesn't have to depend on version or instance ID. Bootstrap script keeps VERSION file inside it to keep track of installed version. This should be enough: self.bin_path = self.m.path['slave_build'].join('cipd') https://codereview.chromium.org/1193813004/diff/140001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/cipd/resources/bootstrap.py (right): https://codereview.chromium.org/1193813004/diff/140001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:182: if not os.path.isdir(bin_path): exe_path = os.path.join(bin_path, 'cipd') try: if not os.path.isfile(exe_path): out = install_cipd_client(bin_path, package, version) assert out == exe_path except Exception as e: .... 1) Presence of the directory alone doesn't mean the client was successfully installed before. Last step of "install_cipd_client" is write_file call that atomically creates <bin_path>/cipd file (at least on Posix), so os.path.isfile check is much more reliable. 2) Assert to make sure this if doesn't silently regress to "always bootstrap" is something changes. 3) No need to rename, since recipe knows about bin_path. 4) Also no need to cleanup old version, there will be no old versions.
https://codereview.chromium.org/1193813004/diff/140001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/cipd/api.py (right): https://codereview.chromium.org/1193813004/diff/140001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/api.py:18: self.bin_path = self.m.path['slave_build'].join( On 2015/07/01 00:10:57, Vadim Sh. wrote: > actually, bin_path doesn't have to depend on version or instance ID. Bootstrap > script keeps VERSION file inside it to keep track of installed version. > > This should be enough: > self.bin_path = self.m.path['slave_build'].join('cipd') Done. https://codereview.chromium.org/1193813004/diff/140001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/cipd/resources/bootstrap.py (right): https://codereview.chromium.org/1193813004/diff/140001/scripts/slave/recipe_m... scripts/slave/recipe_modules/cipd/resources/bootstrap.py:182: if not os.path.isdir(bin_path): On 2015/07/01 00:10:57, Vadim Sh. wrote: > exe_path = os.path.join(bin_path, 'cipd') > try: > if not os.path.isfile(exe_path): > out = install_cipd_client(bin_path, package, version) > assert out == exe_path > except Exception as e: > .... > > 1) Presence of the directory alone doesn't mean the client was successfully > installed before. Last step of "install_cipd_client" is write_file call that > atomically creates <bin_path>/cipd file (at least on Posix), so os.path.isfile > check is much more reliable. > 2) Assert to make sure this if doesn't silently regress to "always bootstrap" is > something changes. > 3) No need to rename, since recipe knows about bin_path. > 4) Also no need to cleanup old version, there will be no old versions. Done.
lgtm
The CQ bit was checked by seanmccullough@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1193813004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: build_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/build_presubmit...)
The CQ bit was checked by seanmccullough@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org Link to the patchset: https://codereview.chromium.org/1193813004/#ps180001 (title: "coverage -> 100%")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1193813004/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=295890 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
