This accomplishes a few things:...

- Add a script which:
--- Checks that all source files are in ppapi.gyp and vice-versa
--- Generates test_c_includes.c and test_cc_includes.cc
- Removes tests/test_image_data, which appears to be obsolete

I want to make the script a gyp action, but it didn't work on Windows last I tried.  If desired, I can fix that in this CL, or I can make it automated later.

BUG=63527, 59791, 53451
TEST=None; improves build-time checking


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66833

[David Springer, 2010-11-18 21:21:42]

Some general pythonic gripes.  Also, as you said off-line, some additional
changes are required to make this test actually pass...

http://codereview.chromium.org/5190004/diff/18001/ppapi/generate_ppapi_includ...
File ppapi/generate_ppapi_include_tests.py (right):

http://codereview.chromium.org/5190004/diff/18001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:19: def get_gyp_sources(gyp_file_data):
I recommend taking a look at the Python style guide.  Things like function names
are CamelCase(), two blank lines between functions, 2-space indents (although
your 4-space might be correct...), etc.

http://codereview.chromium.org/5190004/diff/18001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:90: # ppapi/c can be compiled with a C
compiler.
When you generate this, does test_c_includes.c go into the scons-out dir?  Or is
this a script that has to be run by hand to produce files that then get checked
in?

http://codereview.chromium.org/5190004/diff/18001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:109: if __name__ == "__main__":
From looking around at other .py files, it seems that our standard practice is
to write a main(argv) routine, and simply call it from here.  Something like:

if __name__ == '__main__':
  sys.exit(main(sys.argv[:1]))

This allows people to import your script as a package and call, say,
generate_ppapi_include_tests.main()

http://codereview.chromium.org/5190004/diff/18001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:116: # Generate test_c_includes.c, which
is a test to ensure that all
This comment seems to be spurious - or the file got cut off.

[dmichael(do not use this one), 2010-11-18 21:51:29]

On 2010/11/18 21:21:42, David Springer wrote:
> Some general pythonic gripes.  Also, as you said off-line, some additional
> changes are required to make this test actually pass...
> 
I fixed the include problem that the test uncovered...  see c/pp_stdint.h.

[dmichael(do not use this one), 2010-11-18 21:51:35]

http://codereview.chromium.org/5190004/diff/18001/ppapi/generate_ppapi_includ...
File ppapi/generate_ppapi_include_tests.py (right):

http://codereview.chromium.org/5190004/diff/18001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:19: def get_gyp_sources(gyp_file_data):
On 2010/11/18 21:21:42, David Springer wrote:
> I recommend taking a look at the Python style guide.  Things like function
names
> are CamelCase(),
Looks like lowercase/underscores: 
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Naming
 two blank lines between functions,
I don't see reference to that.  There is an example of one blank line between
functions:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Properties
> 2-space indents (although
> your 4-space might be correct...):
4-space is correct: 
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Indentation, etc.

So...  I looked through the guide and made a few tweaks, and I think I'm now
consistent with the style guide.

http://codereview.chromium.org/5190004/diff/18001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:90: # ppapi/c can be compiled with a C
compiler.
On 2010/11/18 21:21:42, David Springer wrote:
> When you generate this, does test_c_includes.c go into the scons-out dir?  Or
is
> this a script that has to be run by hand to produce files that then get
checked
> in?
Currently it just goes in "tests" alongside non-generated tests, and the tests
get checked in.  It's currently a manual step.  I want to make it a gyp action
eventually.  I updated the comments in the script to mention that.

http://codereview.chromium.org/5190004/diff/18001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:109: if __name__ == "__main__":
On 2010/11/18 21:21:42, David Springer wrote:
> From looking around at other .py files, it seems that our standard practice is
> to write a main(argv) routine, and simply call it from here.  Something like:
> 
> if __name__ == '__main__':
>   sys.exit(main(sys.argv[:1]))
> 
> This allows people to import your script as a package and call, say,
> generate_ppapi_include_tests.main()
> 
Done.

http://codereview.chromium.org/5190004/diff/18001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:116: # Generate test_c_includes.c, which
is a test to ensure that all
On 2010/11/18 21:21:42, David Springer wrote:
> This comment seems to be spurious - or the file got cut off.
Oops, Done.

[dmichael(do not use this one), 2010-11-18 22:22:50]

+Darin, in case he cares.  I'm just trying to put some added mistake-proofing,
although it's still slightly manual.

[neb, 2010-11-18 22:26:41]

ACK

[David Springer, 2010-11-19 00:14:21]

More pythonic nits.  Also, it might be better to delete test_image_data in a
separate CL, while will make it slightly easier to revert them if necessary.

http://codereview.chromium.org/5190004/diff/18001/ppapi/generate_ppapi_includ...
File ppapi/generate_ppapi_include_tests.py (right):

http://codereview.chromium.org/5190004/diff/18001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:19: def get_gyp_sources(gyp_file_data):
On 2010/11/18 21:51:35, dmichael wrote:
> On 2010/11/18 21:21:42, David Springer wrote:
> > I recommend taking a look at the Python style guide.  Things like function
> names
> > are CamelCase(),
> Looks like lowercase/underscores: 
> http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Naming
>  two blank lines between functions,
> I don't see reference to that.  There is an example of one blank line between
> functions:
> http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Properties
> > 2-space indents (although
> > your 4-space might be correct...):
> 4-space is correct: 
> http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Indentation,
etc.
> 
> So...  I looked through the guide and made a few tweaks, and I think I'm now
> consistent with the style guide.

Awesome - it turns out that the internal style guide is totally different
(http://www.corp.google.com/eng/doc/pyguide.xml).  Not sure how to call this one
- Since we are open source, I think we should follow the external guide... 
However, all of the other python scripts I've seen follow the internal guide...

http://codereview.chromium.org/5190004/diff/42001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:27: # Search the local directory and all
its subdirectories for source files.
Are these comments relevant in this file?

http://codereview.chromium.org/5190004/diff/42001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:33: adjusted_root = root[2:]
Consider using globals for these strings (e.g. TARGETS_KEY = 'targets')

http://codereview.chromium.org/5190004/diff/42001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:121: 
Consider passing in argv, this will make it more "general".

http://codereview.chromium.org/5190004/diff/42001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:122: 
"ppapi.gyp" should either be a constant (e.g., PPAPI_GYP_FILE) or (better) a
command-line opt with default value of "ppapi.gyp".  For an example of how to
process cmd-line args, see
nativeclinet-sdk/src/build_tools/install_gtest/intall_gtest.py.

http://codereview.chromium.org/5190004/diff/42001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:130: 
main(sysargv[1:])

http://codereview.chromium.org/5190004/diff/42001/ppapi/tests/test_image_data...
File ppapi/tests/test_image_data/test_image_data.cc (left):

http://codereview.chromium.org/5190004/diff/42001/ppapi/tests/test_image_data...
ppapi/tests/test_image_data/test_image_data.cc:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
Is the removal of these files necessary for this CL , or should they get removed
as part of another one?

[David Springer, 2010-11-19 00:16:40]

Follow-on that I missed earlier... sorry.

http://codereview.chromium.org/5190004/diff/42001/ppapi/generate_ppapi_includ...
File ppapi/generate_ppapi_include_tests.py (right):

http://codereview.chromium.org/5190004/diff/42001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:45: adjusted_root = root[2:]
This is a little magical - can you explain why you peel off the first two
elements of |root| in a comment?

[neb, 2010-11-19 18:17:52]

Can you close/split this one?

[dmichael(do not use this one), 2010-11-19 18:53:22]

Fixed up the python script and removed stuff that got split in to
http://codereview.chromium.org/5233002/

http://codereview.chromium.org/5190004/diff/42001/ppapi/generate_ppapi_includ...
File ppapi/generate_ppapi_include_tests.py (right):

http://codereview.chromium.org/5190004/diff/42001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:1: #!/usr/bin/python
Regarding style:
Figures, I picked the wrongest of the 3 options.  It would appear that Chromium
uses PEP-8, _except_ that indenting is 2-space and function & method names are
CamelCase:
http://www.chromium.org/developers/coding-style
Go figure.

Hopefully I got everything up to snuff.

http://codereview.chromium.org/5190004/diff/42001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:27: # TODO(brettw): Put proxy in the build
when it's ready.
On 2010/11/19 00:14:21, David Springer wrote:
> Are these comments relevant in this file?
Yes.  I added another comment to try to clear it up;  let me know if it's still
unclear.

http://codereview.chromium.org/5190004/diff/42001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:33: for target in
gyp_file_data['targets']:
On 2010/11/19 00:14:21, David Springer wrote:
> Consider using globals for these strings (e.g. TARGETS_KEY = 'targets')
Done.

http://codereview.chromium.org/5190004/diff/42001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:45: adjusted_root = root[2:]
On 2010/11/19 00:16:40, David Springer wrote:
> This is a little magical - can you explain why you peel off the first two
> elements of |root| in a comment?
I reworked it to be a little more general & self-documenting.  Let me know if
it's still unclear.

http://codereview.chromium.org/5190004/diff/42001/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:121: def main():
On 2010/11/19 00:14:21, David Springer wrote:
> Consider passing in argv, this will make it more "general".
This script makes other assumptions that might not be true for other
directories/gyp files.  If it ever needs to be made more general, I'd like to
hold off until then before adding unneeded complexity.

http://codereview.chromium.org/5190004/diff/42001/ppapi/tests/test_image_data...
File ppapi/tests/test_image_data/test_image_data.cc (left):

http://codereview.chromium.org/5190004/diff/42001/ppapi/tests/test_image_data...
ppapi/tests/test_image_data/test_image_data.cc:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2010/11/19 00:14:21, David Springer wrote:
> Is the removal of these files necessary for this CL , or should they get
removed
> as part of another one?
It's not strictly necessary, since the script is not part of a gyp action (yet).
 But these files make the script spit out an error, because
tests/test_image_data/test_image_data.cc is NOT in the gyp file.  It appears
that it was moved directly under tests, and that this file is woefully out of
date.  It no longer compiles.  I'm pretty sure deleting it in this CL is the
right thing to do.

[David Springer, 2010-11-19 20:48:19]

Two minor python nits.

o/w, LGTM.

http://codereview.chromium.org/5190004/diff/14004/ppapi/generate_ppapi_includ...
File ppapi/generate_ppapi_include_tests.py (right):

http://codereview.chromium.org/5190004/diff/14004/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:153: ppapi_gyp_file_name = 'ppapi.gyp'
Nit: maybe make this a global const at the top fo the file?

http://codereview.chromium.org/5190004/diff/14004/ppapi/generate_ppapi_includ...
ppapi/generate_ppapi_include_tests.py:163: sys.exit(main())
Nit: 2-space indent.

[dmichael(do not use this one), 2010-11-19 21:08:44]

My latest try didn't update the CL.  FYI, here are the real trybot results:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[neb, 2010-11-19 22:44:59]

ACK again