Build Tools Cleanup

Build Tools Cleanup

Cleans up Python Code in Build Tools

TEST: build everything and run tests
BUG: https://code.google.com/p/dart/issues/detail?id=19592

R=ricow@google.com

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

[kaendfinger, 2014-06-21 09:08:47]

Cleanup for Build Tools

[ricow1, 2014-06-23 06:58:22]

generally LGTM, and thank you.
I seriously dislike prefixing variable names with the_ though

https://codereview.chromium.org/350483003/diff/20001/tools/android_link.py
File tools/android_link.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/android_link.py#ne...
tools/android_link.py:3: # Copyright (c) 2014 The Dart Authors. All rights
reserved.
We don't normally fixup the year on changes to files, only on new files

https://codereview.chromium.org/350483003/diff/20001/tools/archive_crash.py
File tools/archive_crash.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/archive_crash.py#n...
tools/archive_crash.py:22: def CreateTarball(the_dir, tarname):
why this change?

https://codereview.chromium.org/350483003/diff/20001/tools/create_tarball.py
File tools/create_tarball.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/create_tarball.py#...
tools/create_tarball.py:132: the_copyright = join(temp_dir, 'copyright')
the_ ?

https://codereview.chromium.org/350483003/diff/20001/tools/get_archive.py
File tools/get_archive.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/get_archive.py#new...
tools/get_archive.py:135: def FindPermanentUrl(out, osname, the_revision_num):
the_?

https://codereview.chromium.org/350483003/diff/20001/tools/get_archive.py#new...
tools/get_archive.py:142: latest = (permanent_prefix % { 'osname' : osname,
'num1' : the_revision_num,
long line

https://codereview.chromium.org/350483003/diff/20001/tools/revert.py
File tools/revert.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/revert.py#newcode161
tools/revert.py:161: for the_revert in reverts:
the_?

[kustermann, 2014-06-23 07:56:42]

DBC

https://codereview.chromium.org/350483003/diff/20001/tools/build.py
File tools/build.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/build.py#newcode237
tools/build.py:237: return not the_chunk or (len(the_chunk) == 4 and
the_chunk[1:] == empty_chunk)
long line

https://codereview.chromium.org/350483003/diff/20001/tools/compile_java/compi...
File tools/compile_java/compile_java.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/compile_java/compi...
tools/compile_java/compile_java.py:69: # big compilation step.
Please move this comment up to where the heap settings are.

https://codereview.chromium.org/350483003/diff/20001/tools/copy_dart.py
File tools/copy_dart.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/copy_dart.py#newco...
tools/copy_dart.py:78: print "Usage: %s OUTDIR INPUTS" % sys.argv[0]
The fact that this args variable here was never defined leads me to the
conclusion that this script did not work before / was unused before.

@ricow: Maybe we should get rid of it?

https://codereview.chromium.org/350483003/diff/20001/tools/revert.py
File tools/revert.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/revert.py#newcode122
tools/revert.py:122: global current_commit_id
I don't see a global current_commit_id variable.

Remember that variables inside a python function live all in the same scope --
there is no block scoping.

But you could potentially initialize it here with
current_commit_id = None

[ricow1, 2014-06-23 08:03:14]

https://codereview.chromium.org/350483003/diff/20001/tools/copy_dart.py
File tools/copy_dart.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/copy_dart.py#newco...
tools/copy_dart.py:78: print "Usage: %s OUTDIR INPUTS" % sys.argv[0]
On 2014/06/23 07:56:42, kustermann wrote:
> The fact that this args variable here was never defined leads me to the
> conclusion that this script did not work before / was unused before.
> 
> @ricow: Maybe we should get rid of it?
This is called from:
tools/dom/scripts/dartdomgenerator.py
Note that this code is only ever called if both outdir and inputs are not
defined

[kustermann, 2014-06-23 08:36:56]

https://codereview.chromium.org/350483003/diff/20001/tools/copy_dart.py
File tools/copy_dart.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/copy_dart.py#newco...
tools/copy_dart.py:78: print "Usage: %s OUTDIR INPUTS" % sys.argv[0]
On 2014/06/23 08:03:13, ricow1 wrote:
> On 2014/06/23 07:56:42, kustermann wrote:
> > The fact that this args variable here was never defined leads me to the
> > conclusion that this script did not work before / was unused before.
> > 
> > @ricow: Maybe we should get rid of it?
> This is called from:
> tools/dom/scripts/dartdomgenerator.py
> Note that this code is only ever called if both outdir and inputs are not
> defined

Good point.

[kaendfinger, 2014-06-24 05:54:17]

On 2014/06/23 06:58:22, ricow1 wrote:
> generally LGTM, and thank you.
> I seriously dislike prefixing variable names with the_ though
> 
> https://codereview.chromium.org/350483003/diff/20001/tools/android_link.py
> File tools/android_link.py (right):
> 
>
https://codereview.chromium.org/350483003/diff/20001/tools/android_link.py#ne...
> tools/android_link.py:3: # Copyright (c) 2014 The Dart Authors. All rights
> reserved.
> We don't normally fixup the year on changes to files, only on new files
> 
> https://codereview.chromium.org/350483003/diff/20001/tools/archive_crash.py
> File tools/archive_crash.py (right):
> 
>
https://codereview.chromium.org/350483003/diff/20001/tools/archive_crash.py#n...
> tools/archive_crash.py:22: def CreateTarball(the_dir, tarname):
> why this change?
> 
> https://codereview.chromium.org/350483003/diff/20001/tools/create_tarball.py
> File tools/create_tarball.py (right):
> 
>
https://codereview.chromium.org/350483003/diff/20001/tools/create_tarball.py#...
> tools/create_tarball.py:132: the_copyright = join(temp_dir, 'copyright')
> the_ ?
> 
> https://codereview.chromium.org/350483003/diff/20001/tools/get_archive.py
> File tools/get_archive.py (right):
> 
>
https://codereview.chromium.org/350483003/diff/20001/tools/get_archive.py#new...
> tools/get_archive.py:135: def FindPermanentUrl(out, osname, the_revision_num):
> the_?
> 
>
https://codereview.chromium.org/350483003/diff/20001/tools/get_archive.py#new...
> tools/get_archive.py:142: latest = (permanent_prefix % { 'osname' : osname,
> 'num1' : the_revision_num,
> long line
> 
> https://codereview.chromium.org/350483003/diff/20001/tools/revert.py
> File tools/revert.py (right):
> 
>
https://codereview.chromium.org/350483003/diff/20001/tools/revert.py#newcode161
> tools/revert.py:161: for the_revert in reverts:
> the_?

The the_ prefixing is done because the variable name that was used conflicts
with global builtins in Python, which can cause buggy code and weird errors. I
tried to use something that was readable, so in most places the_ is used, but in
cases where it did not make sense to use the_, I used a_ and other such
prefixes.

[kaendfinger, 2014-06-24 05:54:33]

Explanations for the_ prefixing

https://codereview.chromium.org/350483003/diff/20001/tools/android_link.py
File tools/android_link.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/android_link.py#ne...
tools/android_link.py:3: # Copyright (c) 2014 The Dart Authors. All rights
reserved.
On 2014/06/23 06:58:21, ricow1 wrote:
> We don't normally fixup the year on changes to files, only on new files
From what I read on the Contributing page, I thought I had to. Is it a big deal?

https://codereview.chromium.org/350483003/diff/20001/tools/archive_crash.py
File tools/archive_crash.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/archive_crash.py#n...
tools/archive_crash.py:22: def CreateTarball(the_dir, tarname):
On 2014/06/23 06:58:21, ricow1 wrote:
> why this change?
All of the the_ kind of stuff is due to the overriding of builtin variables in
Python. I used the JetBrains PyCharm IDE for this, and it told me it was a good
idea to not override the variable, for hopefully obvious reasons.

[ricow1, 2014-06-24 07:41:56]

https://codereview.chromium.org/350483003/diff/20001/tools/android_link.py
File tools/android_link.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/android_link.py#ne...
tools/android_link.py:3: # Copyright (c) 2014 The Dart Authors. All rights
reserved.
On 2014/06/24 05:54:33, kaendfinger wrote:
> On 2014/06/23 06:58:21, ricow1 wrote:
> > We don't normally fixup the year on changes to files, only on new files
> From what I read on the Contributing page, I thought I had to. Is it a big
deal?
Nope, just leave it changed, but in anticipation of all the future patches I
don't want you to waste your time doing this :-) We changed this policy last
year I think - consistent with what chromium does. I have updated the wiki page
- thank you

https://codereview.chromium.org/350483003/diff/20001/tools/archive_crash.py
File tools/archive_crash.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/archive_crash.py#n...
tools/archive_crash.py:22: def CreateTarball(the_dir, tarname):
On 2014/06/24 05:54:33, kaendfinger wrote:
> On 2014/06/23 06:58:21, ricow1 wrote:
> > why this change?
> All of the the_ kind of stuff is due to the overriding of builtin variables in
> Python. I used the JetBrains PyCharm IDE for this, and it told me it was a
good
> idea to not override the variable, for hopefully obvious reasons.
The reason is valid, but the_ is not that nice. It is almost always possible to
find a much better name, in this specific case input_dir (but of course that is
context dependent)

https://codereview.chromium.org/350483003/diff/20001/tools/build.py
File tools/build.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/build.py#newcode235
tools/build.py:235: def is_empty_chunk(the_chunk):
the_chunk -> input

https://codereview.chromium.org/350483003/diff/20001/tools/build.py#newcode239
tools/build.py:239: def unbuffered(the_callable):
the_callable -> input

https://codereview.chromium.org/350483003/diff/20001/tools/build.py#newcode239
tools/build.py:239: def unbuffered(the_callable):
the_callable -> input or stream

https://codereview.chromium.org/350483003/diff/20001/tools/build.py#newcode382
tools/build.py:382: for the_arch in options.arch:
what is arch shadowing? Is this simply because you made it global in parse
options?

https://codereview.chromium.org/350483003/diff/20001/tools/compile_java/compi...
File tools/compile_java/compile_java.py (left):

https://codereview.chromium.org/350483003/diff/20001/tools/compile_java/compi...
tools/compile_java/compile_java.py:114: if propertyFiles == []:
this is probably more saying, we already assume above that this is actually a
list (we iterate it)

https://codereview.chromium.org/350483003/diff/20001/tools/compile_java/compi...
File tools/compile_java/compile_java.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/compile_java/compi...
tools/compile_java/compile_java.py:104: for the_file in propertyFiles:
the_file -> propertyFile

https://codereview.chromium.org/350483003/diff/20001/tools/create_tarball.py
File tools/create_tarball.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/create_tarball.py#...
tools/create_tarball.py:132: the_copyright = join(temp_dir, 'copyright')
the_copyright -> copyright_file

https://codereview.chromium.org/350483003/diff/20001/tools/get_archive.py
File tools/get_archive.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/get_archive.py#new...
tools/get_archive.py:191: def FindPermanentUrl(osname):
you can't just remove those arguments, GetFromGsutil below will call this with 3
arguments right?

https://codereview.chromium.org/350483003/diff/20001/tools/get_archive.py#new...
tools/get_archive.py:212: FindPermanentUrl, revision_num)
not your change, but indentation

https://codereview.chromium.org/350483003/diff/20001/tools/utils.py
File tools/utils.py (right):

https://codereview.chromium.org/350483003/diff/20001/tools/utils.py#newcode256
tools/utils.py:256: def GetBuildDir(host_os):
did you grep for this in the whole dart repo?

[kaendfinger, 2014-06-27 03:14:01]

On 2014/06/24 07:41:56, ricow1 wrote:
> https://codereview.chromium.org/350483003/diff/20001/tools/android_link.py
> File tools/android_link.py (right):
> 
>
https://codereview.chromium.org/350483003/diff/20001/tools/android_link.py#ne...
> tools/android_link.py:3: # Copyright (c) 2014 The Dart Authors. All rights
> reserved.
> On 2014/06/24 05:54:33, kaendfinger wrote:
> > On 2014/06/23 06:58:21, ricow1 wrote:
> > > We don't normally fixup the year on changes to files, only on new files
> > From what I read on the Contributing page, I thought I had to. Is it a big
> deal?
> Nope, just leave it changed, but in anticipation of all the future patches I
> don't want you to waste your time doing this :-) We changed this policy last
> year I think - consistent with what chromium does. I have updated the wiki
page
> - thank you
> 
> https://codereview.chromium.org/350483003/diff/20001/tools/archive_crash.py
> File tools/archive_crash.py (right):
> 
>
https://codereview.chromium.org/350483003/diff/20001/tools/archive_crash.py#n...
> tools/archive_crash.py:22: def CreateTarball(the_dir, tarname):
> On 2014/06/24 05:54:33, kaendfinger wrote:
> > On 2014/06/23 06:58:21, ricow1 wrote:
> > > why this change?
> > All of the the_ kind of stuff is due to the overriding of builtin variables
in
> > Python. I used the JetBrains PyCharm IDE for this, and it told me it was a
> good
> > idea to not override the variable, for hopefully obvious reasons.
> The reason is valid, but the_ is not that nice. It is almost always possible
to
> find a much better name, in this specific case input_dir (but of course that
is
> context dependent)
> 
> https://codereview.chromium.org/350483003/diff/20001/tools/build.py
> File tools/build.py (right):
> 
> https://codereview.chromium.org/350483003/diff/20001/tools/build.py#newcode235
> tools/build.py:235: def is_empty_chunk(the_chunk):
> the_chunk -> input
> 
> https://codereview.chromium.org/350483003/diff/20001/tools/build.py#newcode239
> tools/build.py:239: def unbuffered(the_callable):
> the_callable -> input
> 
> https://codereview.chromium.org/350483003/diff/20001/tools/build.py#newcode239
> tools/build.py:239: def unbuffered(the_callable):
> the_callable -> input or stream
> 
> https://codereview.chromium.org/350483003/diff/20001/tools/build.py#newcode382
> tools/build.py:382: for the_arch in options.arch:
> what is arch shadowing? Is this simply because you made it global in parse
> options?
> 
>
https://codereview.chromium.org/350483003/diff/20001/tools/compile_java/compi...
> File tools/compile_java/compile_java.py (left):
> 
>
https://codereview.chromium.org/350483003/diff/20001/tools/compile_java/compi...
> tools/compile_java/compile_java.py:114: if propertyFiles == []:
> this is probably more saying, we already assume above that this is actually a
> list (we iterate it)
> 
>
https://codereview.chromium.org/350483003/diff/20001/tools/compile_java/compi...
> File tools/compile_java/compile_java.py (right):
> 
>
https://codereview.chromium.org/350483003/diff/20001/tools/compile_java/compi...
> tools/compile_java/compile_java.py:104: for the_file in propertyFiles:
> the_file -> propertyFile
> 
> https://codereview.chromium.org/350483003/diff/20001/tools/create_tarball.py
> File tools/create_tarball.py (right):
> 
>
https://codereview.chromium.org/350483003/diff/20001/tools/create_tarball.py#...
> tools/create_tarball.py:132: the_copyright = join(temp_dir, 'copyright')
> the_copyright -> copyright_file
> 
> https://codereview.chromium.org/350483003/diff/20001/tools/get_archive.py
> File tools/get_archive.py (right):
> 
>
https://codereview.chromium.org/350483003/diff/20001/tools/get_archive.py#new...
> tools/get_archive.py:191: def FindPermanentUrl(osname):
> you can't just remove those arguments, GetFromGsutil below will call this with
3
> arguments right?
> 
>
https://codereview.chromium.org/350483003/diff/20001/tools/get_archive.py#new...
> tools/get_archive.py:212: FindPermanentUrl, revision_num)
> not your change, but indentation
> 
> https://codereview.chromium.org/350483003/diff/20001/tools/utils.py
> File tools/utils.py (right):
> 
> https://codereview.chromium.org/350483003/diff/20001/tools/utils.py#newcode256
> tools/utils.py:256: def GetBuildDir(host_os):
> did you grep for this in the whole dart repo?

Ok. I fixed everything.

NOTE: I am using PyCharm, so when I rename things, it looks in the whole repo
for usages, therefore, all this code has been checked and verified to be
semantically and syntactically correct.

[ricow1, 2014-06-27 07:35:34]

https://codereview.chromium.org/350483003/diff/40001/tools/build.py
File tools/build.py (right):

https://codereview.chromium.org/350483003/diff/40001/tools/build.py#newcode78
tools/build.py:78: global arch
I don't think we need this

https://codereview.chromium.org/350483003/diff/40001/tools/compile_java/compi...
File tools/compile_java/compile_java.py (right):

https://codereview.chromium.org/350483003/diff/40001/tools/compile_java/compi...
tools/compile_java/compile_java.py:68: # Use a large enough heap to be able to
compile all of the classes in one
this should go up above the command generation

https://codereview.chromium.org/350483003/diff/40001/tools/revert.py
File tools/revert.py (right):

https://codereview.chromium.org/350483003/diff/40001/tools/revert.py#newcode122
tools/revert.py:122: global current_commit_id
no need to global this

https://codereview.chromium.org/350483003/diff/40001/tools/utils.py
File tools/utils.py (right):

https://codereview.chromium.org/350483003/diff/40001/tools/utils.py#newcode64
tools/utils.py:64: print "Warning: Guessing architecture %s based on os %s\n" %
(os_id, guess_os)
long line

https://codereview.chromium.org/350483003/diff/40001/tools/utils.py#newcode478
tools/utils.py:478: sys.stderr.write(string)
you don't import sys anymore!!!!

[kaendfinger, 2014-06-28 19:59:14]

On 2014/06/27 21:39:14, kaendfinger wrote:

Ping

[ricow1, 2014-06-30 07:02:35]

LGTM, I will land this.
PS: if you when you update your code reply done on each of the comments the
reviewer(s) knows that you actually did all the work requested and they have
direct links to the updated code. This saves time for the reviewers and is a
good checklist for the developer doing the changes as well since you can see how
many comments the reviewers have on actual patch set and you can see how many
you responded to.

[kaendfinger, 2014-07-01 17:06:33]

On 2014/06/30 07:02:35, ricow1 wrote:
> LGTM, I will land this.
> PS: if you when you update your code reply done on each of the comments the
> reviewer(s) knows that you actually did all the work requested and they have
> direct links to the updated code. This saves time for the reviewers and is a
> good checklist for the developer doing the changes as well since you can see
how
> many comments the reviewers have on actual patch set and you can see how many
> you responded to.

Ok I will note this for next time.

[kaendfinger, 2014-07-12 00:25:46]

On 2014/07/01 17:06:33, kaendfinger wrote:
> On 2014/06/30 07:02:35, ricow1 wrote:
> > LGTM, I will land this.
> > PS: if you when you update your code reply done on each of the comments the
> > reviewer(s) knows that you actually did all the work requested and they have
> > direct links to the updated code. This saves time for the reviewers and is a
> > good checklist for the developer doing the changes as well since you can see
> how
> > many comments the reviewers have on actual patch set and you can see how
many
> > you responded to.
> 
> Ok I will note this for next time.

Any reason this hasn't been merged?

[kaendfinger, 2014-07-21 16:22:32]

On 2014/06/30 07:02:35, ricow1 wrote:
> LGTM, I will land this.
> PS: if you when you update your code reply done on each of the comments the
> reviewer(s) knows that you actually did all the work requested and they have
> direct links to the updated code. This saves time for the reviewers and is a
> good checklist for the developer doing the changes as well since you can see
how
> many comments the reviewers have on actual patch set and you can see how many
> you responded to.

Why hasn't this been merged yet?

[Bill Hesse, 2014-07-25 11:48:18]

Committed patchset #4 manually as r38573 (presubmit successful).

[Bill Hesse, 2014-07-25 11:48:46]

Committed, without changes to copyright years.