Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9159067:
Small cleanup (Closed)
|
Created:
8 years, 10 months ago by robertm Modified:
8 years, 10 months ago CC:
chromium-reviews, pam+watch_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionSmall cleanup
This is just to get my feet wet before getting more serious about adding
pnacl support.
Mostly adding comments and a tiny bit of cleanup
Two questions:
* where does this get invoked from?
I saw some mentioning here:
tools/build/scripts/master/factory/gyp_factory.py
bot this is not part of my chrome client
* Where do the toolchain tarballs get downloaded?
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120100
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 4
Patch Set 4 : '' #
Total comments: 8
Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #Messages
Total messages: 17 (0 generated)
PTAL
Two nits. Otherwise LGTM, but I defer to Noel. https://chromiumcodereview.appspot.com/9159067/diff/4001/native_client_sdk/sr... File native_client_sdk/src/build_tools/buildbot_run.py (right): https://chromiumcodereview.appspot.com/9159067/diff/4001/native_client_sdk/sr... native_client_sdk/src/build_tools/buildbot_run.py:12: If the script is invoked with the single argument 'pnacl' Ultra-nit: two spaces should be one. https://chromiumcodereview.appspot.com/9159067/diff/4001/native_client_sdk/sr... native_client_sdk/src/build_tools/buildbot_run.py:53: # For loca runs just make sure gsutil is in your PATH s/loca/local/ and add . at the end.
PTAL https://chromiumcodereview.appspot.com/9159067/diff/4001/native_client_sdk/sr... File native_client_sdk/src/build_tools/buildbot_run.py (right): https://chromiumcodereview.appspot.com/9159067/diff/4001/native_client_sdk/sr... native_client_sdk/src/build_tools/buildbot_run.py:12: If the script is invoked with the single argument 'pnacl' On 2012/01/31 16:38:11, sehr wrote: > Ultra-nit: two spaces should be one. Done. https://chromiumcodereview.appspot.com/9159067/diff/4001/native_client_sdk/sr... native_client_sdk/src/build_tools/buildbot_run.py:53: # For loca runs just make sure gsutil is in your PATH On 2012/01/31 16:38:11, sehr wrote: > s/loca/local/ and add . at the end. Done.
Another thing which is not quite clear to me. It seems that sdk bot assumes that the toolchain tarball is already in place. Who/what is responsible for putting it there? What is the appropriate solution for pnacl? Is it ok for now to download the TC tarball ourselves
https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/sr... File native_client_sdk/src/build_tools/buildbot_run.py (right): https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/sr... native_client_sdk/src/build_tools/buildbot_run.py:9: to package an NNaCl SDK. It automatically determines whether NNaCl https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/sr... native_client_sdk/src/build_tools/buildbot_run.py:12: If the script is invoked with the single argument 'pnacl' This comment does not match the code. https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/sr... native_client_sdk/src/build_tools/buildbot_run.py:13: an PNaCl SDK is build instead. an? https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/sr... native_client_sdk/src/build_tools/buildbot_run.py:57: '''Write and error to stderr, then exit with 1 signaling failure.''' Why are you gratuitously changing these? We do not use ''' more often than """.
On 2012/01/31 18:59:04, robertm wrote: > Another thing which is not quite clear to me. > It seems that sdk bot assumes that the toolchain tarball is already > in place. Who/what is responsible for putting it there? What is the appropriate > solution for pnacl? > Is it ok for now to download the TC tarball ourselves DEPS
PTAL https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/sr... File native_client_sdk/src/build_tools/buildbot_run.py (right): https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/sr... native_client_sdk/src/build_tools/buildbot_run.py:9: to package an NNaCl SDK. It automatically determines whether took out the pnacl references for now. Will add them to the next CL On 2012/01/31 18:59:07, noelallen1 wrote: > NNaCl https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/sr... native_client_sdk/src/build_tools/buildbot_run.py:12: If the script is invoked with the single argument 'pnacl' dito On 2012/01/31 18:59:07, noelallen1 wrote: > This comment does not match the code. https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/sr... native_client_sdk/src/build_tools/buildbot_run.py:13: an PNaCl SDK is build instead. dito On 2012/01/31 18:59:07, noelallen1 wrote: > an? https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/sr... native_client_sdk/src/build_tools/buildbot_run.py:57: '''Write and error to stderr, then exit with 1 signaling failure.''' we should use only one to be style guide compliant. ''' was used in the header and """ for function comments. since we also using single quotes for regular strings it seems consistent to also use them for multi-line strings. The change is low risk but if you prefer I can change it to using """ everywhere. On 2012/01/31 18:59:07, noelallen1 wrote: > Why are you gratuitously changing these? We do not use ''' more often than """.
AFAIK our style tends to be: ' ' -> strings """ """ -> comments If you are making a style change, then make it a separate CL, and apply it to all the files in that part of the repo. If you look at native_client/build/download_toolchains.py that is the direction the NaCl sdk is going. lastchange.py which this includes is almost there with " " incorrectly used on optparse, but otherwise consistent. On Tue, Jan 31, 2012 at 11:06 AM, <robertm@google.com> wrote: > PTAL > > > > https://chromiumcodereview.**appspot.com/9159067/diff/8001/** > native_client_sdk/src/build_**tools/buildbot_run.py<https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/src/build_tools/buildbot_run.py> > File native_client_sdk/src/build_**tools/buildbot_run.py (right): > > https://chromiumcodereview.**appspot.com/9159067/diff/8001/** > native_client_sdk/src/build_**tools/buildbot_run.py#newcode9<https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/src/build_tools/buildbot_run.py#newcode9> > native_client_sdk/src/build_**tools/buildbot_run.py:9: to package an NNaCl > SDK. It automatically determines whether > took out the pnacl references for now. > Will add them to the next CL > > > On 2012/01/31 18:59:07, noelallen1 wrote: > >> NNaCl >> > > https://chromiumcodereview.**appspot.com/9159067/diff/8001/** > native_client_sdk/src/build_**tools/buildbot_run.py#**newcode12<https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/src/build_tools/buildbot_run.py#newcode12> > native_client_sdk/src/build_**tools/buildbot_run.py:12: If the script is > invoked with the single argument 'pnacl' > dito > > On 2012/01/31 18:59:07, noelallen1 wrote: > >> This comment does not match the code. >> > > https://chromiumcodereview.**appspot.com/9159067/diff/8001/** > native_client_sdk/src/build_**tools/buildbot_run.py#**newcode13<https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/src/build_tools/buildbot_run.py#newcode13> > native_client_sdk/src/build_**tools/buildbot_run.py:13: an PNaCl SDK is > build instead. > dito > > On 2012/01/31 18:59:07, noelallen1 wrote: > >> an? >> > > https://chromiumcodereview.**appspot.com/9159067/diff/8001/** > native_client_sdk/src/build_**tools/buildbot_run.py#**newcode57<https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/src/build_tools/buildbot_run.py#newcode57> > native_client_sdk/src/build_**tools/buildbot_run.py:57: '''Write and error > to stderr, then exit with 1 signaling failure.''' > we should use only one to be style guide compliant. > ''' was used in the header and > """ for function comments. > > since we also using single quotes for regular strings > it seems consistent to also use them for multi-line strings. > > The change is low risk but if you prefer I can change it to > using """ everywhere. > > > On 2012/01/31 18:59:07, noelallen1 wrote: > >> Why are you gratuitously changing these? We do not use ''' more often >> > than """. > > https://chromiumcodereview.**appspot.com/9159067/<https://chromiumcodereview.... >
Ok, changed ''' -> """ including the multiline module comment. PTAL On 2012/01/31 19:17:33, noelallen wrote: > AFAIK our style tends to be: > ' ' -> strings > """ """ -> comments > > If you are making a style change, then make it a separate CL, and apply it > to all the files in that part of the repo. If you look at > native_client/build/download_toolchains.py that is the direction the NaCl > sdk is going. lastchange.py which this includes is almost there with " " > incorrectly used on optparse, but otherwise consistent. > > > On Tue, Jan 31, 2012 at 11:06 AM, <mailto:robertm@google.com> wrote: > > > PTAL > > > > > > > > https://chromiumcodereview.**appspot.com/9159067/diff/8001/** > > > native_client_sdk/src/build_**tools/buildbot_run.py<https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/src/build_tools/buildbot_run.py> > > File native_client_sdk/src/build_**tools/buildbot_run.py (right): > > > > https://chromiumcodereview.**appspot.com/9159067/diff/8001/** > > > native_client_sdk/src/build_**tools/buildbot_run.py#newcode9<https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/src/build_tools/buildbot_run.py#newcode9> > > native_client_sdk/src/build_**tools/buildbot_run.py:9: to package an NNaCl > > SDK. It automatically determines whether > > took out the pnacl references for now. > > Will add them to the next CL > > > > > > On 2012/01/31 18:59:07, noelallen1 wrote: > > > >> NNaCl > >> > > > > https://chromiumcodereview.**appspot.com/9159067/diff/8001/** > > > native_client_sdk/src/build_**tools/buildbot_run.py#**newcode12<https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/src/build_tools/buildbot_run.py#newcode12> > > native_client_sdk/src/build_**tools/buildbot_run.py:12: If the script is > > invoked with the single argument 'pnacl' > > dito > > > > On 2012/01/31 18:59:07, noelallen1 wrote: > > > >> This comment does not match the code. > >> > > > > https://chromiumcodereview.**appspot.com/9159067/diff/8001/** > > > native_client_sdk/src/build_**tools/buildbot_run.py#**newcode13<https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/src/build_tools/buildbot_run.py#newcode13> > > native_client_sdk/src/build_**tools/buildbot_run.py:13: an PNaCl SDK is > > build instead. > > dito > > > > On 2012/01/31 18:59:07, noelallen1 wrote: > > > >> an? > >> > > > > https://chromiumcodereview.**appspot.com/9159067/diff/8001/** > > > native_client_sdk/src/build_**tools/buildbot_run.py#**newcode57<https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/src/build_tools/buildbot_run.py#newcode57> > > native_client_sdk/src/build_**tools/buildbot_run.py:57: '''Write and error > > to stderr, then exit with 1 signaling failure.''' > > we should use only one to be style guide compliant. > > ''' was used in the header and > > """ for function comments. > > > > since we also using single quotes for regular strings > > it seems consistent to also use them for multi-line strings. > > > > The change is low risk but if you prefer I can change it to > > using """ everywhere. > > > > > > On 2012/01/31 18:59:07, noelallen1 wrote: > > > >> Why are you gratuitously changing these? We do not use ''' more often > >> > > than """. > > > > > https://chromiumcodereview.**appspot.com/9159067/%3Chttps://chromiumcoderevie...> > >
Oh, and can you point me at the place where this script gets invoked from. On 2012/01/31 19:24:51, robertm wrote: > Ok, changed ''' -> """ including the multiline module comment. > > PTAL > > > On 2012/01/31 19:17:33, noelallen wrote: > > AFAIK our style tends to be: > > ' ' -> strings > > """ """ -> comments > > > > If you are making a style change, then make it a separate CL, and apply it > > to all the files in that part of the repo. If you look at > > native_client/build/download_toolchains.py that is the direction the NaCl > > sdk is going. lastchange.py which this includes is almost there with " " > > incorrectly used on optparse, but otherwise consistent. > > > > > > On Tue, Jan 31, 2012 at 11:06 AM, <mailto:robertm@google.com> wrote: > > > > > PTAL > > > > > > > > > > > > https://chromiumcodereview.**appspot.com/9159067/diff/8001/** > > > > > > native_client_sdk/src/build_**tools/buildbot_run.py<https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/src/build_tools/buildbot_run.py> > > > File native_client_sdk/src/build_**tools/buildbot_run.py (right): > > > > > > https://chromiumcodereview.**appspot.com/9159067/diff/8001/** > > > > > > native_client_sdk/src/build_**tools/buildbot_run.py#newcode9<https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/src/build_tools/buildbot_run.py#newcode9> > > > native_client_sdk/src/build_**tools/buildbot_run.py:9: to package an NNaCl > > > SDK. It automatically determines whether > > > took out the pnacl references for now. > > > Will add them to the next CL > > > > > > > > > On 2012/01/31 18:59:07, noelallen1 wrote: > > > > > >> NNaCl > > >> > > > > > > https://chromiumcodereview.**appspot.com/9159067/diff/8001/** > > > > > > native_client_sdk/src/build_**tools/buildbot_run.py#**newcode12<https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/src/build_tools/buildbot_run.py#newcode12> > > > native_client_sdk/src/build_**tools/buildbot_run.py:12: If the script is > > > invoked with the single argument 'pnacl' > > > dito > > > > > > On 2012/01/31 18:59:07, noelallen1 wrote: > > > > > >> This comment does not match the code. > > >> > > > > > > https://chromiumcodereview.**appspot.com/9159067/diff/8001/** > > > > > > native_client_sdk/src/build_**tools/buildbot_run.py#**newcode13<https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/src/build_tools/buildbot_run.py#newcode13> > > > native_client_sdk/src/build_**tools/buildbot_run.py:13: an PNaCl SDK is > > > build instead. > > > dito > > > > > > On 2012/01/31 18:59:07, noelallen1 wrote: > > > > > >> an? > > >> > > > > > > https://chromiumcodereview.**appspot.com/9159067/diff/8001/** > > > > > > native_client_sdk/src/build_**tools/buildbot_run.py#**newcode57<https://chromiumcodereview.appspot.com/9159067/diff/8001/native_client_sdk/src/build_tools/buildbot_run.py#newcode57> > > > native_client_sdk/src/build_**tools/buildbot_run.py:57: '''Write and error > > > to stderr, then exit with 1 signaling failure.''' > > > we should use only one to be style guide compliant. > > > ''' was used in the header and > > > """ for function comments. > > > > > > since we also using single quotes for regular strings > > > it seems consistent to also use them for multi-line strings. > > > > > > The change is low risk but if you prefer I can change it to > > > using """ everywhere. > > > > > > > > > On 2012/01/31 18:59:07, noelallen1 wrote: > > > > > >> Why are you gratuitously changing these? We do not use ''' more often > > >> > > > than """. > > > > > > > > > https://chromiumcodereview.**appspot.com/9159067/%253Chttps://chromiumcoderev...> > > >
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robertm@google.com/9159067/10005
Presubmit check for 9159067-10005 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. If this change requires manual test instructions to QA team, add TEST=[instructions]. ** Presubmit ERRORS ** Missing LGTM from an OWNER for: native_client_sdk/src/build_tools/buildbot_run.py
Noel: If got a chance can you send me an LGTM from your chromium.org account On 2012/02/01 14:45:39, I haz the power (commit-bot) wrote: > Presubmit check for 9159067-10005 failed and returned exit status 1. > > Running presubmit commit checks ... > > ** Presubmit Messages ** > If this change has an associated bug, add BUG=[bug number]. > > If this change requires manual test instructions to QA team, add > TEST=[instructions]. > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for: > native_client_sdk/src/build_tools/buildbot_run.py
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robertm@google.com/9159067/10005
Change committed as 120100 |
