Policy histogram PRESUBMIT broken |
||
Issue descriptionThe 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
,
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']:
,
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
,
Nov 21 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by pmarko@chromium.org
, Nov 21 2017Components: 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