|
|
DescriptionOnly depend on child sources from app in both target
Added here: https://codereview.chromium.org/1817903002. This makes
chrome.dll depend on all of WebKit at build time.
This changes restores
c:\src\cr\src>gn path //out/Release //chrome:main_dll //third_party/WebKit/public:blink
No non-data paths found between these two targets.
Before this change, public_app_shared_sources was included into multi-dll browser, causing all the child deps to be linked in there.
(crbug.com/680772 is about why assert_no_deps didn't catch this.)
R=brettw@chromium.org
BUG=581766, 680772
Review-Url: https://codereview.chromium.org/2623143005
Cr-Commit-Position: refs/heads/master@{#443636}
Committed: https://chromium.googlesource.com/chromium/src/+/6b2011f69d69405750fef23c7fdc374735fbe160
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : attempt for non-win #
Total comments: 1
Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #Messages
Total messages: 48 (38 generated)
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Description was changed from ========== Only depend on child sources from app in component build Added here: https://codereview.chromium.org/1817903002. This makes chrome.dll depend on all of WebKit at build time, which ideally it wouldn't. I think it's only necessary in component mode, so conditionalize on that. (?) R=brettw@chromium.org BUG=581766 ========== to ========== Only depend on child sources from app in component build Added here: https://codereview.chromium.org/1817903002. This makes chrome.dll depend on all of WebKit at build time, which ideally it wouldn't. I think it's only necessary in component mode, so conditionalize on that. (?) Part of removing dependency of Windows browser DLL on v8. R=brettw@chromium.org BUG=581766 ==========
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by scottmg@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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by scottmg@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: CQ cannot get SignCLA result. Please try later.
The CQ bit was checked by scottmg@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...
Patchset #4 (id:60001) has been deleted
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm https://codereview.chromium.org/2623143005/diff/40001/content/public/app/BUIL... File content/public/app/BUILD.gn (right): https://codereview.chromium.org/2623143005/diff/40001/content/public/app/BUIL... content/public/app/BUILD.gn:44: public_app_shared_deps += [ Can you add a comment here about why this is necessary?
On 2017/01/12 22:51:37, brettw (ping after 24h) wrote: > lgtm > > https://codereview.chromium.org/2623143005/diff/40001/content/public/app/BUIL... > File content/public/app/BUILD.gn (right): > > https://codereview.chromium.org/2623143005/diff/40001/content/public/app/BUIL... > content/public/app/BUILD.gn:44: public_app_shared_deps += [ > Can you add a comment here about why this is necessary? Yeah, if I figure it out I will! I don't want the dependency from chrome:main_dll to gpu_sources because that in turn depends on gpu which depends on all the child stuff. Similarly, chrome:main_dll to renderer_sources -> renderer -> blink, etc. Removing it almost works except that content_main_runner.cc and content_main_delegate.cc want the empty ContentGpuClient in test binaries. I'm not sure what the correct fix is yet. content_main_delegate.obj : error LNK2001: unresolved external symbol "public: virtual class gpu::SyncPointManager * __cdecl content::ContentGpuClient::GetSyncPointManager(void)" (?GetSyncPointManager@ContentGpuClient@content@@UEAAPEAVSyncPointManager@gpu@@XZ) content_main_runner.obj : error LNK2001: unresolved external symbol "public: virtual class gpu::SyncPointManager * __cdecl content::ContentGpuClient::GetSyncPointManager(void)" (?GetSyncPointManager@ContentGpuClient@content@@UEAAPEAVSyncPointManager@gpu@@XZ)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by scottmg@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 checked by scottmg@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.
Description was changed from ========== Only depend on child sources from app in component build Added here: https://codereview.chromium.org/1817903002. This makes chrome.dll depend on all of WebKit at build time, which ideally it wouldn't. I think it's only necessary in component mode, so conditionalize on that. (?) Part of removing dependency of Windows browser DLL on v8. R=brettw@chromium.org BUG=581766 ========== to ========== Only depend on child sources from app in both target Added here: https://codereview.chromium.org/1817903002. This makes chrome.dll depend on all of WebKit at build time. This changes restores c:\src\cr\src>gn path //out/Release //chrome:main_dll //third_party/WebKit/pubNo non-data paths found between these two targets. R=brettw@chromium.org BUG=581766,680772 ==========
Description was changed from ========== Only depend on child sources from app in both target Added here: https://codereview.chromium.org/1817903002. This makes chrome.dll depend on all of WebKit at build time. This changes restores c:\src\cr\src>gn path //out/Release //chrome:main_dll //third_party/WebKit/pubNo non-data paths found between these two targets. R=brettw@chromium.org BUG=581766,680772 ========== to ========== Only depend on child sources from app in both target Added here: https://codereview.chromium.org/1817903002. This makes chrome.dll depend on all of WebKit at build time. This changes restores c:\src\cr\src>gn path //out/Release //chrome:main_dll //third_party/WebKit/public:blink No non-data paths found between these two targets. R=brettw@chromium.org BUG=581766,680772 ==========
Description was changed from ========== Only depend on child sources from app in both target Added here: https://codereview.chromium.org/1817903002. This makes chrome.dll depend on all of WebKit at build time. This changes restores c:\src\cr\src>gn path //out/Release //chrome:main_dll //third_party/WebKit/public:blink No non-data paths found between these two targets. R=brettw@chromium.org BUG=581766,680772 ========== to ========== Only depend on child sources from app in both target Added here: https://codereview.chromium.org/1817903002. This makes chrome.dll depend on all of WebKit at build time. This changes restores c:\src\cr\src>gn path //out/Release //chrome:main_dll //third_party/WebKit/public:blink No non-data paths found between these two targets. Before this change, public_app_shared_sources was included into multi-dll browser, causing all the child deps to be linked in there. (crbug.com/680772 is about why assert_no_deps didn't catch this.) R=brettw@chromium.org BUG=581766,680772 ==========
How do you feel about this approach? Does seem to fix the problem, but I don't think I really understand the intricacies of all the possible configs.
The CQ bit was checked by scottmg@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.
scottmg@chromium.org changed reviewers: + dpranke@chromium.org
Dirk, I believe Brett is out, would you be able to review this one?
lgtm, but I'm not an owner. It's probably fine to tbr brett or jam, though.
On 2017/01/13 19:27:55, Dirk Pranke wrote: > lgtm, but I'm not an owner. It's probably fine to tbr brett or jam, though. Yup, please have a look when you have time Brett.
On 2017/01/13 19:27:55, Dirk Pranke wrote: > lgtm, but I'm not an owner. It's probably fine to tbr brett or jam, though. (Thanks!)
The CQ bit was checked by scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2623143005/#ps110001 (title: ".")
The CQ bit was checked by scottmg@chromium.org
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": 110001, "attempt_start_ts": 1484335741284470, "parent_rev": "1224f7bab210549bd2110b224e5697bc29feb411", "commit_rev": "6b2011f69d69405750fef23c7fdc374735fbe160"}
Message was sent while issue was closed.
Description was changed from ========== Only depend on child sources from app in both target Added here: https://codereview.chromium.org/1817903002. This makes chrome.dll depend on all of WebKit at build time. This changes restores c:\src\cr\src>gn path //out/Release //chrome:main_dll //third_party/WebKit/public:blink No non-data paths found between these two targets. Before this change, public_app_shared_sources was included into multi-dll browser, causing all the child deps to be linked in there. (crbug.com/680772 is about why assert_no_deps didn't catch this.) R=brettw@chromium.org BUG=581766,680772 ========== to ========== Only depend on child sources from app in both target Added here: https://codereview.chromium.org/1817903002. This makes chrome.dll depend on all of WebKit at build time. This changes restores c:\src\cr\src>gn path //out/Release //chrome:main_dll //third_party/WebKit/public:blink No non-data paths found between these two targets. Before this change, public_app_shared_sources was included into multi-dll browser, causing all the child deps to be linked in there. (crbug.com/680772 is about why assert_no_deps didn't catch this.) R=brettw@chromium.org BUG=581766,680772 Review-Url: https://codereview.chromium.org/2623143005 Cr-Commit-Position: refs/heads/master@{#443636} Committed: https://chromium.googlesource.com/chromium/src/+/6b2011f69d69405750fef23c7fdc... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:110001) as https://chromium.googlesource.com/chromium/src/+/6b2011f69d69405750fef23c7fdc... |