ninja windows: Support magic idl build rules

ninja windows: Support magic idl build rules

Magically build .idl files via midl compiler if there's no other explicit
build rule specified. Sort of 'works', but there's no edge between the
generated .h file and .cc files that might include it, so parallelism will
break it.

Committed: https://code.google.com/p/gyp/source/detail?r=1308

[scottmg, 2012-04-05 01:10:10]

This is a not-working version of .idl builds. It's currently just processing
.idl files as another type of 'sources' (which is where they are in gyp files).
The problem is that the .idl build generates a header, which then might be
included by any random .cc file in the same 'sources' block. (Other than the
missing dependency the rest of the gunk seems to be correct.)

Do you have any thoughts on the best way to resolve the dependency problem? I
was thinking I would need to move (all) the .idl builds to a separate pre-target
and then have the real target depend on that one. (sort of what precompile_stamp
is for maybe?)

[scottmg, 2012-04-05 01:21:16]

On 2012/04/05 01:10:10, scottmg wrote:
> This is a not-working version of .idl builds. It's currently just processing
> .idl files as another type of 'sources' (which is where they are in gyp
files).
> The problem is that the .idl build generates a header, which then might be
> included by any random .cc file in the same 'sources' block. (Other than the
> missing dependency the rest of the gunk seems to be correct.)
> 
> Do you have any thoughts on the best way to resolve the dependency problem? I
> was thinking I would need to move (all) the .idl builds to a separate
pre-target
> and then have the real target depend on that one. (sort of what
precompile_stamp
> is for maybe?)

Hmm, it looks like it would make more sense to fit in with the handling of
actions/rules/copies, and it looks like making that a pre-step is already
handled. (I think?)

[scottmg, 2012-04-05 02:54:56]

On 2012/04/05 01:21:16, scottmg wrote:
> On 2012/04/05 01:10:10, scottmg wrote:
> > This is a not-working version of .idl builds. It's currently just processing
> > .idl files as another type of 'sources' (which is where they are in gyp
> files).
> > The problem is that the .idl build generates a header, which then might be
> > included by any random .cc file in the same 'sources' block. (Other than the
> > missing dependency the rest of the gunk seems to be correct.)
> > 
> > Do you have any thoughts on the best way to resolve the dependency problem?
I
> > was thinking I would need to move (all) the .idl builds to a separate
> pre-target
> > and then have the real target depend on that one. (sort of what
> precompile_stamp
> > is for maybe?)
> 
> Hmm, it looks like it would make more sense to fit in with the handling of
> actions/rules/copies, and it looks like making that a pre-step is already
> handled. (I think?)

Continuing my monologue, this turned out to be relatively straightforward and
seems to work pretty well.

[Nico, 2012-04-05 04:37:11]

Nice :-)

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:388: """Visual Studio has implicit .idl build rules
for MIDL files, but other
I'd make the docstring just """Writes rules for MSVC's implicit idl rules."""
and put the rest in a normal comment.

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:391: explicit rules."""
This function is fairly long and branchy. I find functions more readable when
all the things they do are at about the same level of abstraction, this is
currently not quite true for this one.

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:397: return []
Maybe this could be

  if self.msvs_settings.HasExplicitIdlRules(spec.get('rules', [])):
    return []

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:399: for source in spec['sources']:
Maybe

for source in filter(lambda x: x.endswith('.idl'), spec['sources']):

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:440: outputs += self._HandleIdlFiles(spec,
prebuild)
For consistency with the rest of this function, WriteWinIdlFiles(). Only call if
flavor == win

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/msvs_emulatio...
File pylib/gyp/msvs_emulation.py (right):

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/msvs_emulatio...
pylib/gyp/msvs_emulation.py:133: '$(IntDir)': '$!INTERMEDIATE_DIR',
Does msvc differentiate between intermediate_dir (per target) and
shared_intermediate_dir (shared between all targets)? is IntDir the per-target
one?

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/msvs_emulatio...
pylib/gyp/msvs_emulation.py:344: outdir = midl('OutputDirectory')
It looks like this gets ConvertVSMacros()d here already, iirc you did that a
second time after calling this function over in ninja.py. Probably enough to do
that once?

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/win_tool.py
File pylib/gyp/win_tool.py (right):

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/win_tool.py#n...
pylib/gyp/win_tool.py:79: popen = subprocess.Popen(args,
nit: if you break after (, the rest might all fit on one line

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/win_tool.py#n...
pylib/gyp/win_tool.py:90: processings = [os.path.basename(x) for x in lines if
x.startswith(prefix)]
:-D

Do wrap this in a set(), just in case the output gets long. (The os x terminal
has a O(n^2) in line length when you paste something in, and i hate that. that's
probably there because someone thought 'eh, n is usually small' too.)

[scottmg, 2012-04-05 17:52:42]

thanks!

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:388: """Visual Studio has implicit .idl build rules
for MIDL files, but other
On 2012/04/05 04:37:11, Nico wrote:
> I'd make the docstring just """Writes rules for MSVC's implicit idl rules."""
> and put the rest in a normal comment.

Done.

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:391: explicit rules."""
On 2012/04/05 04:37:11, Nico wrote:
> This function is fairly long and branchy. I find functions more readable when
> all the things they do are at about the same level of abstraction, this is
> currently not quite true for this one.

True, improved-ish?

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:397: return []
On 2012/04/05 04:37:11, Nico wrote:
> Maybe this could be
> 
>   if self.msvs_settings.HasExplicitIdlRules(spec.get('rules', [])):
>     return []

Done.

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:399: for source in spec['sources']:
On 2012/04/05 04:37:11, Nico wrote:
> Maybe
> 
> for source in filter(lambda x: x.endswith('.idl'), spec['sources']):

Done.

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:440: outputs += self._HandleIdlFiles(spec,
prebuild)
On 2012/04/05 04:37:11, Nico wrote:
> For consistency with the rest of this function, WriteWinIdlFiles(). Only call
if
> flavor == win

Done.

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/msvs_emulatio...
File pylib/gyp/msvs_emulation.py (right):

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/msvs_emulatio...
pylib/gyp/msvs_emulation.py:133: '$(IntDir)': '$!INTERMEDIATE_DIR',
On 2012/04/05 04:37:11, Nico wrote:
> Does msvc differentiate between intermediate_dir (per target) and
> shared_intermediate_dir (shared between all targets)? is IntDir the per-target
> one?

Hmm, good question. It looks like $(IntDir) is the per project one, and
shared_intermediate_dir would be faked with $(OutDir)\obj\shared or similar. So
I believe this is correct.

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/msvs_emulatio...
pylib/gyp/msvs_emulation.py:344: outdir = midl('OutputDirectory')
On 2012/04/05 04:37:11, Nico wrote:
> It looks like this gets ConvertVSMacros()d here already, iirc you did that a
> second time after calling this function over in ninja.py. Probably enough to
do
> that once?

Done.

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/win_tool.py
File pylib/gyp/win_tool.py (right):

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/win_tool.py#n...
pylib/gyp/win_tool.py:79: popen = subprocess.Popen(args,
On 2012/04/05 04:37:11, Nico wrote:
> nit: if you break after (, the rest might all fit on one line

Done. (and above)

http://chromiumcodereview.appspot.com/9990006/diff/17/pylib/gyp/win_tool.py#n...
pylib/gyp/win_tool.py:90: processings = [os.path.basename(x) for x in lines if
x.startswith(prefix)]
On 2012/04/05 04:37:11, Nico wrote:
> :-D
> 
> Do wrap this in a set(), just in case the output gets long. (The os x terminal
> has a O(n^2) in line length when you paste something in, and i hate that.
that's
> probably there because someone thought 'eh, n is usually small' too.)

Done.

[Nico, 2012-04-05 18:02:24]

lgtm

https://chromiumcodereview.appspot.com/9990006/diff/10001/pylib/gyp/generator...
File pylib/gyp/generator/ninja.py (right):

https://chromiumcodereview.appspot.com/9990006/diff/10001/pylib/gyp/generator...
pylib/gyp/generator/ninja.py:389: with files that are generated."""
assert self.flavor == 'win'

it would be nice if more of this could be in msvs_emulation, but i can't think
of a great way to split it up. (since msvs_emulation is only used by ninja, my
concern is mostly keeping win-only things out of this file as much as possible.)

https://chromiumcodereview.appspot.com/9990006/diff/10001/pylib/gyp/msvs_emul...
File pylib/gyp/msvs_emulation.py (right):

https://chromiumcodereview.appspot.com/9990006/diff/10001/pylib/gyp/msvs_emul...
pylib/gyp/msvs_emulation.py:359: # TODO(scottmg): Non-default configuration
maybe?
This comment is not understandable for people who aren't you. Maybe insert a few
more words to explain what you mean :-)

https://chromiumcodereview.appspot.com/9990006/diff/11008/test/win/idl-rules/...
File test/win/idl-rules/history_indexer.idl (right):

https://chromiumcodereview.appspot.com/9990006/diff/11008/test/win/idl-rules/...
test/win/idl-rules/history_indexer.idl:5: import "oaidl.idl";
these are system idl files?

https://chromiumcodereview.appspot.com/9990006/diff/11008/test/win/idl-rules/...
test/win/idl-rules/history_indexer.idl:15: interface IChromeHistoryIndexer :
IUnknown {
Can you simplify this file a bit? (remove comments, remove most of the
parameters, maybe shorten function names)? I suppose most of the things in here
aren't important for this test.

[scottmg, 2012-04-05 18:14:25]

http://codereview.chromium.org/9990006/diff/10001/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

http://codereview.chromium.org/9990006/diff/10001/pylib/gyp/generator/ninja.p...
pylib/gyp/generator/ninja.py:389: with files that are generated."""
On 2012/04/05 18:02:24, Nico wrote:
> assert self.flavor == 'win'

Done.

> it would be nice if more of this could be in msvs_emulation, but i can't think
> of a great way to split it up. (since msvs_emulation is only used by ninja, my
> concern is mostly keeping win-only things out of this file as much as
possible.)

Yeah. I had it in there wholesale before, but it seemed to have to pass in too
much (functions, etc.) so felt not usefully separated.

I believe there will be something similar required for .rc files so I will keep
it in mind for refactoring when I get there.

http://codereview.chromium.org/9990006/diff/10001/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

http://codereview.chromium.org/9990006/diff/10001/pylib/gyp/msvs_emulation.py...
pylib/gyp/msvs_emulation.py:359: # TODO(scottmg): Non-default configuration
maybe?
On 2012/04/05 18:02:24, Nico wrote:
> This comment is not understandable for people who aren't you. Maybe insert a
few
> more words to explain what you mean :-)

Done.

http://codereview.chromium.org/9990006/diff/11008/test/win/idl-rules/history_...
File test/win/idl-rules/history_indexer.idl (right):

http://codereview.chromium.org/9990006/diff/11008/test/win/idl-rules/history_...
test/win/idl-rules/history_indexer.idl:5: import "oaidl.idl";
On 2012/04/05 18:02:24, Nico wrote:
> these are system idl files?

Yes.

http://codereview.chromium.org/9990006/diff/11008/test/win/idl-rules/history_...
test/win/idl-rules/history_indexer.idl:15: interface IChromeHistoryIndexer :
IUnknown {
On 2012/04/05 18:02:24, Nico wrote:
> Can you simplify this file a bit? (remove comments, remove most of the
> parameters, maybe shorten function names)? I suppose most of the things in
here
> aren't important for this test.

Done.