Effectively duplicates targets that contain rules/actions, therefore enabling...

Effectively duplicates targets that contain rules/actions, therefore enabling correct dependency processing on windows. 

This patch mimics the changes for XCode.

BUG=42
TEST=test/msvs/prebuild

[jochen (gone - plz use gerrit), 2010-07-03 17:15:38]

please review (and commit if you like it :) )

[jochen (gone - plz use gerrit), 2010-07-03 17:18:46]

On 2010/07/03 17:15:38, jochen wrote:
> please review (and commit if you like it :) )

depends on http://codereview.chromium.org/2815047/show for the chromium side btw

[jochen (gone - plz use gerrit), 2010-07-04 08:36:15]

I changed it to only generate _prebuild projects for those targets that contain
rules or actions with process_outputs_as_sources. I guess then the checkbins
change isn't required (cannot test on the buildbots right now, since the
code.google.com seems to be down)

[jochen (gone - plz use gerrit), 2010-07-04 10:40:56]

On 2010/07/04 08:36:15, jochen wrote:
> I changed it to only generate _prebuild projects for those targets that
contain
> rules or actions with process_outputs_as_sources. I guess then the checkbins
> change isn't required (cannot test on the buildbots right now, since the
> http://code.google.com seems to be down)

turns out that the files checked in the checkbins step have
process_outputs_as_sources set :)

[M-A Ruel, 2010-07-05 13:49:45]

http://codereview.chromium.org/2866042/diff/10001/11001
File pylib/gyp/common.py (right):

http://codereview.chromium.org/2866042/diff/10001/11001#newcode222
pylib/gyp/common.py:222: def DeepDependencyTargets(target_dicts, roots,
dependencies = set()):
You can't do that in python due to default argument reuse side-effects, just do
instead:

dependencies=None):
"""..."""
dependencies = dependencies or set()

http://codereview.chromium.org/2866042/diff/10001/11002
File pylib/gyp/generator/msvs.py (right):

http://codereview.chromium.org/2866042/diff/10001/11002#newcode1200
pylib/gyp/generator/msvs.py:1200: target_dicts[new_target]['target_name'] =
"%s_prebuild"  % (target)
quotes inconsistency. I and google style prefer ' but just look at which is
mostly used.

http://codereview.chromium.org/2866042/diff/10001/11005
File test/msvs/prebuild/prebuild.gyp (right):

http://codereview.chromium.org/2866042/diff/10001/11005#newcode8
test/msvs/prebuild/prebuild.gyp:8: 'target_name': 'prebuild',
It would probably be good to have another project type==none to make sure it
doesn't have a _prebuild.

[jochen (gone - plz use gerrit), 2010-07-05 13:56:27]

all done

[M-A Ruel, 2010-07-05 14:13:39]

Also adding Mark since he's the wizard.

http://codereview.chromium.org/2866042/diff/15001/16002
File pylib/gyp/generator/msvs.py (right):

http://codereview.chromium.org/2866042/diff/15001/16002#newcode1174
pylib/gyp/generator/msvs.py:1174: 
don't put an empty line there

http://codereview.chromium.org/2866042/diff/15001/16002#newcode1189
pylib/gyp/generator/msvs.py:1189: # Create a new target as copy of the current
target.
I wonder if it would be a good idea to put this code in a separate function for
better readability. Not sure, just asking.

http://codereview.chromium.org/2866042/diff/15001/16002#newcode1215
pylib/gyp/generator/msvs.py:1215: new_target_list.append(new_target)
Don't you want to remove the actions and rules from qualified_target? Otherwise
these will be run 2 times?

Also, aren't the intdir and outdir incorrect? I think you need to override these
in target_dicts if they aren't explicitely specified in qualified_target.

[jochen (gone - plz use gerrit), 2010-07-05 14:20:15]

http://codereview.chromium.org/2866042/diff/15001/16002
File pylib/gyp/generator/msvs.py (right):

http://codereview.chromium.org/2866042/diff/15001/16002#newcode1215
pylib/gyp/generator/msvs.py:1215: new_target_list.append(new_target)
On 2010/07/05 14:13:39, Marc-Antoine Ruel wrote:
> Don't you want to remove the actions and rules from qualified_target?
Otherwise
> these will be run 2 times?

removing rules would also require to remove all files matching the rule from
sources.

actions are only executed if the inputs are more current than the outputs (same
for rules).

So in the end, the files are checked twice, but not processed twice.


> 
> Also, aren't the intdir and outdir incorrect? I think you need to override
these
> in target_dicts if they aren't explicitely specified in qualified_target.

Which in/outdirs do you mean exactly?

[M-A Ruel, 2010-07-05 14:28:40]

On 2010/07/05 14:20:15, jochen wrote:
> http://codereview.chromium.org/2866042/diff/15001/16002
> File pylib/gyp/generator/msvs.py (right):
> 
> http://codereview.chromium.org/2866042/diff/15001/16002#newcode1215
> pylib/gyp/generator/msvs.py:1215: new_target_list.append(new_target)
> On 2010/07/05 14:13:39, Marc-Antoine Ruel wrote:
> > Don't you want to remove the actions and rules from qualified_target?
> Otherwise
> > these will be run 2 times?
> 
> removing rules would also require to remove all files matching the rule from
> sources.
> 
> actions are only executed if the inputs are more current than the outputs
(same
> for rules).
> 
> So in the end, the files are checked twice, but not processed twice.

Not necessarily when there is no output file defined. I'm unconfortable with
this since it will be confusing for the devs. It's just asking for trouble if
there is stuble configuration mixup. I really prefer to not duplicate these
rules.


> > Also, aren't the intdir and outdir incorrect? I think you need to override
> these
> > in target_dicts if they aren't explicitely specified in qualified_target.
> 
> Which in/outdirs do you mean exactly?

intermediary and output directories, e.g. src/build/debug/obj and
src/build/debug for src/all.sln case.


Also, I wonder why XCode code path didn't require a change to
DeepDependencyTargets(). Maybe Mark could shed some lights on that.

[jochen (gone - plz use gerrit), 2010-07-05 18:12:02]

On 2010/07/05 14:28:40, Marc-Antoine Ruel wrote:
> On 2010/07/05 14:20:15, jochen wrote:
> > http://codereview.chromium.org/2866042/diff/15001/16002
> > File pylib/gyp/generator/msvs.py (right):
> > 
> > http://codereview.chromium.org/2866042/diff/15001/16002#newcode1215
> > pylib/gyp/generator/msvs.py:1215: new_target_list.append(new_target)
> > On 2010/07/05 14:13:39, Marc-Antoine Ruel wrote:
> > > Don't you want to remove the actions and rules from qualified_target?
> > Otherwise
> > > these will be run 2 times?
> > 
> > removing rules would also require to remove all files matching the rule from
> > sources.
> > 
> > actions are only executed if the inputs are more current than the outputs
> (same
> > for rules).
> > 
> > So in the end, the files are checked twice, but not processed twice.
> 
> Not necessarily when there is no output file defined. I'm unconfortable with
> this since it will be confusing for the devs. It's just asking for trouble if
> there is stuble configuration mixup. I really prefer to not duplicate these
> rules.


I now do not create executables for the prebuild rules. Also, the actions and
rules are deleted from the original targets, and the outputs of rules/actions
with process_outputs_as_sources are added as sources to the original targets.
This requires adopting the IntDir for the prebuild targets.


> 
> 
> > > Also, aren't the intdir and outdir incorrect? I think you need to override
> > these
> > > in target_dicts if they aren't explicitely specified in qualified_target.
> > 
> > Which in/outdirs do you mean exactly?
> 
> intermediary and output directories, e.g. src/build/debug/obj and
> src/build/debug for src/all.sln case.
> 
> 
> Also, I wonder why XCode code path didn't require a change to
> DeepDependencyTargets(). Maybe Mark could shed some lights on that.

Oh, that function was just unnecessarily slow.

[jochen (gone - plz use gerrit), 2010-07-07 17:14:04]

btw, the current state is: it works as expected on vs, but incredibuild chokes
on it..

[Mark Mentovai, 2010-07-07 17:45:28]

http://codereview.chromium.org/2866042/diff/12002/22001
File pylib/gyp/common.py (right):

http://codereview.chromium.org/2866042/diff/12002/22001#newcode222
pylib/gyp/common.py:222: def DeepDependencyTargets(target_dicts, roots,
dependencies=None):
Could you explain what’s going on in this function? It’s heinous enough that an
inline comment (or several) would help.

http://codereview.chromium.org/2866042/diff/12002/22002
File pylib/gyp/generator/msvs.py (right):

http://codereview.chromium.org/2866042/diff/12002/22002#newcode1195
pylib/gyp/generator/msvs.py:1195: default_config['msvs_guid'] =
str(uuid.uuid4()).upper()
I’d prefer if these were deterministic so that they don’t change for no reason
when projects are regenerated.

http://codereview.chromium.org/2866042/diff/12002/22003
File test/msvs/prebuild/base/base.gyp (right):

http://codereview.chromium.org/2866042/diff/12002/22003#newcode9
test/msvs/prebuild/base/base.gyp:9: 'msvs_guid':
'B4D59AE8-8D2F-97E1-A8E9-6D2826729530',
Not needed (throughout this file).

http://codereview.chromium.org/2866042/diff/12002/22005
File test/msvs/prebuild/prebuild.gyp (right):

http://codereview.chromium.org/2866042/diff/12002/22005#newcode1
test/msvs/prebuild/prebuild.gyp:1: # Copyright (c) 2010 Google Inc. All rights
reserved.
Is there anything MSVS-specific about these tests?

http://codereview.chromium.org/2866042/diff/12002/22005#newcode9
test/msvs/prebuild/prebuild.gyp:9: 'msvs_guid':
'B4D59AE8-97E1-A8E9-8D2F-6D2826729530',
Not needed.

[Mark Mentovai, 2010-07-14 17:34:54]

http://codereview.chromium.org/2866042/diff/12002/22002
File pylib/gyp/generator/msvs.py (right):

http://codereview.chromium.org/2866042/diff/12002/22002#newcode1236
pylib/gyp/generator/msvs.py:1236: # process_outputs_as_sources flag set.
When we split the targets up this way on the Mac, we didn’t consider
process_outputs_as_sources. I don’t think we should consider
process_outputs_as_sources here, either. There may be other subtle dependencies
that don’t rely on process_outputs_as_sources—this may be as simple as a target
that lists the generated sources instead of declaring
process_outputs_as_sources, or it may be more complex, involving a step that
only generates .h files.

For reference, the logic used in the Xcode generator is in
pylib/gyp/generator/xcode.py at line 661; the condition used is:

    if type != 'none' and (spec_actions or spec_rules):

spec_actions and spec_rules are defined exactly as they are here. type is
spec['type'].

[Pam (message me for reviews), 2010-07-15 09:19:43]

On 2010/07/07 17:14:04, jochen wrote:
> btw, the current state is: it works as expected on vs, but incredibuild chokes
> on it..

Could you define "chokes" more specifically? Or could you make a sample set of
.sln/.vcproj files this produces available, so I can try to find the IB problem?

- Pam

[jochen (gone - plz use gerrit), 2010-07-15 09:25:44]

On 2010/07/15 09:19:43, Pam wrote:
> On 2010/07/07 17:14:04, jochen wrote:
> > btw, the current state is: it works as expected on vs, but incredibuild
chokes
> > on it..
> 
> Could you define "chokes" more specifically? Or could you make a sample set of
> .sln/.vcproj files this produces available, so I can try to find the IB
problem?
> 
> - Pam

some of the actions/rules are writing to the intermediate directory. on VS this
is translated to $(IntDir). To make the output files of the prebuild target
available to the real target, I define the $(IntDir) of the prebuild target to
be the $(IntDir) of the real target.

IncrediBuild seems to not copy over the $(IntDir) of a build step, at least
that's the error I'm getting, i.e. the real target cannot find any sources in
its $(IntDir).

There are IMO two paths to go from here: one is to come up with a reduced test
case, and ask IB support why it doesn't work. The other is to translate
INTERMEDIATE_DIRECTORY not to $(IntDir) but something else, so incredibuild
cannot know that these are temporary files.

I guess the first would be the better. that would also give us a nicer unit
test.

[jochen (gone - plz use gerrit), 2010-07-28 06:53:46]

closing since this seems not to be required anymore