Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2075863003:
Use runtime_deps to tell create_installer_archive what dlls to copy. (Closed)
DescriptionUse runtime_deps to tell create_installer_archive what dlls to copy.
If create_installer_archive copies every DLL, then its dependencies
aren't correctly explained to ninja and the bots complain when the
dependencies are newer than the output.
Using GN's runtime_deps allows us to avoid unnecessary copies and makes
the real dependencies match ninja's.
BUG=586967
Committed: https://crrev.com/c38143af142eee672cf9b5a270253655810a882e
Cr-Commit-Position: refs/heads/master@{#401456}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 9
Patch Set 3 : #
Total comments: 1
Patch Set 4 : remove other deps #
Total comments: 3
Patch Set 5 : nit fixes #
Total comments: 1
Messages
Total messages: 18 (4 generated)
jbauman@chromium.org changed reviewers: + gab@chromium.org
Thanks John, this is an awesome feature of GN I wasn't aware of :-) https://codereview.chromium.org/2075863003/diff/1/chrome/tools/build/win/crea... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/2075863003/diff/1/chrome/tools/build/win/crea... chrome/tools/build/win/create_installer_archive.py:498: 'url_lib.dll' ] This write_runtime_deps thing is awesome :-). Can you please also replace the list above with the runtime deps for setup.exe? Thanks! https://codereview.chromium.org/2075863003/diff/1/chrome/tools/build/win/crea... chrome/tools/build/win/create_installer_archive.py:513: # virtue of chrome.release. Update comment: # Stage all the DLLs listed in |runtime_deps| to the |version_dir| (for the version assembly to be able to refer to them below and make sure chrome.exe can find them at runtime), except the ones that are already staged (i.e. non-component DLLs). https://codereview.chromium.org/2075863003/diff/1/chrome/tools/build/win/crea... chrome/tools/build/win/create_installer_archive.py:521: build_dlls = glob.glob(os.path.join(build_dir, '*.dll')) When would |runtime_deps| ever not be specified? Would rather not keep a code path that isn't exercised. https://codereview.chromium.org/2075863003/diff/1/chrome/tools/build/win/crea... chrome/tools/build/win/create_installer_archive.py:534: continue rm 528-534 https://codereview.chromium.org/2075863003/diff/1/chrome/tools/build/win/crea... chrome/tools/build/win/create_installer_archive.py:647: 'list of component build DLLs to archive.') Make this a required option given comment above.
https://codereview.chromium.org/2075863003/diff/1/chrome/tools/build/win/crea... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/2075863003/diff/1/chrome/tools/build/win/crea... chrome/tools/build/win/create_installer_archive.py:521: build_dlls = glob.glob(os.path.join(build_dir, '*.dll')) On 2016/06/17 18:27:11, gab wrote: > When would |runtime_deps| ever not be specified? Would rather not keep a code > path that isn't exercised. Do we currently want to continue supporting gyp with mini_installer?
awesome! thanks for doing this. i was just asking how to do this on gn-dev a day or two ago.
PTAL. On 2016/06/17 18:27:11, gab wrote: > Thanks John, this is an awesome feature of GN I wasn't aware of :-) > > https://codereview.chromium.org/2075863003/diff/1/chrome/tools/build/win/crea... > File chrome/tools/build/win/create_installer_archive.py (right): > > https://codereview.chromium.org/2075863003/diff/1/chrome/tools/build/win/crea... > chrome/tools/build/win/create_installer_archive.py:498: 'url_lib.dll' ] > This write_runtime_deps thing is awesome :-). Can you please also replace the > list above with the runtime deps for setup.exe? Thanks! Done. > > https://codereview.chromium.org/2075863003/diff/1/chrome/tools/build/win/crea... > chrome/tools/build/win/create_installer_archive.py:513: # virtue of > chrome.release. > Update comment: > > # Stage all the DLLs listed in |runtime_deps| to the |version_dir| (for the > version assembly to be able to refer to them below and make sure chrome.exe can > find them at runtime), except the ones that are already staged (i.e. > non-component DLLs). Done. > > https://codereview.chromium.org/2075863003/diff/1/chrome/tools/build/win/crea... > chrome/tools/build/win/create_installer_archive.py:521: build_dlls = > glob.glob(os.path.join(build_dir, '*.dll')) > When would |runtime_deps| ever not be specified? Would rather not keep a code > path that isn't exercised. I left this glob code in for GYP support. > > https://codereview.chromium.org/2075863003/diff/1/chrome/tools/build/win/crea... > chrome/tools/build/win/create_installer_archive.py:534: continue > rm 528-534 > Also left in for GYP support. > https://codereview.chromium.org/2075863003/diff/1/chrome/tools/build/win/crea... > chrome/tools/build/win/create_installer_archive.py:647: 'list of component build > DLLs to archive.') > Make this a required option given comment above.
Ah right, forgot the need to support GYP still... please add TODOs to strip that code as noted below. Thanks! https://codereview.chromium.org/2075863003/diff/20001/chrome/installer/mini_i... File chrome/installer/mini_installer/BUILD.gn (right): https://codereview.chromium.org/2075863003/diff/20001/chrome/installer/mini_i... chrome/installer/mini_installer/BUILD.gn:72: # build DLLs need to be packaged. s/component build DLLs need to be packaged/component DLLs need to be packaged in a component build/ https://codereview.chromium.org/2075863003/diff/20001/chrome/installer/mini_i... chrome/installer/mini_installer/BUILD.gn:74: installer_runtime_deps = "$root_gen_dir/mini_installer.runtime_deps" s/installer_runtime_deps/component_runtime_deps/ or chrome_runtime_deps (see below) https://codereview.chromium.org/2075863003/diff/20001/chrome/installer/mini_i... chrome/installer/mini_installer/BUILD.gn:171: write_runtime_deps = installer_runtime_deps These actually only need to be the deps for chrome's target, not mini_installer. https://codereview.chromium.org/2075863003/diff/20001/chrome/tools/build/win/... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/2075863003/diff/20001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:489: else: # TODO: Remove when GYP is deprecated on Windows. https://codereview.chromium.org/2075863003/diff/20001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:505: # DLLs needed due to source sets. Remove the ones below (per comment only needed in GN which should no longer use this codepath) https://codereview.chromium.org/2075863003/diff/20001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:515: setup_component_dll_glob)) indent is weird https://codereview.chromium.org/2075863003/diff/20001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:531: # If no runtime_deps was specified, every DLL in build_dir is considered # TODO: Remove when GYP is deprecated on Windows. https://codereview.chromium.org/2075863003/diff/20001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:657: parser.add_option('--runtime_deps', --component_runtime_deps (here and in param names above) https://codereview.chromium.org/2075863003/diff/20001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:657: parser.add_option('--runtime_deps', # TODO: Make --component_runtime_deps and --setup_runtime_deps mandatory when GYP is deprecated on Windows.
PTAL.
https://codereview.chromium.org/2075863003/diff/40001/chrome/installer/mini_i... File chrome/installer/mini_installer/BUILD.gn (right): https://codereview.chromium.org/2075863003/diff/40001/chrome/installer/mini_i... chrome/installer/mini_installer/BUILD.gn:83: "//third_party/icu:icudata", I think you should only need to depend on "chrome_initial" (so long as that pulls in the tree of dependencies under it) If that list can't be a single item we risk getting the same issues where this needs to be updated as did the list in create_installer_archive.py :-(. "setup" for one should definitely not need to be in that list.
On 2016/06/21 15:15:16, gab wrote: > https://codereview.chromium.org/2075863003/diff/40001/chrome/installer/mini_i... > File chrome/installer/mini_installer/BUILD.gn (right): > > https://codereview.chromium.org/2075863003/diff/40001/chrome/installer/mini_i... > chrome/installer/mini_installer/BUILD.gn:83: "//third_party/icu:icudata", > I think you should only need to depend on "chrome_initial" (so long as that > pulls in the tree of dependencies under it) > > If that list can't be a single item we risk getting the same issues where this > needs to be updated as did the list in create_installer_archive.py :-(. > > "setup" for one should definitely not need to be in that list. Ok, removed everything except //chrome, which seems to be enough.
lgtm % nits, thanks! https://codereview.chromium.org/2075863003/diff/60001/chrome/installer/mini_i... File chrome/installer/mini_installer/BUILD.gn (right): https://codereview.chromium.org/2075863003/diff/60001/chrome/installer/mini_i... chrome/installer/mini_installer/BUILD.gn:74: chrome_runtime_deps = "$root_gen_dir/chrome_component.runtime_deps" nit: flip order here so they are declared in same order as their matching group below https://codereview.chromium.org/2075863003/diff/60001/chrome/installer/mini_i... chrome/installer/mini_installer/BUILD.gn:120: "--component_runtime_deps", Actually since this param is passed even in non-component builds (just happens to be ignored), how about --chrome_runtime_deps? Also matches better with the variable used for it. (same for variables inside the script) https://codereview.chromium.org/2075863003/diff/60001/chrome/tools/build/win/... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/2075863003/diff/60001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:655: 'list of component build DLLs to archive.') s/to archive/to archive in a component build/ (here and below)
The CQ bit was checked by jbauman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2075863003/#ps80001 (title: "nit fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2075863003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Use runtime_deps to tell create_installer_archive what dlls to copy. If create_installer_archive copies every DLL, then its dependencies aren't correctly explained to ninja and the bots complain when the dependencies are newer than the output. Using GN's runtime_deps allows us to avoid unnecessary copies and makes the real dependencies match ninja's. BUG=586967 ========== to ========== Use runtime_deps to tell create_installer_archive what dlls to copy. If create_installer_archive copies every DLL, then its dependencies aren't correctly explained to ninja and the bots complain when the dependencies are newer than the output. Using GN's runtime_deps allows us to avoid unnecessary copies and makes the real dependencies match ninja's. BUG=586967 Committed: https://crrev.com/c38143af142eee672cf9b5a270253655810a882e Cr-Commit-Position: refs/heads/master@{#401456} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c38143af142eee672cf9b5a270253655810a882e Cr-Commit-Position: refs/heads/master@{#401456}
Message was sent while issue was closed.
Hey John, if you have time, would be great to address these post-GYP TODOs :). https://codereview.chromium.org/2075863003/diff/80001/chrome/tools/build/win/... File chrome/tools/build/win/create_installer_archive.py (right): https://codereview.chromium.org/2075863003/diff/80001/chrome/tools/build/win/... chrome/tools/build/win/create_installer_archive.py:652: # mandatory when GYP is deprecated on Windows. We're now past the GYP era, mind prepping a CL for TODOs in this file :)? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
