Add script for generating a source tarball and rules for bilding a Debian package

Add script for generating a source tarball and rules for bilding a Debian package

R=ricow@google.com, kusternamm@google.com
BUG=

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

[Søren Gjesse, 2014-02-04 17:42:51]

This is a start. We should discuss the directory structure. That is the debian
directory is moved one level out when making the tarball.

Also should we go with the whitelist approach for files/directories when
generating the tarball.

[ricow1, 2014-02-04 18:16:01]

I don't really know anything about debian packaging.

https://codereview.chromium.org/154073002/diff/1/debian/rules
File debian/rules (right):

https://codereview.chromium.org/154073002/diff/1/debian/rules#newcode43
debian/rules:43: make -C dart -j$(PARALLEL_JOBS) BUILDTYPE=$(BUILD_TYPE)
$(BUILD_ARGS) create_sdk
long line

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py
File tools/create_tarball.py (right):

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py#newc...
tools/create_tarball.py:21: if (tar_info.name.endswith('.git') or
How about we split this up in 3 lists that we have as top level lists, this way
this is really easy to maintain:
ignoredPaths = ['tools/testing/bin']
ignoredDirs = ['.svn', '.git', ...]
ignoredEndings = ['~'] (do we need this, the bots that will archive this will
never create these, we will not even build)
then
_, tail = os.path.split(tar_info.name)
if tail in ignoredDirs:
  return None
for path in ignoredPaths:
  if tar_info.name.startswith(path):
    return None
for ending in ignoredEndings:
  if tar_info.name.endswith(ending):
    return None

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py#newc...
tools/create_tarball.py:34: if tar_info.name.startswith('debian/'):
Since we are already special casing this maybe we should put the debian dir
under tools/alternative_systems/debian and copy it out from there?

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py#newc...
tools/create_tarball.py:47: version = utils.GetVersion()
you may want to add a comment here stating that this is almost static on
bleeding edge ( we only update it when we release a new stable release)

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py#newc...
tools/create_tarball.py:52: tardir =
dirname(dirname(dirname(realpath(__file__))))
why not create this in a temp dir, and copy it to out/... afterwards?

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py#newc...
tools/create_tarball.py:58: for directory in ['debian', 'editor', 'pkg',
'runtime', 'samples', 'sdk',
I would really prefer if we did it the other way around - instead of a whitelist
we should have a blacklist (in the ignoredPaths above if you go that route. This
is just another place we need to maintain otherwise.

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py#newc...
tools/create_tarball.py:61: for directory in ['apache_ant', 'apache_compress',
'args4j', 'bzip2',
Same comment as above

[ricow1, 2014-02-05 06:42:35]

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py
File tools/create_tarball.py (right):

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py#newc...
tools/create_tarball.py:52: tardir =
dirname(dirname(dirname(realpath(__file__))))
On 2014/02/04 18:16:01, ricow1 wrote:
> why not create this in a temp dir, and copy it to out/... afterwards?
That was a pretty stupid suggestion, why not just create this inside out/.. :-)

[Søren Gjesse, 2014-02-06 08:19:08]

PTAL

The copyright and changelog files for the building Debian packages are now
generated. For bleeding_edge the svn revision is added to the tarball for
generating the correct version.

https://codereview.chromium.org/154073002/diff/1/debian/rules
File debian/rules (right):

https://codereview.chromium.org/154073002/diff/1/debian/rules#newcode43
debian/rules:43: make -C dart -j$(PARALLEL_JOBS) BUILDTYPE=$(BUILD_TYPE)
$(BUILD_ARGS) create_sdk
On 2014/02/04 18:16:01, ricow1 wrote:
> long line

Done.

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py
File tools/create_tarball.py (right):

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py#newc...
tools/create_tarball.py:21: if (tar_info.name.endswith('.git') or
On 2014/02/04 18:16:01, ricow1 wrote:
> How about we split this up in 3 lists that we have as top level lists, this
way
> this is really easy to maintain:
> ignoredPaths = ['tools/testing/bin']
> ignoredDirs = ['.svn', '.git', ...]
> ignoredEndings = ['~'] (do we need this, the bots that will archive this will
> never create these, we will not even build)
> then
> _, tail = os.path.split(tar_info.name)
> if tail in ignoredDirs:
>   return None
> for path in ignoredPaths:
>   if tar_info.name.startswith(path):
>     return None
> for ending in ignoredEndings:
>   if tar_info.name.endswith(ending):
>     return None

Good point, done.

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py#newc...
tools/create_tarball.py:34: if tar_info.name.startswith('debian/'):
On 2014/02/04 18:16:01, ricow1 wrote:
> Since we are already special casing this maybe we should put the debian dir
> under tools/alternative_systems/debian and copy it out from there?

Used the directory tools/linux_dist_support/debian.

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py#newc...
tools/create_tarball.py:47: version = utils.GetVersion()
On 2014/02/04 18:16:01, ricow1 wrote:
> you may want to add a comment here stating that this is almost static on
> bleeding edge ( we only update it when we release a new stable release)

It does contain the SVN revision, so not really static.

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py#newc...
tools/create_tarball.py:52: tardir =
dirname(dirname(dirname(realpath(__file__))))
On 2014/02/05 06:42:35, ricow1 wrote:
> On 2014/02/04 18:16:01, ricow1 wrote:
> > why not create this in a temp dir, and copy it to out/... afterwards?
> That was a pretty stupid suggestion, why not just create this inside out/..
:-)

Done.

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py#newc...
tools/create_tarball.py:58: for directory in ['debian', 'editor', 'pkg',
'runtime', 'samples', 'sdk',
On 2014/02/04 18:16:01, ricow1 wrote:
> I would really prefer if we did it the other way around - instead of a
whitelist
> we should have a blacklist (in the ignoredPaths above if you go that route.
This
> is just another place we need to maintain otherwise.

Done.

https://codereview.chromium.org/154073002/diff/1/tools/create_tarball.py#newc...
tools/create_tarball.py:61: for directory in ['apache_ant', 'apache_compress',
'args4j', 'bzip2',
On 2014/02/04 18:16:01, ricow1 wrote:
> Same comment as above

Done.

[ricow1, 2014-02-06 08:45:25]

The changes to tools/utils.py is not showing up in the review tool, could you
reupload?

Archiving code lgtm with some minor suggestions

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py
File tools/create_tarball.py (right):

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:34: # TODO (sgjesse): Remove this when the LICENSE file
becomes part of
Feel free to file a bug and assign it to me, and I will remove this when we
moved in the file (on all channels)

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:99: 'third_party/clang', 'third_party/d8']
maybe add third_party/firefox_jsshell

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:125: if tar_info.name.startswith('debian/'):
this now starts with tools/linux_dist_support/debian/, but looking at the code
in CreateTarball I think we always just unconditionally add this, so this could
be removed

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:170: global ignoredPaths
why do you global this?

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:178: tardir = 'out'
use utils.GetBuildDir

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:183: tar = tarfile.open(tarfilename, mode='w:gz')
I think we assume python 2.7 - so you could do with tarfile.open(..) as tar and
remove the close below

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:186: debian_dir = 'tools/linux_dist_support/debian'
maybe put this at the top of the file collecting all the config variables there?

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:188: tar.add(join('tools/linux_dist_support/debian', f),
tar.add(join(debian_dir, f), ...

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:191: temp_dir = tempfile.mkdtemp()
you can use 
with utils.TempDir() as temp_dir:
then remove the deletion below as well

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:204: full_version = utils.ReadVersionFile()
use utils.GetChannel() instead (in the if statement below, remove this)

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:221: return 0
maybe return non zero here?

[Søren Gjesse, 2014-02-06 13:15:30]

PTAL

Thanks for all the useful comments

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py
File tools/create_tarball.py (right):

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:34: # TODO (sgjesse): Remove this when the LICENSE file
becomes part of
On 2014/02/06 08:45:26, ricow1 wrote:
> Feel free to file a bug and assign it to me, and I will remove this when we
> moved in the file (on all channels)

Thanks, done.

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:99: 'third_party/clang', 'third_party/d8']
On 2014/02/06 08:45:26, ricow1 wrote:
> maybe add third_party/firefox_jsshell

Done.

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:125: if tar_info.name.startswith('debian/'):
On 2014/02/06 08:45:26, ricow1 wrote:
> this now starts with tools/linux_dist_support/debian/, but looking at the code
> in CreateTarball I think we always just unconditionally add this, so this
could
> be removed

Yes, I forgot to remove this, done

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:170: global ignoredPaths
On 2014/02/06 08:45:26, ricow1 wrote:
> why do you global this?

From some testing, removed.

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:178: tardir = 'out'
On 2014/02/06 08:45:26, ricow1 wrote:
> use utils.GetBuildDir

Done.

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:183: tar = tarfile.open(tarfilename, mode='w:gz')
On 2014/02/06 08:45:26, ricow1 wrote:
> I think we assume python 2.7 - so you could do with tarfile.open(..) as tar
and
> remove the close below

Good point, done.

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:186: debian_dir = 'tools/linux_dist_support/debian'
On 2014/02/06 08:45:26, ricow1 wrote:
> maybe put this at the top of the file collecting all the config variables
there?

Done.

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:188: tar.add(join('tools/linux_dist_support/debian', f),
On 2014/02/06 08:45:26, ricow1 wrote:
> tar.add(join(debian_dir, f), ...

Done.

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:191: temp_dir = tempfile.mkdtemp()
On 2014/02/06 08:45:26, ricow1 wrote:
> you can use 
> with utils.TempDir() as temp_dir:
> then remove the deletion below as well

Nice!

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:204: full_version = utils.ReadVersionFile()
On 2014/02/06 08:45:26, ricow1 wrote:
> use utils.GetChannel() instead (in the if statement below, remove this)

Done.

https://codereview.chromium.org/154073002/diff/110001/tools/create_tarball.py...
tools/create_tarball.py:221: return 0
On 2014/02/06 08:45:26, ricow1 wrote:
> maybe return non zero here?

Done.

[ricow1, 2014-02-06 13:25:21]

LGTM

https://codereview.chromium.org/154073002/diff/230001/tools/utils.py
File tools/utils.py (right):

https://codereview.chromium.org/154073002/diff/230001/tools/utils.py#newcode345
tools/utils.py:345: fd = open(svn_revision_file)
I am a sucker for with :-)
try:
  with open(svn_revision_file) as fd:
    return fd.read()
expect:
  pass

[Søren Gjesse, 2014-02-06 14:37:55]

Committed patchset #5 manually as r32359 (presubmit successful).

[kustermann, 2014-02-06 15:28:03]

kusternamm -> kustermann :)

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py
File tools/create_tarball.py (right):

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:17: # Debian is needed.
"to follow the Debian" sounds strange.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:28: import tempfile
tempfile not used.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:31: from os import listdir, makedirs, remove, rmdir
remove, rmdir not used.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:32: from os.path import basename, dirname, join,
realpath, exists, isdir, split
basename, dirname, realpath, isdir unused.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:38: license = [
Python supports multiline strings, why don't you do something like this?:

license = """
..........
..........
"""

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:137: lf = open('../LICENSE', 'r')
'r' is the default + using context manager?

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:139: print license_lines
Why print?

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:160: datetime.datetime.utcnow().strftime('%a, %d %b %Y
%X +0000'))
Maybe use a multiline string? [see comment at license]

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:164: f = open(filename, 'w')
It's usually nicer (and safer if an exception occurs, because then you don't
leak) if you use a context manager:

with open(filename, 'w') as f:
  f.write(svn_revision)

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:180: tarfilename = join(tardir, tarname)
It's slightly spooky, that you tar up the dart directory into a tarball which
lives inside the dart directory. Though you ignore "out", so it's not taring up
itself.

Since this is not invoked by gyp anyway, why do we need to put the tarball in
out/buildDir/*.tar.gz ?
You could add a commandline option for the destination of the tarball.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:181: print 'Creating tarball: %s' % (tarfilename)
No parenthesis needed!

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:183: for f in listdir('.'):
You assume this is run from the root of the dart directory I assume?

Why not ensure it by using DART_DIR as you do in other places?

https://codereview.chromium.org/154073002/diff/270001/tools/linux_dist_suppor...
File tools/linux_dist_support/debian/control (right):

https://codereview.chromium.org/154073002/diff/270001/tools/linux_dist_suppor...
tools/linux_dist_support/debian/control:7: subversion,
Why is subversion a build dependency?
You explicitly ignore all '.svn' directories, right?

https://codereview.chromium.org/154073002/diff/270001/tools/linux_dist_suppor...
File tools/linux_dist_support/debian/dart.links (right):

https://codereview.chromium.org/154073002/diff/270001/tools/linux_dist_suppor...
tools/linux_dist_support/debian/dart.links:1: usr/lib/dart/bin/dart usr/bin/dart
Why not usr/lib/dart-sdk/bin/dart ?

[I guess we may want to change this anyway if we want to being able to install
multiple versions at the same time.]

https://codereview.chromium.org/154073002/diff/270001/tools/utils.py
File tools/utils.py (right):

https://codereview.chromium.org/154073002/diff/270001/tools/utils.py#newcode343
tools/utils.py:343: svn_revision_file = os.path.join(DART_DIR, 'tools',
'SVN_REVISION')
You could make this a constant in utils.py add a comment explaining what it is
and use that constant in create_tarball.py as well.

https://codereview.chromium.org/154073002/diff/270001/tools/utils.py#newcode345
tools/utils.py:345: with open(svn_revision_file) as fd
There is a colon missing here. I don't think this can work without ":".

[Søren Gjesse, 2014-02-07 09:52:43]

Thanks for the comments Martin, they have been addressed in
https://codereview.chromium.org/157383002.

https://codereview.chromium.org/154073002/diff/230001/tools/utils.py
File tools/utils.py (right):

https://codereview.chromium.org/154073002/diff/230001/tools/utils.py#newcode345
tools/utils.py:345: fd = open(svn_revision_file)
On 2014/02/06 13:25:21, ricow1 wrote:
> I am a sucker for with :-)
> try:
>   with open(svn_revision_file) as fd:
>     return fd.read()
> expect:
>   pass

Done.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py
File tools/create_tarball.py (right):

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:17: # Debian is needed.
On 2014/02/06 15:28:04, kustermann wrote:
> "to follow the Debian" sounds strange.

Done.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:28: import tempfile
On 2014/02/06 15:28:04, kustermann wrote:
> tempfile not used.

Removed.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:31: from os import listdir, makedirs, remove, rmdir
On 2014/02/06 15:28:04, kustermann wrote:
> remove, rmdir not used.

Removed.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:32: from os.path import basename, dirname, join,
realpath, exists, isdir, split
On 2014/02/06 15:28:04, kustermann wrote:
> basename, dirname, realpath, isdir unused.

Removed.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:38: license = [
On 2014/02/06 15:28:04, kustermann wrote:
> Python supports multiline strings, why don't you do something like this?:
> 
> license = """
> ..........
> ..........
> """

This will go away when https://codereview.chromium.org/156573003/ lands.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:137: lf = open('../LICENSE', 'r')
On 2014/02/06 15:28:04, kustermann wrote:
> 'r' is the default + using context manager?

Done.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:139: print license_lines
On 2014/02/06 15:28:04, kustermann wrote:
> Why print?

Debug code. Removed.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:160: datetime.datetime.utcnow().strftime('%a, %d %b %Y
%X +0000'))
On 2014/02/06 15:28:04, kustermann wrote:
> Maybe use a multiline string? [see comment at license]

It does not indent as well as this I think.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:164: f = open(filename, 'w')
On 2014/02/06 15:28:04, kustermann wrote:
> It's usually nicer (and safer if an exception occurs, because then you don't
> leak) if you use a context manager:
> 
> with open(filename, 'w') as f:
>   f.write(svn_revision)

Done.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:180: tarfilename = join(tardir, tarname)
On 2014/02/06 15:28:04, kustermann wrote:
> It's slightly spooky, that you tar up the dart directory into a tarball which
> lives inside the dart directory. Though you ignore "out", so it's not taring
up
> itself.
> 
> Since this is not invoked by gyp anyway, why do we need to put the tarball in
> out/buildDir/*.tar.gz ?
> You could add a commandline option for the destination of the tarball.

I started out by generating it in the parent directory to avoid the spooky part.
However placing everything we generate in out seems best.

Adding an option for the tar file is an option, but that way one can easily
place the tar file in an included directory, and we would have to check for
that.

Changed the excluding of out to use the name from GetBuildDir.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:181: print 'Creating tarball: %s' % (tarfilename)
On 2014/02/06 15:28:04, kustermann wrote:
> No parenthesis needed!

Done.

https://codereview.chromium.org/154073002/diff/270001/tools/create_tarball.py...
tools/create_tarball.py:183: for f in listdir('.'):
On 2014/02/06 15:28:04, kustermann wrote:
> You assume this is run from the root of the dart directory I assume?
> 
> Why not ensure it by using DART_DIR as you do in other places?

Done. The script can now be run from anywhere.