assert_no_deps doesn't seem to be working as expected |
||
Issue descriptionSome details here: https://bugs.chromium.org/p/chromium/issues/detail?id=581766#c24 At head for a non-component build, //chrome:main_dll currently depends on //third_party/WebKit/public:blink. main_dll has a deps on group(:browser_dependencies), which in turn has an assert_no_deps that includes "//third_party/WebKit/public:blink", however it doesn't trigger. Adding assert_no_deps = ["//third_party/WebKit/public:blink"] directly to main_dll does cause it to trigger. I believe this caused a 13+M regression in chrome.dll binary size 10 months ago in https://codereview.chromium.org/1817903002. :/
,
Jan 13 2017
It turns out is_official_build=true isn't enough to get real official now, and when the second official flag (full_wpo_on_official=true) is turned on, the linker seems to manage to drop that 13M. So fortunately, there wasn't a size regression. We should still investigate why assert_no_deps doesn't seem to catch this though.
,
Jan 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6b2011f69d69405750fef23c7fdc374735fbe160 commit 6b2011f69d69405750fef23c7fdc374735fbe160 Author: scottmg <scottmg@chromium.org> Date: Fri Jan 13 19:36:44 2017 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} [modify] https://crrev.com/6b2011f69d69405750fef23c7fdc374735fbe160/content/public/app/BUILD.gn
,
Jan 23 2017
assert_no_deps only affects the dependencies of a target, not everything that ends up linking that target. Higher-level targets can add dependencies on those things without tripping the assertion. If you wanted to assert main_dll doesn't depend on blink, the assertion would need to be on main_dll (or something that itself depends on main_dll).
,
Jan 23 2017
You're saying this didn't make sense then? https://codereview.chromium.org/1643633003/diff/100001/chrome/BUILD.gn ? Do you think group() should be flattened into its parent dependency for this check?
,
Jan 23 2017
Maybe I'm misunderstanding. What was the path from main_dll to public_app_shared_sources that was bringing in blink?
,
Jan 24 2017
I'm probably confused. public_app_shared_sources was used in source_set(browser) here https://codereview.chromium.org/1817903002/diff/60001/content/public/app/BUILD.gn?context=10&column_width=80&tab_spaces=10 which main_dll depends on. That seems to make sense. But the assert_no_deps is in chrome/ in group(browser_dependencies). That doesn't seem to express what we want since the whole point is to keep blink out of the browser dll. |
||
►
Sign in to add a comment |
||
Comment 1 by dpranke@chromium.org
, Jan 12 2017Owner: brettw@chromium.org
Status: Assigned (was: Untriaged)