Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
929353002:
Fix generate_policy_source.py to honour supported_on ranges. (Closed)
|
Created:
5 years, 10 months ago by Thiemo Nagel Modified:
5 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix generate_policy_source.py to honour supported_on ranges.
BUG=451073
Committed: https://crrev.com/87a25e53185cfbd598a888cb35cd674b7d2394de
Cr-Commit-Position: refs/heads/master@{#326848}
Patch Set 1 #Patch Set 2 : Add GN support. #Patch Set 3 : Add some error handling for malformatted supported_on version ranges. #
Total comments: 4
Patch Set 4 : Rebase. #Patch Set 5 : Parse chrome/VERSION in generate_policy_source.py. #
Total comments: 10
Patch Set 6 : Address Mattias' comments. #
Total comments: 6
Patch Set 7 : Address more comments by Mattias. #
Total comments: 2
Patch Set 8 : Address Mattias' final nit. #
Messages
Total messages: 27 (7 generated)
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
tnagel@chromium.org changed reviewers: + mnissler@chromium.org
Hi Mattias, could you please take a look? Thank you! Thiemo
LGTM from my side, but please also get a review from somebody who is competent to review the GN change to determine the version. https://codereview.chromium.org/929353002/diff/80001/components/policy/BUILD.gn File components/policy/BUILD.gn (right): https://codereview.chromium.org/929353002/diff/80001/components/policy/BUILD.... components/policy/BUILD.gn:81: [ chrome_version_abspath ]) I have no idea where this came from, but I assume you copied it from somewhere? If so, can you please CC the respective OWNER/reviewer to perform a sanity check?
tnagel@chromium.org changed reviewers: + brettw@chromium.org
Hi Brett, could you please take a look whether the GN part looks reasonable to you? If you prefer, I could break out the version-reading code to build/config/version.gni or something similar. Thank you and best regards, Thiemo https://codereview.chromium.org/929353002/diff/80001/components/policy/BUILD.gn File components/policy/BUILD.gn (right): https://codereview.chromium.org/929353002/diff/80001/components/policy/BUILD.... components/policy/BUILD.gn:81: [ chrome_version_abspath ]) On 2015/02/18 10:34:42, Mattias Nissler wrote: > I have no idea where this came from, but I assume you copied it from somewhere? > If so, can you please CC the respective OWNER/reviewer to perform a sanity > check? Done.
Friendly ping: Brett, could you please take a look at the GN part or delegate to someone else who can review GN? Thank you! Thiemo
https://codereview.chromium.org/929353002/diff/80001/components/policy/BUILD.gn File components/policy/BUILD.gn (right): https://codereview.chromium.org/929353002/diff/80001/components/policy/BUILD.... components/policy/BUILD.gn:81: [ chrome_version_abspath ]) I have tried very hard not to add this kind of thing. We did it a lot in GYP because everything is so slow you don't notice that everything grinds to a halt while we run python and do a bunch of disk I/O. But this really adds up. For this version thing in particular, the GYP build runs version.py a *lot* of times over and over in the build. We've always been able to avoid doing the same in the GN build. Your use of this is somewhat different than the others. Usually version.py is called to generate some header so my standard instructions won't work. But since this is just being used to call generate_policy_source.py, that script itself can just compute what it needs at build time (rather than GN/GYP time). So your action below can just run version.py, or you can have the script load the VERSION. Either way, be sure to add any files the script depends on (VERSION/version.py or both) to the inputs of the action below.
We should be able to do the same in GYP to keep GYP from getting slower.
Patchset #5 (id:120001) has been deleted
Thanks a lot for your input, Brett! Could you please take another look now that I have moved version number calculation into generate_policy_source.py? Mattias, could you please take a look whether you are still happy with the changes to generate_policy_source.py? https://codereview.chromium.org/929353002/diff/80001/components/policy/BUILD.gn File components/policy/BUILD.gn (right): https://codereview.chromium.org/929353002/diff/80001/components/policy/BUILD.... components/policy/BUILD.gn:81: [ chrome_version_abspath ]) > But since > this is just being used to call generate_policy_source.py, that script itself > can just compute what it needs at build time (rather than GN/GYP time). Done.
https://codereview.chromium.org/929353002/diff/140001/components/policy/tools... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/929353002/diff/140001/components/policy/tools... components/policy/tools/generate_policy_source.py:170: print('Exactly VERSION path, platform, chromium_os flag and ' what is a VERSION path? https://codereview.chromium.org/929353002/diff/140001/components/policy/tools... components/policy/tools/generate_policy_source.py:171: 'input file must be specified as free parameters.') free parameters? as opposed to bound parameters? I think the more common term is "arguments" https://codereview.chromium.org/929353002/diff/140001/components/policy/tools... components/policy/tools/generate_policy_source.py:180: version = None It looks like this stuff should be broken out into a function https://codereview.chromium.org/929353002/diff/140001/components/policy/tools... components/policy/tools/generate_policy_source.py:186: if version == None: nit: if version is None
Thank you for the review! Could you please take another look? https://codereview.chromium.org/929353002/diff/140001/components/policy/tools... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/929353002/diff/140001/components/policy/tools... components/policy/tools/generate_policy_source.py:170: print('Exactly VERSION path, platform, chromium_os flag and ' On 2015/04/23 09:01:29, Mattias Nissler wrote: > what is a VERSION path? Maybe a VERSION file? https://codereview.chromium.org/929353002/diff/140001/components/policy/tools... components/policy/tools/generate_policy_source.py:171: 'input file must be specified as free parameters.') On 2015/04/23 09:01:29, Mattias Nissler wrote: > free parameters? as opposed to bound parameters? I think the more common term is > "arguments" The correct term seems to be "positional parameters" (200k Google results) or "positional arguments" (90k Google results). https://codereview.chromium.org/929353002/diff/140001/components/policy/tools... components/policy/tools/generate_policy_source.py:180: version = None On 2015/04/23 09:01:29, Mattias Nissler wrote: > It looks like this stuff should be broken out into a function Done. https://codereview.chromium.org/929353002/diff/140001/components/policy/tools... components/policy/tools/generate_policy_source.py:186: if version == None: On 2015/04/23 09:01:29, Mattias Nissler wrote: > nit: if version is None Done.
https://codereview.chromium.org/929353002/diff/140001/components/policy/tools... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/929353002/diff/140001/components/policy/tools... components/policy/tools/generate_policy_source.py:170: print('Exactly VERSION path, platform, chromium_os flag and ' On 2015/04/23 09:58:00, Thiemo Nagel wrote: > On 2015/04/23 09:01:29, Mattias Nissler wrote: > > what is a VERSION path? > > Maybe a VERSION file? How about "src/chrome/VERSION file path" ? https://codereview.chromium.org/929353002/diff/160001/components/policy/tools... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/929353002/diff/160001/components/policy/tools... components/policy/tools/generate_policy_source.py:65: def __init__(self, policy, version, os, is_chromium_os): nit: s/version/chrome_major_version/ for clarity. https://codereview.chromium.org/929353002/diff/160001/components/policy/tools... components/policy/tools/generate_policy_source.py:78: major_version = int(version.split('.')[0]) As far as I understand, you're passing the major version, so this is not needed. https://codereview.chromium.org/929353002/diff/160001/components/policy/tools... components/policy/tools/generate_policy_source.py:141: def ParseMajorVersion(version_path): nit: This should be ParseVersionFile as it doesn't parse the version, but the file.
Thank you! Could you please take another look? https://codereview.chromium.org/929353002/diff/140001/components/policy/tools... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/929353002/diff/140001/components/policy/tools... components/policy/tools/generate_policy_source.py:170: print('Exactly VERSION path, platform, chromium_os flag and ' > How about "src/chrome/VERSION file path" ? How about "path to src/chrome/VERSION"? https://codereview.chromium.org/929353002/diff/160001/components/policy/tools... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/929353002/diff/160001/components/policy/tools... components/policy/tools/generate_policy_source.py:65: def __init__(self, policy, version, os, is_chromium_os): On 2015/04/23 10:14:53, Mattias Nissler wrote: > nit: s/version/chrome_major_version/ for clarity. Done. https://codereview.chromium.org/929353002/diff/160001/components/policy/tools... components/policy/tools/generate_policy_source.py:78: major_version = int(version.split('.')[0]) On 2015/04/23 10:14:53, Mattias Nissler wrote: > As far as I understand, you're passing the major version, so this is not needed. You're right. Thanks. https://codereview.chromium.org/929353002/diff/160001/components/policy/tools... components/policy/tools/generate_policy_source.py:141: def ParseMajorVersion(version_path): On 2015/04/23 10:14:53, Mattias Nissler wrote: > nit: This should be ParseVersionFile as it doesn't parse the version, but the > file. Done.
LGTM https://codereview.chromium.org/929353002/diff/180001/components/policy/tools... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/929353002/diff/180001/components/policy/tools... components/policy/tools/generate_policy_source.py:181: print('Exactly path to src/chrome/VERSION, platform, chromium_os flag and ' final nit: This might become more readable if you change it to "Please specify ... as positional parameters."
Thank you! https://codereview.chromium.org/929353002/diff/180001/components/policy/tools... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/929353002/diff/180001/components/policy/tools... components/policy/tools/generate_policy_source.py:181: print('Exactly path to src/chrome/VERSION, platform, chromium_os flag and ' On 2015/04/23 11:39:19, Mattias Nissler wrote: > final nit: This might become more readable if you change it to "Please specify > ... as positional parameters." Sounds good. Done.
On 2015/04/23 11:39:19, Mattias Nissler wrote: > LGTM Does your l-g-t-m cover the now-simplified BUILD.gn as well or shall I ask Brett about it again?
Please ask Brett, I'm not very familiar with GN.
GN stuff LGTM
On 2015/04/24 17:22:48, brettw wrote: > GN stuff LGTM Thanks a lot!
The CQ bit was checked by tnagel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mnissler@chromium.org Link to the patchset: https://codereview.chromium.org/929353002/#ps200001 (title: "Address Mattias' final nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/929353002/200001
Message was sent while issue was closed.
Committed patchset #8 (id:200001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/87a25e53185cfbd598a888cb35cd674b7d2394de Cr-Commit-Position: refs/heads/master@{#326848} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
