New issue
Advanced search Search tips

Issue 787044 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Policy histogram PRESUBMIT broken

Project Member Reported by pmarko@chromium.org, Nov 20 2017

Issue description

The policy histogram PRESUBMIT did not warn me that I forgot a enums.xml value in https://chromium-review.googlesource.com/c/chromium/src/+/758649 patchets 1..3.

Maybe it broke due to the policy_templates.json reorg (list of policies empty?)

See CheckPolicyHistograms in [1].

[1] https://cs.chromium.org/chromium/src/components/policy/resources/PRESUBMIT.py?rcl=8eccc8c7a2c0de051dea4b85309266c3bc7d8ee5&l=113
 

Comment 1 by pmarko@chromium.org, Nov 21 2017

Cc: rsorokin@chromium.org
Components: Enterprise
It seems that the following diff fixes it:
--- a/components/policy/resources/PRESUBMIT.py      
+++ b/components/policy/resources/PRESUBMIT.py
@@ -15,9 +15,9 @@ def _GetPolicyTemplates(template_path):
   # is actually maintained as a python dictionary.
   with open(template_path) as f:
     template_data = eval(f.read(), {})
-  policies = ( policy
+  policies = list(policy
                for policy in template_data['policy_definitions']
-               if policy['type'] != 'group' )
+               if policy['type'] != 'group')
   return policies

 def _CheckPolicyTemplatesSyntax(input_api, output_api):

IIUC, |policies| is currently assigned a "generator". This is then used for two checks (_CheckPolicyTestCases, _CheckPolicyHistograms).
As the generator generates the elements on the fly, it can only be iterated once, so it appears to be empty for _CheckPolicyHistograms.

Possible solutions for root cause:
Use list comprehension - square brackets:
  [policy for policy in .. if ..]
Use a list created from the generator:
  list(policy for policy in .. if ..)

According to [1], in Python 3 both are equivalent; in Python 2 the "policy" iteration variable leaks out of the list comprehension case, so if that is correct I'd lean towards the list thing as we're using Python 2.7.
I'll take a look at what other files use to adhere to their conventions.

Additional TODOs for this bug:
- (Optional) Try to do a diff both-ways in _CheckPolicyHistograms, i.e. also print excess enum values not found in policy_templates.json. _CheckPolicyTestCases does this, and it would have shown us that the PRESUBMIT is broken.
- Go through https://chromium-review.googlesource.com/c/chromium/src/+/657402 and see if the same bug may have sneaked into other .py files

[1] https://stackoverflow.com/questions/47789/generator-expressions-vs-list-comprehension

Comment 2 by pmarko@chromium.org, Nov 21 2017

So, looking through the CL:
- PRESUBMIT (the case above) is using the generator directly (see above)
- components/policy/tools/generate_policy_source.py uses list comprehension  [..]
- components/policy/tools/syntax_check_policy_template_json.py uses dictionary comprehension {..} and list comprehension [..] *
- components/policy/tools/template_writers/policy_template_generator.py uses dictionary comprehension {..} and list comprehension [..]

So I'd suggest we fix this by switching to list comprehension in PRESUBMIT too.


* Technically, this could probably use a generator here, as it's only iterating once :-)
    for group in [policy for policy in policy_definitions
                  if policy['type'] == 'group']:
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d2bc3eaf729a97bab085062151676e5e8a374a3

commit 9d2bc3eaf729a97bab085062151676e5e8a374a3
Author: Pavol Marko <pmarko@chromium.org>
Date: Tue Nov 21 14:09:21 2017

Fix policy histogram PRESUBMIT

Fix the policy histogram PRESUBMIT (it did not report missing enum
values).
Additionaly, warn about extra enum ids.

Example output:
Policy 'MinimumRequiredChromeVersion' (id 394) was added to policy_templates.json but not to src/tools/metrics/histograms/enums.xml. Please update both files. To regenerate the policy part of enums.xml, run:
python tools/metrics/histograms/update_policies.py

Policy id 215 was found in src/tools/metrics/histograms/enums.xml, but no policy with this id exists in policy_templates.json. To regenerate the policy part of enums.xml, run:
python tools/metrics/histograms/update_policies.py

BUG= 787044 

Change-Id: Ib081763c4bf812b77c49ce47735f960a3b9625c7
Reviewed-on: https://chromium-review.googlesource.com/781460
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Roman Sorokin <rsorokin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518240}
[modify] https://crrev.com/9d2bc3eaf729a97bab085062151676e5e8a374a3/components/policy/resources/PRESUBMIT.py

Comment 4 by pmarko@chromium.org, Nov 21 2017

Status: Fixed (was: Assigned)

Sign in to add a comment