Generate shim headers for libpng

Generate shim headers for libpng

BUG=165264
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173618

[Ami GONE FROM CHROMIUM, 2012-12-10 22:33:05]

FTR this change (and its predecessor ideas) was discussed in
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/yJ5P0WMRD8k/dis...

FWIW patchset #2 LGTM but that doesn't mean much considering the set of files
touched in this CL :)

[darin (slow to review), 2012-12-17 20:26:49]

I'm really happy with how this allows our .cc files to get simpler.  I'm
probably not the best reviewer of Python code in Chromium though :-(

[Paweł Hajdan Jr., 2012-12-17 20:42:35]

Alright! Mark, could you take a look at the Python parts?

[Mark Mentovai, 2012-12-17 20:48:46]

Remember the last time we looked at a generator like this…

I’m going to need to see all of the output of the Python script, including the
shims that it generates and what it says when you invoke it with the options
that it’s invoked with like --outputs.

[Paweł Hajdan Jr., 2012-12-17 22:37:28]

On 2012/12/17 20:48:46, Mark Mentovai wrote:
> Remember the last time we looked at a generator like this…
> 
> I’m going to need to see all of the output of the Python script, including the
> shims that it generates and what it says when you invoke it with the options
> that it’s invoked with like --outputs.

$ tree out/Debug/obj/gen/shim_headers/
out/Debug/obj/gen/shim_headers/
└── third_party
    └── libpng
        ├── pngconf.h
        └── png.h

2 directories, 2 files
$ cat out/Debug/obj/gen/shim_headers/third_party/libpng/png.h

#if defined(OFFICIAL_BUILD)
#error shim headers must not be used in official builds!
#endif

#include <png.h>
$ python -c 'import generate_shim_headers; print
generate_shim_headers.DoMain(["--headers-root=third_party/libpng",
"--output-directory=out/Debug/obj/gen/shim_headers", "--outputs", "png.h",
"pngconf.h"])'
out/Debug/obj/gen/shim_headers/tools/generate_shim_headers/third_party/libpng
out/Debug/obj/gen/shim_headers/tools/generate_shim_headers/third_party/libpng/png.h
out/Debug/obj/gen/shim_headers/tools/generate_shim_headers/third_party/libpng/pngconf.h

[Paweł Hajdan Jr., 2012-12-17 22:38:38]

On 2012/12/17 22:37:28, Paweł Hajdan Jr. wrote:
> $ python -c 'import generate_shim_headers; print
> generate_shim_headers.DoMain(["--headers-root=third_party/libpng",
> "--output-directory=out/Debug/obj/gen/shim_headers", "--outputs", "png.h",
> "pngconf.h"])'
> out/Debug/obj/gen/shim_headers/tools/generate_shim_headers/third_party/libpng
>
out/Debug/obj/gen/shim_headers/tools/generate_shim_headers/third_party/libpng/png.h
>
out/Debug/obj/gen/shim_headers/tools/generate_shim_headers/third_party/libpng/pngconf.h

Eh, this is messed up because I had to cd to tools/generate_shim_headers. It
works OK when run by gyp.

[Mark Mentovai, 2012-12-17 22:46:31]

What does “works OK” mean? Do you just mean that the paths are right, or do you
mean that the directory isn’t printed.

It’s wrong to list a directory in an “outputs” section.

[Paweł Hajdan Jr., 2012-12-17 22:54:19]

On 2012/12/17 22:46:31, Mark Mentovai wrote:
> What does “works OK” mean? Do you just mean that the paths are right, or
do you
> mean that the directory isn’t printed.

Paths should be OK in that case.

> It’s wrong to list a directory in an “outputs” section.

Oh, fixed that.

[Mark Mentovai, 2012-12-17 23:02:41]

https://codereview.chromium.org/11470020/diff/14001/build/shim_headers.gypi
File build/shim_headers.gypi (right):

https://codereview.chromium.org/11470020/diff/14001/build/shim_headers.gypi#n...
build/shim_headers.gypi:13: 'shim_headers_path':
'<(SHARED_INTERMEDIATE_DIR)/shim_headers',
I don’t think you should use the shared dir. Since you’re going to be pushing
the include dir out to dependents in direct_dependent_settings (below), it’s
fine (safer) to use INTERMEDIATE_DIR.

https://codereview.chromium.org/11470020/diff/14001/build/shim_headers.gypi#n...
build/shim_headers.gypi:16: 'include_dirs+': [
WIth the previous suggestion, this right here might also help flush out targets
that depend on targets without declaring that dependency in GYP. Consider that a
positive.

https://codereview.chromium.org/11470020/diff/14001/tools/generate_shim_heade...
File tools/generate_shim_headers/generate_shim_headers.py (right):

https://codereview.chromium.org/11470020/diff/14001/tools/generate_shim_heade...
tools/generate_shim_headers/generate_shim_headers.py:44:
os.path.join(os.path.dirname(__file__), '..', '..'))
I’m surprised we don’t have a better way to find the source root in Python
already written.

https://codereview.chromium.org/11470020/diff/14001/tools/generate_shim_heade...
tools/generate_shim_headers/generate_shim_headers.py:50: if options.generate and
not os.path.exists(target_directory):
You can pull this outside the loop.

(Either add len(args) >= 1 if you really don’t want to mkdir if there are no
args, or assert earlier in the program that len(args) >= 1.)

[Paweł Hajdan Jr., 2012-12-17 23:18:26]

https://codereview.chromium.org/11470020/diff/14001/build/shim_headers.gypi
File build/shim_headers.gypi (right):

https://codereview.chromium.org/11470020/diff/14001/build/shim_headers.gypi#n...
build/shim_headers.gypi:13: 'shim_headers_path':
'<(SHARED_INTERMEDIATE_DIR)/shim_headers',
On 2012/12/17 23:02:41, Mark Mentovai wrote:
> I don’t think you should use the shared dir. Since you’re going to be pushing
> the include dir out to dependents in direct_dependent_settings (below), it’s
> fine (safer) to use INTERMEDIATE_DIR.

Done.

https://codereview.chromium.org/11470020/diff/14001/tools/generate_shim_heade...
File tools/generate_shim_headers/generate_shim_headers.py (right):

https://codereview.chromium.org/11470020/diff/14001/tools/generate_shim_heade...
tools/generate_shim_headers/generate_shim_headers.py:44:
os.path.join(os.path.dirname(__file__), '..', '..'))
On 2012/12/17 23:02:41, Mark Mentovai wrote:
> I’m surprised we don’t have a better way to find the source root in Python
> already written.

I'd prefer to use that one, to avoid various issues with PYTHONPATH. I can take
a deeper look though if you think I should.

https://codereview.chromium.org/11470020/diff/14001/tools/generate_shim_heade...
tools/generate_shim_headers/generate_shim_headers.py:50: if options.generate and
not os.path.exists(target_directory):
On 2012/12/17 23:02:41, Mark Mentovai wrote:
> You can pull this outside the loop.
> 
> (Either add len(args) >= 1 if you really don’t want to mkdir if there are no
> args, or assert earlier in the program that len(args) >= 1.)

Done.

[Mark Mentovai, 2012-12-17 23:23:24]

https://codereview.chromium.org/11470020/diff/14001/tools/generate_shim_heade...
File tools/generate_shim_headers/generate_shim_headers.py (right):

https://codereview.chromium.org/11470020/diff/14001/tools/generate_shim_heade...
tools/generate_shim_headers/generate_shim_headers.py:44:
os.path.join(os.path.dirname(__file__), '..', '..'))
Paweł Hajdan Jr. wrote:
> On 2012/12/17 23:02:41, Mark Mentovai wrote:
> > I’m surprised we don’t have a better way to find the source root in Python
> > already written.
> 
> I'd prefer to use that one, to avoid various issues with PYTHONPATH. I can
take
> a deeper look though if you think I should.

If there’s something that’s prebaked that we can use here, I think we should.
What’s written here now amounts to a hard-code. Doing something for PYTHONPATH
may also, but the difference is that if this script moves in the source tree,
the hard-code that you’ve written will allow this script to run and silently
produce bad output, and the only build errors will come later in the build,
where they’ll be more difficult to track down to this script. A PYTHONPATH
hard-code, if necessary, at least moves the potential for build errors right
into this script, right at the source of the problem if the script ever moves.

https://codereview.chromium.org/11470020/diff/17001/tools/generate_shim_heade...
File tools/generate_shim_headers/generate_shim_headers.py (right):

https://codereview.chromium.org/11470020/diff/17001/tools/generate_shim_heade...
tools/generate_shim_headers/generate_shim_headers.py:36: options, args =
parser.parse_args(argv)
I still think an assert on len(args) >= 1 is a good idea, unless you have a good
reason not to.

[Paweł Hajdan Jr., 2012-12-17 23:35:48]

https://codereview.chromium.org/11470020/diff/14001/tools/generate_shim_heade...
File tools/generate_shim_headers/generate_shim_headers.py (right):

https://codereview.chromium.org/11470020/diff/14001/tools/generate_shim_heade...
tools/generate_shim_headers/generate_shim_headers.py:44:
os.path.join(os.path.dirname(__file__), '..', '..'))
There are path_utils.py in tools/python/google, but that's equivalent to
dirname(__file__), so would actually make things worse IMHO.

I'd rather leave this as-is. This is not used at all during Google Chrome build,
only custom Chromium builds when -Duse_system_ options are used.

https://codereview.chromium.org/11470020/diff/17001/tools/generate_shim_heade...
File tools/generate_shim_headers/generate_shim_headers.py (right):

https://codereview.chromium.org/11470020/diff/17001/tools/generate_shim_heade...
tools/generate_shim_headers/generate_shim_headers.py:36: options, args =
parser.parse_args(argv)
On 2012/12/17 23:23:24, Mark Mentovai wrote:
> I still think an assert on len(args) >= 1 is a good idea, unless you have a
good
> reason not to.

Done.

[Mark Mentovai, 2012-12-17 23:37:09]

LGTM