Change actions.txt to actions.xml

tools/metrics/actions/PRESUBMIT.py

 # Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-"""
+"""Presubmit script for actions.xml.
+
 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
 for more details on the presubmit API built into gcl.
 """
 
 
 def CheckChange(input_api, output_api):
-  """Checks that histograms.xml is pretty-printed and well-formatted."""
+  """Checks that actions.xml is up to date and pretty-printed."""

[Ilya Sherman, 2014/02/22 07:07:37]

This PRESUBMIT script only checks that actions.xml is up-to-date if the CL
already includes changes to that file.  What are we doing to make sure that
people don't forget to update the file when they add actions?

Note that today, I can forget to add my actions to the file, and the next person
to come along can do it for me.  Presumably this won't work once we start
requiring descriptions and owners for actions.  Moreover, a previous developer
having forgotten to update actions.xml can cause pain for the next developer who
adds actions.  Am I misunderstanding something?

[Alexei Svitkine (slow), 2014/02/24 18:32:35]

I agree that it's a valid point and it would be good for a general presubmit to
detect when these are changed without the actions XML file being changed - but
I'm not sure it's in scope for this change. We already have this problem for
histograms, for example.

Perhaps we can address this by more communication / dev education - e.g. send an
email to chromium-dev when this lands.

(The reason I'm not just advocating for just doing it in a presubmit for all
code is that I'm worried how much time this would add to all developers'
workflows - for example I've seen output saying that some check for some NS
stuff took many seconds to run. I don't want us to add more overhead like that.)

[Ilya Sherman, 2014/02/24 18:48:06]

For histograms.xml, we don't have any automated script to add histograms to the
XML file.  That results in many histograms not being mapped (:sadface:), but
also means that if I'm adding a new histogram to the file, the only diffs I see
are related to my changes.

With actions.txt, we had a script that would just automatically add the mapping
for you.  Often people would forget to run it; but the next person to run it
would automatically fix that up.  It created a bit of confusion for people to
see unrelated changes in their uploaded actions.txt files, but it was easy to
convince folks that this was ok.

Now, forgotten metrics will each show up in later reviews as a huge block of
text asking for information from the current developer.  We can still tell
people that this is ok, but I suspect it's going to cause a lot more confusion
that the status quo does.

I agree that we shouldn't slow down presubmit times globally.  Maybe it would
not be too slow to run the crawler over just the files that were actually
modified?  If so, we might even be able to add a similar crawler + PRESUBMIT for
histograms, which would be a big win IMO.

   for f in input_api.AffectedTextFiles():
     p = f.AbsoluteLocalPath()
-    if (input_api.basename(p) == 'histograms.xml'
+    if (input_api.basename(p) == 'actions.xml'
         and input_api.os_path.dirname(p) == input_api.PresubmitLocalPath()):
       cwd = input_api.os_path.dirname(p)
       exit_code = input_api.subprocess.call(
-          ['python', 'pretty_print.py', '--presubmit'], cwd=cwd)
+          ['python', 'extract_actions.py', '--presubmit'], cwd=cwd)
       if exit_code != 0:
         return [output_api.PresubmitError(
-            'histograms.xml is not formatted correctly; run pretty_print.py '
-            'to fix')]
-
-      exit_code = input_api.subprocess.call(
-          ['python', 'validate_format.py'], cwd=cwd)
-      if exit_code != 0:
-        return [output_api.PresubmitError(
-            'histograms.xml is not well formatted; run validate_format.py '
-            'and fix the reported errors')]
+            'actions.xml is not up to date or is not formatted correctly; '
+            'run extract_actions.py to fix')]

[Ilya Sherman, 2014/02/22 07:07:37]

Please separate out the cases of not up-to-date vs. not formatted correctly.

[yao, 2014/02/24 19:16:23]

It's not trivial to differentiate between 'not up to date' and 'not formatted
correctly' based on the code now.
Given the fact that running extract_actions.py fixes both problem together, is
it necessary to differentiate them? 

   return []
 
 
 def CheckChangeOnUpload(input_api, output_api):
   return CheckChange(input_api, output_api)
 
 
 def CheckChangeOnCommit(input_api, output_api):
   return CheckChange(input_api, output_api)