Update prebuilt.py to filter Packages database.

Update prebuilt.py to filter Packages database.

This ensures that the Packages database doesn't contain private packages.
This is necessary so that the database matches up with the actual files
we upload.

BUG=chromium-os:4843
TEST=Ran unit test

Change-Id: I0355b4ab867a0d08396e45b04523a487951475f4

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=4f7f855

[scottz, 2010-10-19 23:06:39]

http://codereview.chromium.org/3930001/diff/1/2
File prebuilt.py (right):

http://codereview.chromium.org/3930001/diff/1/2#newcode216
prebuilt.py:216: cpv = line.replace("CPV: ", "").rstrip()
.strip() no sense in being specific to the side.

http://codereview.chromium.org/3930001/diff/1/2#newcode218
prebuilt.py:218: skip = True
shouldn't you have a continue here otherwise buffer.append adds the CPV line to
the buffer?

It is hard for me to tell exactly where the buffer.append is so I may be wrong
here.

http://codereview.chromium.org/3930001/diff/1/2#newcode245
prebuilt.py:245: packages_file = open(local_file)
Why are you opening the file here? Any reason not to keep it all contained in
your FilterPackagesFile function?

http://codereview.chromium.org/3930001/diff/1/2#newcode248
prebuilt.py:248: local_file = filtered_packages_file.name
These files should be cleaned up after they are used. From a space stand point
it clearly doesn't matter but going into /tmp and seeing a ton of files is just
not pretty. Yes technically they will reboot and wipe this but we should clean
up after ourselves.

[davidjames, 2010-10-20 00:38:40]

http://codereview.chromium.org/3930001/diff/1/2
File prebuilt.py (right):

http://codereview.chromium.org/3930001/diff/1/2#newcode216
prebuilt.py:216: cpv = line.replace("CPV: ", "").rstrip()
On 2010/10/19 23:06:40, scottz wrote:
> .strip() no sense in being specific to the side. 

Newlines are only on the right side :) Packages files are in a specific format
so there should never need to be any stripping on the left.

http://codereview.chromium.org/3930001/diff/1/2#newcode218
prebuilt.py:218: skip = True
On 2010/10/19 23:06:40, scottz wrote:
> shouldn't you have a continue here otherwise buffer.append adds the CPV line
to
> the buffer?
> 
> It is hard for me to tell exactly where the buffer.append is so I may be wrong
> here. 

The code is correct, but maybe this function needs to be split into two to make
it more understandable. I've done this.

http://codereview.chromium.org/3930001/diff/1/2#newcode245
prebuilt.py:245: packages_file = open(local_file)
On 2010/10/19 23:06:40, scottz wrote:
> Why are you opening the file here? Any reason not to keep it all contained in
> your FilterPackagesFile function?

This is to make the unit testing easier. I didn't want to have to override
file() in the unit test.

http://codereview.chromium.org/3930001/diff/1/2#newcode248
prebuilt.py:248: local_file = filtered_packages_file.name
On 2010/10/19 23:06:40, scottz wrote:
> These files should be cleaned up after they are used. From a space stand point
> it clearly doesn't matter but going into /tmp and seeing a ton of files is
just
> not pretty. Yes technically they will reboot and wipe this but we should clean
> up after ourselves.

NamedTemporaryFile deletes the object automatically when the function exits.
I've added a comment.

[scottz, 2010-10-20 19:19:39]

It is a matter of taste so I am LGTMing this but I highly recommend making your
function open and close the file. You can test this in your unittest by using
tempfiles, which ensures the whole process is tested not just the parsing. 

Either way in general this update looks really good thanks for cranking this
out!

[sosa, 2010-10-20 19:56:14]

What's wrong with mocking out _builtins_.open() and returning a mock
file?  Is it so bad?

On Wed, Oct 20, 2010 at 12:19 PM,  <scottz@chromium.org> wrote:
> It is a matter of taste so I am LGTMing this but I highly recommend making
> your
> function open and close the file. You can test this in your unittest by
> using
> tempfiles, which ensures the whole process is tested not just the parsing.
>
> Either way in general this update looks really good thanks for cranking this
> out!
>
> http://codereview.chromium.org/3930001/show
>

[scottz, 2010-10-20 21:48:24]

That is fine as well. I am of the opinion though that if you are mucking with
files and you have a testcase that has all the strings you need, why not go the
full way and just use a file to test it? 

-Scott

On 2010/10/20 19:56:14, sosa wrote:
> What's wrong with mocking out _builtins_.open() and returning a mock
> file?  Is it so bad?
> 
> On Wed, Oct 20, 2010 at 12:19 PM,  <mailto:scottz@chromium.org> wrote:
> > It is a matter of taste so I am LGTMing this but I highly recommend making
> > your
> > function open and close the file. You can test this in your unittest by
> > using
> > tempfiles, which ensures the whole process is tested not just the parsing.
> >
> > Either way in general this update looks really good thanks for cranking this
> > out!
> >
> > http://codereview.chromium.org/3930001/show
> >
>