Need to break dependency browser->blink |
|||||||||
Issue descriptionRepro: gn gen out/Default ninja -C out/Default chrome Build error: LINK ./nacl_helper FAILED: nacl_helper ../../content/common/child_process_sandbox_support_impl_linux.cc:69: error: undefined reference to 'blink::WebFontRenderStyle::setDefaults()'
,
Apr 30 2016
,
Apr 30 2016
Oh and this change from yesterday touched the same line as that last fix: https://codereview.chromium.org/1926443002 I'm betting it's that, will confirm :)
,
Apr 30 2016
Confirmed, reverting that change fixes the link error. scottmg@, please take a look.
,
Apr 30 2016
,
Apr 30 2016
I won't have time to look at this right away. You can revert if this is a config someone uses.
,
Apr 30 2016
Personally, I'm unblocked by switching to component build. I don't know how many people use debug static.
,
Apr 30 2016
It's probably OK to "fix" these by adding bizarre dependencies to the nacl components. Ideally things would be refactored better all over the place so that you don't have any dependency trail from nacl_helper to e.g. any part of blink, because that is just insane. In practice it has so far proven insurmountable to worry about that from the nacl_helper perspective (not that the required refactorings wouldn't be their own inherent goods for general modularity and so forth) because the status quo is just such a tangled web. This avoids being disastrous only because -ffunction-sections+--gc-sections clears out all the false dependencies quite well (and equivalent MSVC linker magic for the analogous nacl64.exe build). But someone needs to stay wary of nacl_helper (and nacl64.exe) binary size jumps and ensure that any "fix" for a problem like this one doesn't actually add anything to these binary sizes (in the release build anyway). That's just a drive-by because I'm out of the loop on NaCl maintenance by next week (short of dire needs, of course). Brad will have to be the one (to delegate) worrying about making sure this stuff doesn't slide now.
,
May 2 2016
I'm "surprised and delighted" to hear that nacl_helper depends on Blink. It does indeed look the same as https://codereview.chromium.org/1682843002/ where Nick commented it looked like the wrong fix. :/ It looks to me like exported/linux/WebFontRenderStyle.cpp should be in blink_minimal, and that should be the dependency, but I'm not too sure. My little Linux laptop is chugging along, but it might be a while.
,
May 3 2016
Maybe https://codereview.chromium.org/1945473002 is right, I'm not sure.
,
May 3 2016
Issue 608715 has been merged into this issue.
,
May 3 2016
Issue 608718 has been merged into this issue.
,
May 3 2016
Alright, I give, reverting https://codereview.chromium.org/1926443002. We'll have to make our code not crazy another day.
,
May 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8a6abb71c16674052eec28cff8d16d15fda4577b commit 8a6abb71c16674052eec28cff8d16d15fda4577b Author: scottmg <scottmg@chromium.org> Date: Tue May 03 18:50:16 2016 Revert of Turn on assert_no_deps for blink in browser (patchset #2 id:20001 of https://codereview.chromium.org/1926443002/ ) Reason for revert: debug static linux is sadness. BUG= 608114 and others. Original issue's description: > Turn on assert_no_deps for blink in browser > > R=brettw@chromium.org > BUG= 582206 > > Committed: https://crrev.com/ffb95be04fb4effb750884083cb1c166c5499b7a > Cr-Commit-Position: refs/heads/master@{#390123} TBR=brettw@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 582206 Review-Url: https://codereview.chromium.org/1947643002 Cr-Commit-Position: refs/heads/master@{#391314} [modify] https://crrev.com/8a6abb71c16674052eec28cff8d16d15fda4577b/chrome/BUILD.gn [modify] https://crrev.com/8a6abb71c16674052eec28cff8d16d15fda4577b/components/password_manager/content/public/cpp/BUILD.gn [modify] https://crrev.com/8a6abb71c16674052eec28cff8d16d15fda4577b/content/browser/BUILD.gn [modify] https://crrev.com/8a6abb71c16674052eec28cff8d16d15fda4577b/content/common/BUILD.gn
,
May 4 2016
I trial-and-errored this today for a while: https://codereview.chromium.org/1945473002/. It passes the bots (but hey, that's what they said last time). Not sure if the Blink changes are kosher either. blimp_shell->blink was added fairly recently in https://chromium.googlesource.com/chromium/src/+/12a558c16f853751ab702c84fbd0ffd7de565cdc. I gave blimp_shell a direct dependency on blink for that, because... I dunno. I'm not sure what that should be doing, maybe someone else has a thought.
,
May 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/581bbb4d26224e4dce31e93651af0df157901c60 commit 581bbb4d26224e4dce31e93651af0df157901c60 Author: scottmg <scottmg@chromium.org> Date: Fri May 06 18:35:16 2016 Break dependencies of browser -> blink In order to get assert_no_deps on, move a couple small implementation files to blink_common (WebInputEvent.cpp and WebFontRenderStyle.cpp). blimp_shell adds a dependency on blink, which it needs for blink::WebFontRendering::setSkiaFontManager. I'm not sure what to do about that one. BUG= 582206 , 608114 Review-Url: https://codereview.chromium.org/1945473002 Cr-Commit-Position: refs/heads/master@{#392105} [modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/blimp/client/BUILD.gn [modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/chrome/BUILD.gn [modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/components/password_manager/content/public/cpp/BUILD.gn [modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/content/browser/BUILD.gn [modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/content/common/BUILD.gn [modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/third_party/WebKit/Source/platform/BUILD.gn [modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/third_party/WebKit/Source/platform/blink_platform.gyp [modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/third_party/WebKit/Source/platform/blink_platform.gypi [modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/third_party/WebKit/Source/web/WebInputEvent.cpp [modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/third_party/WebKit/Source/web/web.gypi [modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/third_party/WebKit/public/platform/linux/WebFontRenderStyle.h [modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/third_party/WebKit/public/web/WebInputEvent.h
,
Jun 10 2016
fixed?
,
Jun 10 2016
Yes, I think this was fixed.
,
Jun 10 2016
Blimp is still broken.
,
Jun 10 2016
(or was.. I'm not sure if it got fixed. I guess it could be a new bug by now anyway.)
,
Jun 10 2016
khushalsagar, I think you were looking at that, did you manage to figure out a way to avoid the dependency?
,
Jun 15 2016
Sorry about the late reply, that dependency exists in browser on linux because we need to create the SkFontMgr for overriding blink's default font manager before the sandbox is initialized in the zygote process, because the font manager opens and cached fds on initialization. This is because we have to use the android font manager on the engine for an android client, and it assumes access to font files within the renderer sandbox. I think the better solution would be to fix the font management setup here so we can rely on the browser for access to these files instead. This will either way be necessary for Blimp, crbug.com/601129 is tracking it. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by halliwell@chromium.org
, Apr 30 2016