Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2175413004:
Enable whitelist generation for all builds. (Closed)
|
Created:
4 years, 4 months ago by estevenson Modified:
4 years, 4 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. |
DescriptionEnable whitelist generation for official builds.
Currently, all resources are included in PAK files when Chrome is
built locally. Only official_buildbot.sh uses a resource whitelist. This CL
enables local builds to use resource whitelisting by setting the
enable_resource_whitelist_generation gn flag to true, or by building an
official build.
This will allow developers to more easily monitor the changes in APK size
for each commit they make.
However, a large amount of output is generated (_pragma is used to
create warnings to allow whitelisted resources to be listed), so for now
the whitelist will only be generated for official builds.
Comparison of ChromePublic.apk_InstallSize APK size using resource_sizes.py:
* Local build with this patch: 73338275 bytes
* Local build without this patch: 75248223 bytes
BUG=632385
Committed: https://crrev.com/3f3fa5fc415baab5a875839e6520c2cbb245ec38
Cr-Commit-Position: refs/heads/master@{#409511}
Patch Set 1 #
Total comments: 59
Patch Set 2 : Addressed agrieve's comments and removed stderr filtering. #
Total comments: 18
Patch Set 3 : Enable whitelist generation for official builds. #
Total comments: 4
Patch Set 4 : Enable whitelist generation for official builds. #
Total comments: 2
Patch Set 5 : Enable whitelist generation for official builds. #Patch Set 6 : rebase + change output dir #
Messages
Total messages: 30 (11 generated)
estevenson@chromium.org changed reviewers: + agrieve@chromium.org
https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_ar_wrap... File build/toolchain/gcc_ar_wrapper.py (right): https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_ar_wrap... build/toolchain/gcc_ar_wrapper.py:2: # Copyright 2016 The Chromium Authors. All rights reserved. These actually shouldn't be updated. They are meant to reflect the earliest copyright on the file. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_ar_wrap... build/toolchain/gcc_ar_wrapper.py:39: help='Response file', nit: response file is already passed as a positional arg, so probably no need to pass it explicitly here. E.g.: rsp_files = [a for a in args.inputs if a.startswith('@')] https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_ar_wrap... build/toolchain/gcc_ar_wrapper.py:42: help='Indicates that whitelisting should be performed.') It's probably not obvious to most what "whitelisting" refers to. How about: help='Merge all resource whitelists into a single whitelist' https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_ar_wrap... build/toolchain/gcc_ar_wrapper.py:46: whitelist_file = '%s.whitelist' % args.output nit: For compiler wrapper you pass in the path to the output. Probably makes sense to do so here as well. May also make sense to have --resource-whitelist=PATH in place of a boolean flag. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_ar_wrap... build/toolchain/gcc_ar_wrapper.py:51: nit: revert https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_compile... File build/toolchain/gcc_compile_wrapper.py (right): https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_compile... build/toolchain/gcc_compile_wrapper.py:32: if returncode != 0 or not args.whitelist: Need to write stderr before returning. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_compile... build/toolchain/gcc_compile_wrapper.py:40: if used_resources: If a file used to have a resource, and is changed to not have one, this needs to delete the existing whitelist file (or replace it with an empty file) https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_toolcha... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:18: _whitelist_flag = "--whitelist" nit: move this to be closer to where it is used. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:239: outputs = [ nit: add a comment here that ninja doesn't support multiple outputs here. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:244: command = "$python_path \"$compile_wrapper\" $_whitelist_flag --whitelist-file={{output}}.whitelist $command" nit: this will result in a double space when whitelist isn't enabled. It won't hurt anything, but will be seen a lot an will raise eyebrows, so best to avoid it. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... File build/toolchain/wrapper_utils.py (right): https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:12: PRAGMA_WARNING = '[-Wunknown-pragmas]' nit: Prefix with _ https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:39: def CombineWhitelists(rspfile, outfile): nit: CombineWhitelists -> CombineResourceWhitelists https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:47: tokens = f.read().split() nit: split won't properly handle filenames with spaces in them. Should use shlex.split instead. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:48: whitelists = ['%s.whitelist' % token for token in tokens nit: unindent https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:60: def GetResourceIdsInPragmaWarnings(text): nit: never prefix a non-trivial function with "Get". In this case, maybe go for "Extract" https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:71: unknown_pragma_warning_pattern = re.compile( nit: Make this a constant at the top of the file. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:94: # TODO(estevenson): Find a cleaner, less error prone way to do this. Yep, ideally, this should be combined with the above function since they both need to extract the warnings. Also - need to be able to support both GCC and clang. This function is probably worth of having a unittests that includes example compiler output. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:106: def FilterUnknownPragmas(stderr): Note: We only want to filter warnings that are cause by whitelist pragmas. If there were another unknown pragma, we'd want to print it. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:118: def GetCommandOutput(args, cwd=None, env=None): nit: remove optional params since they are not used. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:130: stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd, env=env) There's no need to pipe stdout since we're not filtering it. I think omitting it causes the parent stdout to be inherited, so it should "just work" https://codereview.chromium.org/2175413004/diff/1/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2175413004/diff/1/chrome/BUILD.gn#newcode1476 chrome/BUILD.gn:1476: repack_whitelist = "$root_out_dir/resource_whitelist.txt" nit: Make a constant for this since it's used in multiple places. Also nice to not put it in the root out dir: android_resource_whitelist = "$target_gen_dir/resource_whitelist.txt" https://codereview.chromium.org/2175413004/diff/1/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2175413004/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:267: action("resource_whitelist") { okay, I lied, I think this should go in //chrome/BUILD.gn since that's the only place it's used, and then you can share the outfile constant. https://codereview.chromium.org/2175413004/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:273: infile = "$root_out_dir/libchrome.cr.so.whitelist" nit: infile -> _infile https://codereview.chromium.org/2175413004/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:278: outfile = "$root_out_dir/resource_whitelist.txt" outfile->_outfile https://codereview.chromium.org/2175413004/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:285: rebase_path(infile), nit: for no good reason, convention is to use build-relative paths rather than absolute ones by passing root_build_dir as the second arg here. https://codereview.chromium.org/2175413004/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:289: rebase_path(root_build_dir), You can just hardcode this as: --out-dir=. https://codereview.chromium.org/2175413004/diff/1/tools/grit/grit_rule.gni File tools/grit/grit_rule.gni (right): https://codereview.chromium.org/2175413004/diff/1/tools/grit/grit_rule.gni#ne... tools/grit/grit_rule.gni:88: enable_resource_whitelist_generation = false Looks like this used to work for non-android targets, but we're now breaking it (by removing repack_whitelist param and not adding toolchain filtering to windows). I think breaking it is fine since no other platforms use it, but maybe let's add an: if (!is_android && enable_resource_whitelist_generation) { print("Warning: GN logic for resource whitelists is implemented only for Android") } https://codereview.chromium.org/2175413004/diff/1/tools/resources/generate_re... File tools/resources/generate_resource_whitelist.py (right): https://codereview.chromium.org/2175413004/diff/1/tools/resources/generate_re... tools/resources/generate_resource_whitelist.py:22: gyp/gn variable enable_resource_whitelist_generation set to 1/true. nit: delete gyp mentions here. https://codereview.chromium.org/2175413004/diff/1/tools/resources/generate_re... tools/resources/generate_resource_whitelist.py:29: On Windows, the message is simply a message via __pragma(message(...)). woohoo! This prompted me to uncover this: http://stackoverflow.com/questions/9695212/gcc-pragma-message-ignored Looks like #pragma message has been added to gcc/clang since this script was created. I think we can actually delete the unknown-pragmas logic in favour of always use #pragma message. I tested this with the versions of gcc / clang that we use and found they both output to stderr: clang: ../../base/lazy_instance.cc:5:9: warning: hello world [-W#pragma-messages] #pragma message "hello world " ^ 1 warning generated. gcc: ../../base/lazy_instance.cc:5:17: note: #pragma message: hello world #pragma message "hello world " ^ This makes the parsing a bit easier (always 3 lines each), but the "1 warning generated" for clang might cause issues. Perhaps we should count the number of whitelist entries, and then stript that line if it ends with "$num_whitelist_messages warnings? generated" https://codereview.chromium.org/2175413004/diff/1/tools/resources/generate_re... tools/resources/generate_resource_whitelist.py:62: help='The build log to read (default stdin)') nit: update
Description was changed from ========== Enable whitelist generation for all builds. Currently, all resources are included in PAK files when Chrome is built locally. Only official builds use a resource whitelist. This CL enables local builds to use resource whitelisting by setting the enable_resource_whitelist_generation gn flag to true. This will allow developers to more easily monitor the changes in APK size for each commit they make. Comparison of ChromePublic.apk_InstallSize APK size using resource_sizes.py: * Local build with this patch: 73338275 bytes * Local build without this patch: 75248223 bytes Current known issues: * Need to suppress some build output * Need to refactor stderr filtering BUG= ========== to ========== Enable whitelist generation for official builds. Currently, all resources are included in PAK files when Chrome is built locally. Only official_buildbot.sh uses a resource whitelist. This CL enables local builds to use resource whitelisting by setting the enable_resource_whitelist_generation gn flag to true, or by building an official build. This will allow developers to more easily monitor the changes in APK size for each commit they make. However, a large amount of output is generated (_pragma is used to create warnings to allow whitelisted resources to be listed), so for now the whitelist will only be generated for official builds. Comparison of ChromePublic.apk_InstallSize APK size using resource_sizes.py: * Local build with this patch: 73338275 bytes * Local build without this patch: 75248223 bytes BUG=632385 ==========
https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_ar_wrap... File build/toolchain/gcc_ar_wrapper.py (right): https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_ar_wrap... build/toolchain/gcc_ar_wrapper.py:2: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/07/27 at 01:42:27, agrieve wrote: > These actually shouldn't be updated. They are meant to reflect the earliest copyright on the file. Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_ar_wrap... build/toolchain/gcc_ar_wrapper.py:39: help='Response file', On 2016/07/27 at 01:42:27, agrieve wrote: > nit: response file is already passed as a positional arg, so probably no need to pass it explicitly here. > > E.g.: rsp_files = [a for a in args.inputs if a.startswith('@')] Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_ar_wrap... build/toolchain/gcc_ar_wrapper.py:42: help='Indicates that whitelisting should be performed.') On 2016/07/27 at 01:42:27, agrieve wrote: > It's probably not obvious to most what "whitelisting" refers to. How about: > > help='Merge all resource whitelists into a single whitelist' Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_ar_wrap... build/toolchain/gcc_ar_wrapper.py:46: whitelist_file = '%s.whitelist' % args.output On 2016/07/27 at 01:42:27, agrieve wrote: > nit: For compiler wrapper you pass in the path to the output. Probably makes sense to do so here as well. May also make sense to have --resource-whitelist=PATH in place of a boolean flag. Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_ar_wrap... build/toolchain/gcc_ar_wrapper.py:51: On 2016/07/27 at 01:42:27, agrieve wrote: > nit: revert Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_compile... File build/toolchain/gcc_compile_wrapper.py (right): https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_compile... build/toolchain/gcc_compile_wrapper.py:32: if returncode != 0 or not args.whitelist: On 2016/07/27 at 01:42:27, agrieve wrote: > Need to write stderr before returning. Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_compile... build/toolchain/gcc_compile_wrapper.py:40: if used_resources: On 2016/07/27 at 01:42:27, agrieve wrote: > If a file used to have a resource, and is changed to not have one, this needs to delete the existing whitelist file (or replace it with an empty file) Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_toolcha... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:18: _whitelist_flag = "--whitelist" On 2016/07/27 at 01:42:27, agrieve wrote: > nit: move this to be closer to where it is used. Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:239: outputs = [ On 2016/07/27 at 01:42:27, agrieve wrote: > nit: add a comment here that ninja doesn't support multiple outputs here. Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:244: command = "$python_path \"$compile_wrapper\" $_whitelist_flag --whitelist-file={{output}}.whitelist $command" On 2016/07/27 at 01:42:27, agrieve wrote: > nit: this will result in a double space when whitelist isn't enabled. It won't hurt anything, but will be seen a lot an will raise eyebrows, so best to avoid it. Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... File build/toolchain/wrapper_utils.py (right): https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:12: PRAGMA_WARNING = '[-Wunknown-pragmas]' On 2016/07/27 at 01:42:27, agrieve wrote: > nit: Prefix with _ Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:39: def CombineWhitelists(rspfile, outfile): On 2016/07/27 at 01:42:28, agrieve wrote: > nit: CombineWhitelists -> CombineResourceWhitelists Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:47: tokens = f.read().split() On 2016/07/27 at 01:42:27, agrieve wrote: > nit: split won't properly handle filenames with spaces in them. Should use shlex.split instead. Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:48: whitelists = ['%s.whitelist' % token for token in tokens On 2016/07/27 at 01:42:27, agrieve wrote: > nit: unindent Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:60: def GetResourceIdsInPragmaWarnings(text): On 2016/07/27 at 01:42:27, agrieve wrote: > nit: never prefix a non-trivial function with "Get". In this case, maybe go for "Extract" Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:71: unknown_pragma_warning_pattern = re.compile( On 2016/07/27 at 01:42:27, agrieve wrote: > nit: Make this a constant at the top of the file. Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:94: # TODO(estevenson): Find a cleaner, less error prone way to do this. On 2016/07/27 01:42:27, agrieve wrote: > Yep, ideally, this should be combined with the above function since they both > need to extract the warnings. > > Also - need to be able to support both GCC and clang. This function is probably > worth of having a unittests that includes example compiler output. We discussed this, there doesn't seem to be a clean/reliable way to do this so we will leave the output in for now. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:106: def FilterUnknownPragmas(stderr): On 2016/07/27 01:42:27, agrieve wrote: > Note: We only want to filter warnings that are cause by whitelist pragmas. If > there were another unknown pragma, we'd want to print it. Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:118: def GetCommandOutput(args, cwd=None, env=None): On 2016/07/27 at 01:42:28, agrieve wrote: > nit: remove optional params since they are not used. Done. https://codereview.chromium.org/2175413004/diff/1/build/toolchain/wrapper_uti... build/toolchain/wrapper_utils.py:130: stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd, env=env) On 2016/07/27 at 01:42:28, agrieve wrote: > There's no need to pipe stdout since we're not filtering it. I think omitting it causes the parent stdout to be inherited, so it should "just work" Done. https://codereview.chromium.org/2175413004/diff/1/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2175413004/diff/1/chrome/BUILD.gn#newcode1476 chrome/BUILD.gn:1476: repack_whitelist = "$root_out_dir/resource_whitelist.txt" On 2016/07/27 at 01:42:28, agrieve wrote: > nit: Make a constant for this since it's used in multiple places. Also nice to not put it in the root out dir: > > android_resource_whitelist = "$target_gen_dir/resource_whitelist.txt" Done. https://codereview.chromium.org/2175413004/diff/1/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2175413004/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:267: action("resource_whitelist") { On 2016/07/27 at 01:42:28, agrieve wrote: > okay, I lied, I think this should go in //chrome/BUILD.gn since that's the only place it's used, and then you can share the outfile constant. Done. https://codereview.chromium.org/2175413004/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:273: infile = "$root_out_dir/libchrome.cr.so.whitelist" On 2016/07/27 at 01:42:28, agrieve wrote: > nit: infile -> _infile Done. https://codereview.chromium.org/2175413004/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:278: outfile = "$root_out_dir/resource_whitelist.txt" On 2016/07/27 at 01:42:28, agrieve wrote: > outfile->_outfile Done. https://codereview.chromium.org/2175413004/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:285: rebase_path(infile), On 2016/07/27 at 01:42:28, agrieve wrote: > nit: for no good reason, convention is to use build-relative paths rather than absolute ones by passing root_build_dir as the second arg here. Done. https://codereview.chromium.org/2175413004/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:289: rebase_path(root_build_dir), On 2016/07/27 at 01:42:28, agrieve wrote: > You can just hardcode this as: --out-dir=. Done. https://codereview.chromium.org/2175413004/diff/1/tools/grit/grit_rule.gni File tools/grit/grit_rule.gni (right): https://codereview.chromium.org/2175413004/diff/1/tools/grit/grit_rule.gni#ne... tools/grit/grit_rule.gni:88: enable_resource_whitelist_generation = false On 2016/07/27 at 01:42:28, agrieve wrote: > Looks like this used to work for non-android targets, but we're now breaking it (by removing repack_whitelist param and not adding toolchain filtering to windows). > > I think breaking it is fine since no other platforms use it, but maybe let's add an: > > if (!is_android && enable_resource_whitelist_generation) { > print("Warning: GN logic for resource whitelists is implemented only for Android") > } Done. https://codereview.chromium.org/2175413004/diff/1/tools/resources/generate_re... File tools/resources/generate_resource_whitelist.py (right): https://codereview.chromium.org/2175413004/diff/1/tools/resources/generate_re... tools/resources/generate_resource_whitelist.py:22: gyp/gn variable enable_resource_whitelist_generation set to 1/true. On 2016/07/27 at 01:42:28, agrieve wrote: > nit: delete gyp mentions here. Done. https://codereview.chromium.org/2175413004/diff/1/tools/resources/generate_re... tools/resources/generate_resource_whitelist.py:29: On Windows, the message is simply a message via __pragma(message(...)). On 2016/07/27 at 01:42:28, agrieve wrote: > woohoo! This prompted me to uncover this: > http://stackoverflow.com/questions/9695212/gcc-pragma-message-ignored > > Looks like #pragma message has been added to gcc/clang since this script was created. I think we can actually delete the unknown-pragmas logic in favour of always use #pragma message. > > I tested this with the versions of gcc / clang that we use and found they both output to stderr: > > clang: > ../../base/lazy_instance.cc:5:9: warning: hello world [-W#pragma-messages] > #pragma message "hello world " > ^ > 1 warning generated. > > gcc: > ../../base/lazy_instance.cc:5:17: note: #pragma message: hello world > #pragma message "hello world " > ^ > > > This makes the parsing a bit easier (always 3 lines each), but the "1 warning generated" for clang might cause issues. Perhaps we should count the number of whitelist entries, and then stript that line if it ends with "$num_whitelist_messages warnings? generated" We discussed this and it seems that this doesn't work with gcc.
just nits really. Looking good! https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_com... File build/toolchain/gcc_compile_wrapper.py (right): https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_com... build/toolchain/gcc_compile_wrapper.py:34: with open(args.resource_whitelist, 'w') as f: resource_whitelist is optional, so should do this only when it's provided. https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_too... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_too... build/toolchain/gcc_toolchain.gni:243: command = "$python_path \"$compile_wrapper\" --resource-whitelist=\"$whitelistfile\" $command" For all of these commands, we should not pass --resource-whitelist unless enable_resource_whitelist_generation == true. One way to fix the spacing would be to do: whitelist_flag = " " if (enable_resource_whitelist_generation) { whitelist_flage = " --resource-whitelist=\"{{output}}.whitelist\"" } command = "$python_path \"$compile_wrapper\"$whitelist_flag $command" https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_too... build/toolchain/gcc_toolchain.gni:309: whitelistfile = "$sofile.whitelist" nit: whitelistfile -> whitelist_file https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_too... build/toolchain/gcc_toolchain.gni:367: whitelistfile, Should not list this in outputs unless enable_resource_whitelist_generation https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/wrapper... File build/toolchain/wrapper_utils.py (right): https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/wrapper... build/toolchain/wrapper_utils.py:15: 'whitelisted_resource_(?P<resource_id>[0-9]+)') nit: fits on prev line? https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/wrapper... build/toolchain/wrapper_utils.py:66: open(outfile, 'w').close() missing return. Although I think it's probably better to just do nothing when whitelist=False. No need to clear the file in this case. E.g. in the normal dev case, this will cause a bunch of unused empty files to be created. Might confuse. https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/wrapper... build/toolchain/wrapper_utils.py:68: whitelists = ['%s.whitelist' % candidate for candidate in whitelist_candidates nit: better to use a generator if you're just going to iterate through it next (parans rather than []s) https://codereview.chromium.org/2175413004/diff/20001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2175413004/diff/20001/chrome/BUILD.gn#newcode... chrome/BUILD.gn:1779: action("resource_whitelist") { You should guard this target behind: if (is_android && enable_resource_whitelist_generation) This is probably what's causing a gn gen failure. https://codereview.chromium.org/2175413004/diff/20001/tools/grit/repack.gni File tools/grit/repack.gni (right): https://codereview.chromium.org/2175413004/diff/20001/tools/grit/repack.gni#n... tools/grit/repack.gni:41: args += [ "--suppress-removed-key-output" ] nit: the print will never happen unless repack_whitelist is set, so probably makes sense to put this within the next if block.
https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_com... File build/toolchain/gcc_compile_wrapper.py (right): https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_com... build/toolchain/gcc_compile_wrapper.py:34: with open(args.resource_whitelist, 'w') as f: On 2016/07/29 01:55:21, agrieve wrote: > resource_whitelist is optional, so should do this only when it's provided. Done. https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_too... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_too... build/toolchain/gcc_toolchain.gni:243: command = "$python_path \"$compile_wrapper\" --resource-whitelist=\"$whitelistfile\" $command" On 2016/07/29 01:55:21, agrieve wrote: > For all of these commands, we should not pass --resource-whitelist unless > enable_resource_whitelist_generation == true. > > One way to fix the spacing would be to do: > > whitelist_flag = " " > if (enable_resource_whitelist_generation) { > whitelist_flage = " --resource-whitelist=\"{{output}}.whitelist\"" > } > > command = "$python_path \"$compile_wrapper\"$whitelist_flag $command" Done. https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_too... build/toolchain/gcc_toolchain.gni:309: whitelistfile = "$sofile.whitelist" On 2016/07/29 01:55:21, agrieve wrote: > nit: whitelistfile -> whitelist_file Done. https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_too... build/toolchain/gcc_toolchain.gni:367: whitelistfile, On 2016/07/29 01:55:21, agrieve wrote: > Should not list this in outputs unless enable_resource_whitelist_generation Done. https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/wrapper... File build/toolchain/wrapper_utils.py (right): https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/wrapper... build/toolchain/wrapper_utils.py:15: 'whitelisted_resource_(?P<resource_id>[0-9]+)') On 2016/07/29 01:55:21, agrieve wrote: > nit: fits on prev line? Done. https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/wrapper... build/toolchain/wrapper_utils.py:66: open(outfile, 'w').close() On 2016/07/29 01:55:21, agrieve wrote: > missing return. Although I think it's probably better to just do nothing when > whitelist=False. No need to clear the file in this case. E.g. in the normal dev > case, this will cause a bunch of unused empty files to be created. Might > confuse. > Done. https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/wrapper... build/toolchain/wrapper_utils.py:68: whitelists = ['%s.whitelist' % candidate for candidate in whitelist_candidates On 2016/07/29 01:55:21, agrieve wrote: > nit: better to use a generator if you're just going to iterate through it next > (parans rather than []s) Done. https://codereview.chromium.org/2175413004/diff/20001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2175413004/diff/20001/chrome/BUILD.gn#newcode... chrome/BUILD.gn:1779: action("resource_whitelist") { On 2016/07/29 01:55:21, agrieve wrote: > You should guard this target behind: if (is_android && > enable_resource_whitelist_generation) > > This is probably what's causing a gn gen failure. Done. https://codereview.chromium.org/2175413004/diff/20001/tools/grit/repack.gni File tools/grit/repack.gni (right): https://codereview.chromium.org/2175413004/diff/20001/tools/grit/repack.gni#n... tools/grit/repack.gni:41: args += [ "--suppress-removed-key-output" ] On 2016/07/29 01:55:21, agrieve wrote: > nit: the print will never happen unless repack_whitelist is set, so probably > makes sense to put this within the next if block. Done.
On 2016/07/29 15:33:57, estevenson1 wrote: > https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_com... > File build/toolchain/gcc_compile_wrapper.py (right): > > https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_com... > build/toolchain/gcc_compile_wrapper.py:34: with open(args.resource_whitelist, > 'w') as f: > On 2016/07/29 01:55:21, agrieve wrote: > > resource_whitelist is optional, so should do this only when it's provided. > > Done. > > https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_too... > File build/toolchain/gcc_toolchain.gni (right): > > https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_too... > build/toolchain/gcc_toolchain.gni:243: command = "$python_path > \"$compile_wrapper\" --resource-whitelist=\"$whitelistfile\" $command" > On 2016/07/29 01:55:21, agrieve wrote: > > For all of these commands, we should not pass --resource-whitelist unless > > enable_resource_whitelist_generation == true. > > > > One way to fix the spacing would be to do: > > > > whitelist_flag = " " > > if (enable_resource_whitelist_generation) { > > whitelist_flage = " --resource-whitelist=\"{{output}}.whitelist\"" > > } > > > > command = "$python_path \"$compile_wrapper\"$whitelist_flag $command" > > Done. > > https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_too... > build/toolchain/gcc_toolchain.gni:309: whitelistfile = "$sofile.whitelist" > On 2016/07/29 01:55:21, agrieve wrote: > > nit: whitelistfile -> whitelist_file > > Done. > > https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/gcc_too... > build/toolchain/gcc_toolchain.gni:367: whitelistfile, > On 2016/07/29 01:55:21, agrieve wrote: > > Should not list this in outputs unless enable_resource_whitelist_generation > > Done. > > https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/wrapper... > File build/toolchain/wrapper_utils.py (right): > > https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/wrapper... > build/toolchain/wrapper_utils.py:15: > 'whitelisted_resource_(?P<resource_id>[0-9]+)') > On 2016/07/29 01:55:21, agrieve wrote: > > nit: fits on prev line? > > Done. > > https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/wrapper... > build/toolchain/wrapper_utils.py:66: open(outfile, 'w').close() > On 2016/07/29 01:55:21, agrieve wrote: > > missing return. Although I think it's probably better to just do nothing when > > whitelist=False. No need to clear the file in this case. E.g. in the normal > dev > > case, this will cause a bunch of unused empty files to be created. Might > > confuse. > > > > Done. > > https://codereview.chromium.org/2175413004/diff/20001/build/toolchain/wrapper... > build/toolchain/wrapper_utils.py:68: whitelists = ['%s.whitelist' % candidate > for candidate in whitelist_candidates > On 2016/07/29 01:55:21, agrieve wrote: > > nit: better to use a generator if you're just going to iterate through it next > > (parans rather than []s) > > Done. > > https://codereview.chromium.org/2175413004/diff/20001/chrome/BUILD.gn > File chrome/BUILD.gn (right): > > https://codereview.chromium.org/2175413004/diff/20001/chrome/BUILD.gn#newcode... > chrome/BUILD.gn:1779: action("resource_whitelist") { > On 2016/07/29 01:55:21, agrieve wrote: > > You should guard this target behind: if (is_android && > > enable_resource_whitelist_generation) > > > > This is probably what's causing a gn gen failure. > > Done. > > https://codereview.chromium.org/2175413004/diff/20001/tools/grit/repack.gni > File tools/grit/repack.gni (right): > > https://codereview.chromium.org/2175413004/diff/20001/tools/grit/repack.gni#n... > tools/grit/repack.gni:41: args += [ "--suppress-removed-key-output" ] > On 2016/07/29 01:55:21, agrieve wrote: > > nit: the print will never happen unless repack_whitelist is set, so probably > > makes sense to put this within the next if block. > > Done. lgtm
agrieve@chromium.org changed reviewers: + dpranke@chromium.org
Dirk - please take a look. Some alternatives discussed in the attached bug, and I think this is the least bad :S.
The CQ bit was checked by estevenson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2175413004/diff/40001/tools/grit/grit_rule.gni File tools/grit/grit_rule.gni (right): https://codereview.chromium.org/2175413004/diff/40001/tools/grit/grit_rule.gn... tools/grit/grit_rule.gni:89: enable_resource_whitelist_generation = is_official_build This should default to (is_android && is_offical_build) (i.e., we don't want this to be true on other platforms). https://codereview.chromium.org/2175413004/diff/40001/tools/grit/grit_rule.gn... tools/grit/grit_rule.gni:93: } Can you make this: assert(!enable_resource_whitelist_generation || is_android, "resource whitelist generation only works on android") ? If you do that, then you can get rid of the if (is_android && ) part of all of your other checks.
https://codereview.chromium.org/2175413004/diff/40001/tools/grit/grit_rule.gni File tools/grit/grit_rule.gni (right): https://codereview.chromium.org/2175413004/diff/40001/tools/grit/grit_rule.gn... tools/grit/grit_rule.gni:89: enable_resource_whitelist_generation = is_official_build On 2016/07/30 00:14:51, Dirk Pranke wrote: > This should default to (is_android && is_offical_build) (i.e., we don't want > this to be true on other platforms). Done. https://codereview.chromium.org/2175413004/diff/40001/tools/grit/grit_rule.gn... tools/grit/grit_rule.gni:93: } On 2016/07/30 00:14:51, Dirk Pranke wrote: > Can you make this: > > assert(!enable_resource_whitelist_generation || is_android, "resource whitelist > generation only works on android") > > ? > > If you do that, then you can get rid of the if (is_android && ) part of all of > your other checks. Done.
lgtm w/ nit. https://codereview.chromium.org/2175413004/diff/60001/tools/grit/grit_rule.gni File tools/grit/grit_rule.gni (right): https://codereview.chromium.org/2175413004/diff/60001/tools/grit/grit_rule.gn... tools/grit/grit_rule.gni:91: "resource whitelist generation only works on android") The assert should be outside of the declare_args() block, to ensure that whatever is set in args.gn is picked up properly.
https://codereview.chromium.org/2175413004/diff/60001/tools/grit/grit_rule.gni File tools/grit/grit_rule.gni (right): https://codereview.chromium.org/2175413004/diff/60001/tools/grit/grit_rule.gn... tools/grit/grit_rule.gni:91: "resource whitelist generation only works on android") On 2016/08/03 00:59:07, Dirk Pranke wrote: > The assert should be outside of the declare_args() block, to ensure that > whatever is set in args.gn is picked up properly. Done.
The CQ bit was checked by estevenson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org, dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2175413004/#ps80001 (title: "Enable whitelist generation for official builds.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Enable whitelist generation for official builds. Currently, all resources are included in PAK files when Chrome is built locally. Only official_buildbot.sh uses a resource whitelist. This CL enables local builds to use resource whitelisting by setting the enable_resource_whitelist_generation gn flag to true, or by building an official build. This will allow developers to more easily monitor the changes in APK size for each commit they make. However, a large amount of output is generated (_pragma is used to create warnings to allow whitelisted resources to be listed), so for now the whitelist will only be generated for official builds. Comparison of ChromePublic.apk_InstallSize APK size using resource_sizes.py: * Local build with this patch: 73338275 bytes * Local build without this patch: 75248223 bytes BUG=632385 ========== to ========== Enable whitelist generation for official builds. Currently, all resources are included in PAK files when Chrome is built locally. Only official_buildbot.sh uses a resource whitelist. This CL enables local builds to use resource whitelisting by setting the enable_resource_whitelist_generation gn flag to true, or by building an official build. This will allow developers to more easily monitor the changes in APK size for each commit they make. However, a large amount of output is generated (_pragma is used to create warnings to allow whitelisted resources to be listed), so for now the whitelist will only be generated for official builds. Comparison of ChromePublic.apk_InstallSize APK size using resource_sizes.py: * Local build with this patch: 73338275 bytes * Local build without this patch: 75248223 bytes BUG=632385 Committed: https://crrev.com/3f3fa5fc415baab5a875839e6520c2cbb245ec38 Cr-Commit-Position: refs/heads/master@{#409511} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3f3fa5fc415baab5a875839e6520c2cbb245ec38 Cr-Commit-Position: refs/heads/master@{#409511}
Message was sent while issue was closed.
On 2016/08/03 14:56:57, commit-bot: I haz the power wrote: > Patchset 5 (id:??) landed as > https://crrev.com/3f3fa5fc415baab5a875839e6520c2cbb245ec38 > Cr-Commit-Position: refs/heads/master@{#409511} This appears to be breaking internal chrome OS builds in the chroot (at least for me locally) with: chromeos-chrome-9999: FAILED: ash_test_strings.pak chromeos-chrome-9999: python ../../../../../../../home/stevenjb/chrome_root/src/tools/grit/grit/format/repack.py ash_test_strings.pak gen/ash/common/strings/ash_strings_en-US.pak gen/ui/strings/app_locale_settings_en-US.pak gen/ui/strings/ui_strings_en-US.pak gen/device/bluetooth/strings/bluetooth_strings_en-US.pak gen/ui/chromeos/strings/ui_chromeos_strings_en-US.pak chromeos-chrome-9999: Traceback (most recent call last): chromeos-chrome-9999: File "../../../../../../../home/stevenjb/chrome_root/src/tools/grit/grit/format/repack.py", line 40, in <module> chromeos-chrome-9999: main(sys.argv[1:]) chromeos-chrome-9999: File "../../../../../../../home/stevenjb/chrome_root/src/tools/grit/grit/format/repack.py", line 37, in main chromeos-chrome-9999: suppress_removed_key_output=options.suppress_removed_key_output) chromeos-chrome-9999: TypeError: RePack() got an unexpected keyword argument 'suppress_removed_key_output' I am testing outside of the chroot now.
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2209763002/ by estevenson@chromium.org. The reason for reverting is: Breaks internal Chrome OS builds..
Message was sent while issue was closed.
Description was changed from ========== Enable whitelist generation for official builds. Currently, all resources are included in PAK files when Chrome is built locally. Only official_buildbot.sh uses a resource whitelist. This CL enables local builds to use resource whitelisting by setting the enable_resource_whitelist_generation gn flag to true, or by building an official build. This will allow developers to more easily monitor the changes in APK size for each commit they make. However, a large amount of output is generated (_pragma is used to create warnings to allow whitelisted resources to be listed), so for now the whitelist will only be generated for official builds. Comparison of ChromePublic.apk_InstallSize APK size using resource_sizes.py: * Local build with this patch: 73338275 bytes * Local build without this patch: 75248223 bytes BUG=632385 Committed: https://crrev.com/3f3fa5fc415baab5a875839e6520c2cbb245ec38 Cr-Commit-Position: refs/heads/master@{#409511} ========== to ========== Enable whitelist generation for official builds. Currently, all resources are included in PAK files when Chrome is built locally. Only official_buildbot.sh uses a resource whitelist. This CL enables local builds to use resource whitelisting by setting the enable_resource_whitelist_generation gn flag to true, or by building an official build. This will allow developers to more easily monitor the changes in APK size for each commit they make. However, a large amount of output is generated (_pragma is used to create warnings to allow whitelisted resources to be listed), so for now the whitelist will only be generated for official builds. Comparison of ChromePublic.apk_InstallSize APK size using resource_sizes.py: * Local build with this patch: 73338275 bytes * Local build without this patch: 75248223 bytes BUG=632385 Committed: https://crrev.com/3f3fa5fc415baab5a875839e6520c2cbb245ec38 Cr-Commit-Position: refs/heads/master@{#409511} ==========
On 2016/08/15 14:34:29, Eric Stevenson wrote: When I attempt to apply this patch I encounter two problems: 1. For generate_resource_whitelist.py I get "Cannot rename file without two valid file names". The CL shows that file as A+ whereas I do not see it in ToT. Are you sure the patch is upstreamed correctly? 2. I end up with a bunch of untracked files in third_party/swiftshader. I remember this caused a follow up CL adding third_party/swiftshader to .gitignore, but I'm not sure whether or not that was correct? I also recall there being headaches reverting this because of that. Can we check that we are doing all of the right things to convertin swiftshader to an external dependency?
On 2016/08/15 16:28:48, stevenjb wrote: > On 2016/08/15 14:34:29, Eric Stevenson wrote: > > When I attempt to apply this patch I encounter two problems: > 1. For generate_resource_whitelist.py I get "Cannot rename file without two > valid file names". The CL shows that file as A+ whereas I do not see it in ToT. > Are you sure the patch is upstreamed correctly? > > 2. I end up with a bunch of untracked files in third_party/swiftshader. I > remember this caused a follow up CL adding third_party/swiftshader to > .gitignore, but I'm not sure whether or not that was correct? I also recall > there being headaches reverting this because of that. Can we check that we are > doing all of the right things to convertin swiftshader to an external > dependency? Please ignore the previous comment, I got my CLs confused. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
