Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2670723002:
[MD History] Vulcanize as part of GN build. (Closed)
|
Created:
3 years, 10 months ago by calamity Modified:
3 years, 10 months ago CC:
arv+watch_chromium.org, asanka, chrome-apps-syd-reviews_chromium.org, chromium-reviews, dbeam+watch-history_chromium.org, dbeam+watch-downloads_chromium.org, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, pam+watch_chromium.org, tsergeant Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD History] Vulcanize as part of GN build.
This CL removes the need to manually vulcanize MD History by adding the
vulcanization step to the build process.
This CL also:
- changes the request list to be per target-name so that multiple
bundles in the same directory don't overwrite each other's request
lists.
- makes the MD History presubmit check enforce git cl formatting.
BUG=673825
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2670723002
Cr-Commit-Position: refs/heads/master@{#449824}
Committed: https://chromium.googlesource.com/chromium/src/+/9b40cc378411af0dccf39b5ab74137ae8ac7ea69
Patch Set 1 #
Total comments: 5
Patch Set 2 : revert css_build #
Total comments: 14
Patch Set 3 : rebase #Patch Set 4 : address_comments #
Total comments: 2
Patch Set 5 : address nits #
Total comments: 1
Messages
Total messages: 42 (23 generated)
Description was changed from ========== [MD History] Vulcanize as part of GN build. This CL removes the need to manually vulcanize MD History by adding the vulcanization step to the build process. This CL also moves the polymer-css-build out of the vulcanize_gn script and into its own script so that multiple files can be specified. BUG=673825 ========== to ========== [MD History] Vulcanize as part of GN build. This CL removes the need to manually vulcanize MD History by adding the vulcanization step to the build process. This CL also moves the polymer-css-build out of the vulcanize_gn script and into its own script so that multiple files can be specified. BUG=673825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [MD History] Vulcanize as part of GN build. This CL removes the need to manually vulcanize MD History by adding the vulcanization step to the build process. This CL also moves the polymer-css-build out of the vulcanize_gn script and into its own script so that multiple files can be specified. BUG=673825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD History] Vulcanize as part of GN build. This CL removes the need to manually vulcanize MD History by adding the vulcanization step to the build process. This CL also: - moves the polymer-css-build out of the vulcanize_gn script and into its own script so that multiple files can be specified. - changes the request list to be per target-name so that multiple bundles in the same directory don't overwrite each other's request lists. - makes the MD History presubmit check enforce git cl formatting. BUG=673825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
calamity@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2670723002/diff/20001/chrome/browser/resource... File chrome/browser/resources/vulcanize.gni (right): https://codereview.chromium.org/2670723002/diff/20001/chrome/browser/resource... chrome/browser/resources/vulcanize.gni:80: } I'm not sure how to share this logic between this and the vulcanize template. I was thinking of doing a base template("node") with an action but then I'd have to copy all of the properties out of invoker... Any ideas?
The CQ bit was checked by calamity@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
cool! https://codereview.chromium.org/2670723002/diff/20001/chrome/browser/resource... File chrome/browser/resources/vulcanize.gni (right): https://codereview.chromium.org/2670723002/diff/20001/chrome/browser/resource... chrome/browser/resources/vulcanize.gni:80: } On 2017/02/02 04:24:05, calamity wrote: > I'm not sure how to share this logic between this and the vulcanize template. > > I was thinking of doing a base template("node") with an action but then I'd have > to copy all of the properties out of invoker... > > Any ideas? i think it might be worthwhile to make a "all things node" template and track all inputs that might affect node. perhaps a little simpler (for now): is it possible to hoist this logic out of both rules to the top of this file and just append a common list of inputs to both vulcanize / css_build? node_inputs = [] if (is_linux) { node_inputs += [...] } if (is_win) { node_inputs += [...] } if (is_mac) { node_inputs += [...] } template("vulcanize") { inputs = node_inputs ... } i don't know GN super well. it looks like Python but isn't.
https://codereview.chromium.org/2670723002/diff/20001/chrome/browser/resource... File chrome/browser/resources/vulcanize.gni (right): https://codereview.chromium.org/2670723002/diff/20001/chrome/browser/resource... chrome/browser/resources/vulcanize.gni:80: } On 2017/02/02 at 17:14:52, Dan Beam (slow) wrote: > On 2017/02/02 04:24:05, calamity wrote: > > I'm not sure how to share this logic between this and the vulcanize template. > > > > I was thinking of doing a base template("node") with an action but then I'd have > > to copy all of the properties out of invoker... > > > > Any ideas? > > i think it might be worthwhile to make a "all things node" template and track all inputs that might affect node. > > perhaps a little simpler (for now): is it possible to hoist this logic out of both rules to the top of this file and just append a common list of inputs to both vulcanize / css_build? > > node_inputs = [] > if (is_linux) { node_inputs += [...] } > if (is_win) { node_inputs += [...] } > if (is_mac) { node_inputs += [...] } > > template("vulcanize") { > inputs = node_inputs > ... > } > > i don't know GN super well. it looks like Python but isn't. "All thing node" template was implemented at https://codereview.chromium.org/2533153002. I never landed this (or sent for review), but I can revive it if you think is useful.
> "All thing node" template was implemented at https://codereview.chromium.org/2533153002. I never landed this (or sent for review), but I can revive it if you think is useful. BTW, I am a bit concerned with making large scale changes to vulcanize_gn.py while our Settings vulcanization is in progress, and there are still few uncommitted changes to the same file, in order to make Windows work correctly. Referring to https://codereview.chromium.org/2674553002/ specifically. I'll take a closer look at this CL though, perhaps there is no real conflicts, even if we change the same file. Would it be reasonable to split this CL as follows? CL 1: Make all the necessary node.py/vulcanize_gn.py changes on a separate CL (splitting the css_build stuff to its own gni/py) CL 2: Migrate History to use it
https://codereview.chromium.org/2670723002/diff/20001/chrome/browser/resource... File chrome/browser/resources/vulcanize.gni (right): https://codereview.chromium.org/2670723002/diff/20001/chrome/browser/resource... chrome/browser/resources/vulcanize.gni:65: template("css_build") { Can we move this to its own css_build.gni now that the logic has been moved to a separate python script?
https://codereview.chromium.org/2670723002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_history/BUILD.gn (right): https://codereview.chromium.org/2670723002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_history/BUILD.gn:33: css_build("build") { I am trying to understand the necessity of splitting css_build to its own action. At first glance, I don't see why is necessary (and I think it makes the build files more complicated). Why can't we have the vulcanize() rule run polymer_css_build, on the html_out_file. We are removing that as part of vulcanize, and then we invoke it separately, just to pass the 2 previously created html_out_file(s) together. Per my understanding if each vulcanize() invocation produces a fully processed output file, there is no need to do anything separately. There will be one rule for each of vulcanize_app and vulcanize_lazy_load, and each of the outputs will have already been processed by polymer_css_build.
On 2017/02/02 22:08:46, dpapad wrote: > https://codereview.chromium.org/2670723002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/md_history/BUILD.gn (right): > > https://codereview.chromium.org/2670723002/diff/20001/chrome/browser/resource... > chrome/browser/resources/md_history/BUILD.gn:33: css_build("build") { > I am trying to understand the necessity of splitting css_build to its own > action. At first glance, I don't see why is necessary (and I think it makes the > build files more complicated). > > Why can't we have the vulcanize() rule run polymer_css_build, on the > html_out_file. We are removing that as part of vulcanize, and then we invoke it > separately, just to pass the 2 previously created html_out_file(s) together. > > Per my understanding if each vulcanize() invocation produces a fully processed > output file, there is no need to do anything separately. There will be one rule > for each of vulcanize_app and vulcanize_lazy_load, and each of the outputs will > have already been processed by polymer_css_build. polymer_css_build produces different outputs when running on a single file vs multiple. In this case, it inlines the cr-shared-styles, slightly increasing the size of the lazy_load bundle but making the style resolution quicker. This is pretty minor, but there's a possibility that polymer-css-build will add more optimizations when looking across multiple bundles simultaneously. I'm happy to leave the css build how it is since the benefit is so small. WDYT?
On 2017/02/03 at 02:19:17, calamity wrote: > On 2017/02/02 22:08:46, dpapad wrote: > > https://codereview.chromium.org/2670723002/diff/20001/chrome/browser/resource... > > File chrome/browser/resources/md_history/BUILD.gn (right): > > > > https://codereview.chromium.org/2670723002/diff/20001/chrome/browser/resource... > > chrome/browser/resources/md_history/BUILD.gn:33: css_build("build") { > > I am trying to understand the necessity of splitting css_build to its own > > action. At first glance, I don't see why is necessary (and I think it makes the > > build files more complicated). > > > > Why can't we have the vulcanize() rule run polymer_css_build, on the > > html_out_file. We are removing that as part of vulcanize, and then we invoke it > > separately, just to pass the 2 previously created html_out_file(s) together. > > > > Per my understanding if each vulcanize() invocation produces a fully processed > > output file, there is no need to do anything separately. There will be one rule > > for each of vulcanize_app and vulcanize_lazy_load, and each of the outputs will > > have already been processed by polymer_css_build. > > polymer_css_build produces different outputs when running on a single file vs multiple. In this case, it inlines the cr-shared-styles, slightly increasing the size of the lazy_load bundle but making the style resolution quicker. This is pretty minor, but there's a possibility that polymer-css-build will add more optimizations when looking across multiple bundles simultaneously. > > I'm happy to leave the css build how it is since the benefit is so small. WDYT? Leaving as is for now, SGTM because of its simplicity. Once we get everything working (Settings, Downloads (check), History), we can revisit whether there is room for further optimizations and whether they are worth the investment. We already have a bit of technical debt to solve after we have on-the-fly Vulcanization of the above WebUI pages. 1) Migrate Vulcanize to polymer-bundler, as well as trying to make more sense of how/why Vulcanize handles relative paths on Windows in a weird way (and potentially remove the platform specific logic in vulcanize_gn.py). 2) You might have already noticed that there are two slightly different codepaths in vulcanize_gn.py, PAK_FILE and FOLDER. I would like eventually to merge all our pages to a single codepath (PAK_FILE).
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by calamity@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== [MD History] Vulcanize as part of GN build. This CL removes the need to manually vulcanize MD History by adding the vulcanization step to the build process. This CL also: - moves the polymer-css-build out of the vulcanize_gn script and into its own script so that multiple files can be specified. - changes the request list to be per target-name so that multiple bundles in the same directory don't overwrite each other's request lists. - makes the MD History presubmit check enforce git cl formatting. BUG=673825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD History] Vulcanize as part of GN build. This CL removes the need to manually vulcanize MD History by adding the vulcanization step to the build process. This CL also: - changes the request list to be per target-name so that multiple bundles in the same directory don't overwrite each other's request lists. - makes the MD History presubmit check enforce git cl formatting. BUG=673825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Okay, reverted the CSS build stuff. Now the patch is simple enough that I don't think it's worth splitting into 2 pieces.
you need to merge/rebase because i landed some changes to the flags and some other code you're updating. sorry! but it should make your stuff way saner on Windows. i'll check your .requestlist and .d files on windows soon. there's a pretty gnarly bug in nodejs itself (in url.resolve() not dealing with chrome:// URLs well). it affects vulcanize. https://github.com/nodejs/node/issues/8921 https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_history/BUILD.gn (right): https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/BUILD.gn:30: "chrome://history/constants.html", why are you excluding these things? util.html and constants.html? https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_history/PRESUBMIT.py (right): https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/PRESUBMIT.py:7: input_api, output_api, check_js=True) might want to do this separately, but i get that it's the only thing left after removing the vulcanize stuff https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/vulcanize_gn.py (left): https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn.py:110: out_path, _REQUEST_LIST_FILE), 'r').read().splitlines() rebase https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn.py:195: parser.add_argument('--exclude', action='append') rebase
but generally looks great, btw!
https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/vulcanize.gni (right): https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... chrome/browser/resources/vulcanize.gni:66: "--exclude", Can we not just pass all the excludes with a single --exclude flag? It seems simpler and I think python's argparse can handle that for us, by passing nargs='*' to add_argument. https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn.py:45: '--exclude', 'crisper.js', Is this necessary? We run crisper after vulcanize, so there shouldn't be any crisper.js references by the time vulcanize runs. Besides that, it also refers to a hardcoded crisper.js name instead of using js_out_file, so probably obsolete. https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn.py:99: return os.path.join(out_path, html_out_file + '.requestlist') I am not super fond of using random file extensions for a text file. Can we either name it <html_out_file>_requestlist.txt OR <html_out_file>_requestlist (no extension at all)? Also, I guess html_out_file is unique, even if there are multiple vulcanize() targets in the same BUILD.gn file, so this is OK. The alternative is to pass ${target_name} from the gni to the python script, which is guaranteed to always be unique. In this case it would generate two files: vulcanize_app_requestlist.txt and vulcanize_lazy_load_request_list.txt
https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_history/BUILD.gn (right): https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/BUILD.gn:30: "chrome://history/constants.html", On 2017/02/08 05:41:55, Dan Beam wrote: > why are you excluding these things? util.html and constants.html? These files are loaded by history.html before app.vulcanized.html https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_history/PRESUBMIT.py (right): https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/PRESUBMIT.py:7: input_api, output_api, check_js=True) On 2017/02/08 05:41:55, Dan Beam wrote: > might want to do this separately, but i get that it's the only thing left after > removing the vulcanize stuff Lol, so the script here blows up because it expects the generated files to exist. I could delete this then add the format in the next patch, but I thought this may as well just happen in one step. https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/vulcanize.gni (right): https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... chrome/browser/resources/vulcanize.gni:66: "--exclude", On 2017/02/08 23:04:18, dpapad wrote: > Can we not just pass all the excludes with a single --exclude flag? It seems > simpler and I think python's argparse can handle that for us, by passing > nargs='*' to add_argument. Done. https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/vulcanize_gn.py (left): https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn.py:110: out_path, _REQUEST_LIST_FILE), 'r').read().splitlines() On 2017/02/08 05:41:55, Dan Beam wrote: > rebase Done. https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn.py:45: '--exclude', 'crisper.js', On 2017/02/08 23:04:18, dpapad wrote: > Is this necessary? We run crisper after vulcanize, so there shouldn't be any > crisper.js references by the time vulcanize runs. > > Besides that, it also refers to a hardcoded crisper.js name instead of using > js_out_file, so probably obsolete. I've removed this and nothing bad seems to have happened. https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn.py:99: return os.path.join(out_path, html_out_file + '.requestlist') On 2017/02/08 23:04:19, dpapad wrote: > I am not super fond of using random file extensions for a text file. Can we > either name it > <html_out_file>_requestlist.txt OR > <html_out_file>_requestlist (no extension at all)? > > Also, I guess html_out_file is unique, even if there are multiple vulcanize() > targets in the same BUILD.gn file, so this is OK. The alternative is to pass > ${target_name} from the gni to the python script, which is guaranteed to always > be unique. In this case it would generate two files: > > vulcanize_app_requestlist.txt and vulcanize_lazy_load_request_list.txt I've changed it to <html_out_file>_requestlist.txt because I think it's easier to associate with the actual file it relates to. And the .txt extension ensures this will openly correctly on Windows in Notepad. https://codereview.chromium.org/2670723002/diff/60001/chrome/browser/resource... chrome/browser/resources/vulcanize_gn.py:195: parser.add_argument('--exclude', action='append') On 2017/02/08 05:41:55, Dan Beam wrote: > rebase Done.
LGTM++ https://codereview.chromium.org/2670723002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/vulcanize.gni (right): https://codereview.chromium.org/2670723002/diff/100001/chrome/browser/resourc... chrome/browser/resources/vulcanize.gni:55: args += [ "--exclude" ] Nit: Would that work? args += [ "--exclude" ] + invoker.excludes https://codereview.chromium.org/2670723002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2670723002/diff/100001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:189: # crbug.com/619091. The referenced bug is marked as Fixed. Is this TODO still relevant?
The CQ bit was checked by calamity@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...
lgtm
fwiw: I just looked at the 2 .d and _requestlist.txt files on windows. 👍👍
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2670723002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/vulcanize.py (left): https://codereview.chromium.org/2670723002/diff/120001/chrome/browser/resourc... chrome/browser/resources/vulcanize.py:1: #!/usr/bin/env python FYI, this file is still referenced by https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/reproduce.sh?q=..., which seems obsolete. Perhaps remove the reference in the same CL?
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2670723002/#ps120001 (title: "address nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486776925135590,
"parent_rev": "77aa1a3c7c75269bf904d4d447cee5b7052120e2", "commit_rev":
"9b40cc378411af0dccf39b5ab74137ae8ac7ea69"}
Message was sent while issue was closed.
Description was changed from ========== [MD History] Vulcanize as part of GN build. This CL removes the need to manually vulcanize MD History by adding the vulcanization step to the build process. This CL also: - changes the request list to be per target-name so that multiple bundles in the same directory don't overwrite each other's request lists. - makes the MD History presubmit check enforce git cl formatting. BUG=673825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD History] Vulcanize as part of GN build. This CL removes the need to manually vulcanize MD History by adding the vulcanization step to the build process. This CL also: - changes the request list to be per target-name so that multiple bundles in the same directory don't overwrite each other's request lists. - makes the MD History presubmit check enforce git cl formatting. BUG=673825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2670723002 Cr-Commit-Position: refs/heads/master@{#449824} Committed: https://chromium.googlesource.com/chromium/src/+/9b40cc378411af0dccf39b5ab741... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/9b40cc378411af0dccf39b5ab741... |
