Fixing msvs handling of actions which have overlapping inputs....

pylib/gyp/generator/msvs.py

Index: pylib/gyp/generator/msvs.py
===================================================================
--- pylib/gyp/generator/msvs.py	(revision 879)
+++ pylib/gyp/generator/msvs.py	(working copy)
@@ -288,22 +288,53 @@
                            has_input_path, quote_cmd)
 
 
-def _PickPrimaryInput(inputs):
-  # Pick second input as the primary one, unless there's only one.
-  # TODO(bradnelson): this is a bit of a hack,
-  # find something more general.
-  if len(inputs) > 1:
-    return inputs[1]
-  else:
-    return inputs[0]
+def _AddActionStep(actions_dict, inputs, outputs, description, command):
+  """Merge action into an existing list of actions.
 
+  Care must be taken so that actions which have overlapping inputs either don't
+  get assigned to the same input, or get collapsed into one.
 
-def _AddCustomBuildToolForMSVS(p, spec, inputs, outputs, description, cmd):
+  Arguments:
+    actions_dict: dictionary keyed on input name, which maps to a list of
+      dicts describing the actions attached to that input file.
+    inputs: list of inputs
+    outputs: list of outputs
+    description: description of the action
+    command: command line to execute
+  """
+  # Require there to be at least one input (call sites will ensure this).
+  assert inputs
+
+  action = {
+      'inputs': inputs,
+      'outputs': outputs,
+      'description': description,
+      'command': command,
+  }
+
+  # Pick where to stick this action. Pick the slot with the least actions
+  # currently.
+  chosen_input = None
+  for i in inputs:
+    if (chosen_input is None or
+        len(actions_dict.get(i, [])) < len(actions_dict.get(chosen_input, []))):
+      chosen_input = i
+  assert chosen_input is not None
+

[jeanluc1, 2011/02/01 02:00:08]

You could have done:

chosen_input = reduce(lambda x,y:len(actions_dict[x]) < len(actions_dict[y]) and
x or y, actions_dict)

+  # Add it there.
+  if chosen_input not in actions_dict:
+    actions_dict[chosen_input] = []
+  actions_dict[chosen_input].append(action)
+
+
+def _AddCustomBuildToolForMSVS(p, spec, primary_input,
+                               inputs, outputs, description, cmd):
   """Add a custom build tool to execute something.
 
   Arguments:
     p: the target project
     spec: the target project dict
+    primary_input: input file to attach the build tool to
     inputs: list of inputs
     outputs: list of outputs
     description: description of the action
@@ -318,13 +349,44 @@
       'Outputs': ';'.join(outputs),
       'CommandLine': cmd,
       })
-  primary_input = _PickPrimaryInput(inputs)
   # Add to the properties of primary input for each config.
   for config_name, c_data in spec['configurations'].iteritems():
-    p.AddFileConfig(primary_input,
+    p.AddFileConfig(_FixPath(primary_input),
                     _ConfigFullName(config_name, c_data), tools=[tool])
 
 
+def _AddAccumulatedActions(p, spec, actions_dict):
+  """Add actions accumulated into an actions_dict, merging as needed.
+
+  Arguments:
+    p: the target project
+    spec: the target project dict
+    actions_dict: dictionary keyed on input name, which maps to a list of
+      dicts describing the actions attached to that input file.

[jeanluc1, 2011/02/01 02:00:08]

two more spaces?

+  """
+  for input in actions_dict:
+    inputs = set()
+    outputs = set()
+    description = ''
+    command = ''
+    for action in actions_dict[input]:
+      inputs.update(set(action['inputs']))
+      outputs.update(set(action['outputs']))
+      if description:
+        description += ', and also '
+      description += action['description']

[jeanluc1, 2011/02/01 02:00:08]

Another option is to pass a list for description and command, and inside of
_addCustom do a ', and also'.join().

[bradn, 2011/02/01 02:55:52]

Done.

+      if command:
+        command += ' && '
+      command += action['command']
+    # Add the custom build step for one input file.
+    _AddCustomBuildToolForMSVS(p, spec,
+                               primary_input=input,
+                               inputs=inputs,
+                               outputs=outputs,
+                               description=description,
+                               cmd=command)
+
+
 def _RuleExpandPath(path, input_file):
   """Given the input file to which a rule applied, string substitute a path.
 
@@ -488,12 +550,10 @@
   #                   the primary input.
   all_inputs = list(all_inputs)
   all_inputs.insert(1, filename)
-  actions_to_add.append({
-      'inputs': [_FixPath(i) for i in all_inputs],
-      'outputs': [_FixPath(i) for i in all_outputs],
-      'description': 'Running %s' % cmd,
-      'cmd': cmd,
-      })
+  _AddActionStep(inputs=[_FixPath(i) for i in all_inputs],

[jeanluc1, 2011/02/01 02:00:08]

I would expect the first argument to be actions_to_add or something similar.

[bradn, 2011/02/01 02:55:52]

Done.

+                 outputs=[_FixPath(i) for i in all_outputs],
+                 description='Running %s' % cmd,
+                 command=cmd)
 
 
 def _EscapeEnvironmentVariableExpansion(s):
@@ -596,8 +656,8 @@
     _GenerateExternalRules(rules_external, output_dir, spec,
                            sources, options, actions_to_add)
   _AdjustSourcesForRules(rules, sources, excluded_sources)
-  
-  
+
+
 def _AdjustSourcesForRules(rules, sources, excluded_sources):
   # Add outputs generated by each rule (if applicable).
   for rule in rules:
@@ -682,31 +742,27 @@
   sources, excluded_sources = _PrepareListOfSources(spec, project.build_file)
 
   # Add rules.
-  actions_to_add = []
+  actions_to_add = {}
   _GenerateRulesForMSVS(p, gyp_dir, options, spec,
                         sources, excluded_sources,
                         actions_to_add)
-  sources, excluded_sources, excluded_idl = _AdjustSourcesAndConverToFilterHierarchy(spec, options,
-      gyp_dir, sources, excluded_sources)
+  sources, excluded_sources, excluded_idl = (
+      _AdjustSourcesAndConverToFilterHierarchy(
+          spec, options, gyp_dir, sources, excluded_sources))
 
   # Add in files.
   p.AddFiles(sources)
 
-  # Add deferred actions to add.
-  for a in actions_to_add:
-    _AddCustomBuildToolForMSVS(p, spec,
-                               inputs=a['inputs'],
-                               outputs=a['outputs'],
-                               description=a['description'],
-                               cmd=a['cmd'])
-
   _ExcludeFilesFromBeingBuilt(p, spec, excluded_sources, excluded_idl)
   _AddToolFilesToMSVS(p, spec)
   _HandlePreCompileHeaderStubs(p, spec)
-  _AddActionsToMSVS(p, spec)
+  _AddActionsToMSVS(actions_to_add, spec, project.build_file)
+  _AddCopiesForMSVS(actions_to_add, spec)
   _WriteMSVSUserFile(project.path, version, spec)
-  _AddCopiesForMSVS(p, spec)
 
+  # Do this after all actions have been decided.
+  _AddAccumulatedActions(p, spec, actions_to_add)
+
   # Write it out.
   p.Write()
 
@@ -1036,23 +1092,14 @@
   _AddNormalizedSources(sources, spec.get('sources', []))
   excluded_sources = set()
   # Add in the gyp file.
-  gyp_file = os.path.split(build_file)[1]
+  gyp_file = posixpath.split(build_file)[1]

[jeanluc1, 2011/02/01 02:00:08]

Why posixpath?

[bradn, 2011/02/01 02:55:52]

Previously the case where there are no inputs below didn't actually create an
action. So this was wrong, and never caught. At this point the build file is
still in /s.

   sources.add(_NormalizedSource(gyp_file))
   # Add in 'action' inputs and outputs.
   for a in spec.get('actions', []):
-    inputs = a.get('inputs')
-    if not inputs:
-      # This is an action with no inputs.  Make the primary input
-      # be the .gyp file itself so Visual Studio has a place to
-      # hang the custom build rule.
-      inputs = [gyp_file]
-      a['inputs'] = inputs
+    inputs = a.get('inputs', [])
     inputs = [_NormalizedSource(i) for i in inputs]
-    primary_input = _PickPrimaryInput(inputs)
     inputs = set(inputs)
     sources.update(inputs)
-    inputs.remove(primary_input)
-    excluded_sources.update(inputs)
     if int(a.get('process_outputs_as_sources', False)):
       _AddNormalizedSources(sources, a.get('outputs', []))
   # Add in 'copies' inputs and outputs.
@@ -1182,16 +1229,21 @@
                       {}, tools=[tool])
 
 
-def _AddActionsToMSVS(p, spec):
+def _AddActionsToMSVS(actions_to_add, spec, gyp_file):
   # Add actions.
   actions = spec.get('actions', [])
   for a in actions:
     cmd = _PrepareAction(spec, a, has_input_path=False)
-    _AddCustomBuildToolForMSVS(p, spec,
-                               inputs=a.get('inputs', []),
-                               outputs=a.get('outputs', []),
-                               description=a.get('message', a['action_name']),
-                               cmd=cmd)
+    # Attach actions to the gyp file if nothing else is there.

[jeanluc1, 2011/02/01 02:00:08]

When would this happen?  What does it mean?

[bradn, 2011/02/01 02:55:52]

We have a few test cases where actions have no inputs (though technically they
should be on the script that's running).
It's not ideal that this is even supported, but it works currently, and one of
the actions tests fails without it.
This is the complement of the bit that adds the gyp file in around 1099.

Actually the more interesting question is why the null_input test in subdir3
passed before on windows.
I'm looking into that now.

+    inputs = a.get('inputs')
+    if not inputs:
+      inputs = [gyp_file]

[jeanluc1, 2011/02/01 02:00:08]

Could have been:
inputs = a.get('inputs') or [gyp_file]

[bradn, 2011/02/01 02:55:52]

Duh...
Done.

+    # Add the action.
+    _AddActionStep(actions_to_add,
+                   inputs=inputs,
+                   outputs=a.get('outputs', []),
+                   description=a.get('message', a['action_name']),
+                   command=cmd)
 
 
 def _WriteMSVSUserFile(project_path, version, spec):
@@ -1215,11 +1267,11 @@
   user_file.Write()
 
 
-def _AddCopiesForMSVS(p, spec):
+def _AddCopiesForMSVS(actions_to_add, spec):
   copies = _GetCopies(spec)
   for inputs, outputs, cmd, description in copies:
-    _AddCustomBuildToolForMSVS(p, spec, inputs=inputs, outputs=outputs,
-                               description=description, cmd=cmd)
+    _AddActionStep(actions_to_add, inputs=inputs, outputs=outputs,
+                   description=description, command=cmd)
 
 
 def _GetCopies(spec):
@@ -1436,7 +1488,7 @@
   # Figure out all the projects that will be generated and their guids
   project_objects = _CreateProjectObjects(target_list, target_dicts, options,
                                           msvs_version)
-  
+
   # Generate each project.
   for project in project_objects.values():
     fixpath_prefix = project.fixpath_prefix