Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2036683002:
[iOS] Remove dependency third_party/WebKit on iOS. (Closed)
|
Created:
4 years, 6 months ago by sdefresne Modified:
4 years, 6 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, blink-reviews, darin (slow to review), ben+mojo_chromium.org, blink-reviews-wtf_chromium.org, Mikhail, bzanotti Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[iOS] Remove dependency third_party/WebKit on iOS.
Chrome on iOS now requires mojo for WebUI which has an optional dependency
on third_party/WebKit. Chrome on iOS cannot depends on third_party/WebKit,
so only enable those optional dependencies if target OS is not iOS.
Fixes the following build failures when building with clang Xcode (which
is only done by downstream release bots as we do not have capacity to run
it on each commit):
error: unable to load plugin 'third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib': 'dlopen(third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib, 9): Symbol not found: __ZN4llvm8RegistryIN5clang15PluginASTActionEE4HeadE
Referenced from: third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib
To prevent further dependencies on third_party/WebKit to creep in change
third_party/WebKit/Source/{config.gyp,wtf/wtf.gyp} to not define any
target when target OS is iOS (in order to generate errors when running
"gclient runhooks").
This is similar to corresponding GN changes in issue
https://codereview.chromium.org/2004743002/.
BUG=616750
Committed: https://crrev.com/aa9125d2a9f1417d069a99d178f86784f8210215
Cr-Commit-Position: refs/heads/master@{#398272}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add comment explaining why target are conditionally defined #
Total comments: 8
Patch Set 3 : Fix comment #Patch Set 4 : Rebase #
Messages
Total messages: 29 (11 generated)
Description was changed from
==========
[iOS] Remove dependency third_party/WebKit on iOS.
Chrome on iOS now requires mojo for WebUI which has an optional dependency
on third_party/WebKit. Chrome on iOS cannot depends on third_party/WebKit,
so only enable those optional dependencies if target OS is not iOS.
Fixes the following build failures when building with clang Xcode (which
is only done by downstream release bots as we do not have capacity to run
it on each commit):
error: unable to load plugin
'third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib':
'dlopen(third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib, 9):
Symbol not found: __ZN4llvm8RegistryIN5clang15PluginASTActionEE4HeadE
Referenced from:
third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib
To prevent further dependencies on third_party/WebKit to creep in change
third_party/WebKit/Source/{config.gyp,wtf/wtf.gyp} to not define any
target when target OS is iOS (in order to generate errors when running
"gclient runhooks").
BUG=None
==========
to
==========
[iOS] Remove dependency third_party/WebKit on iOS.
Chrome on iOS now requires mojo for WebUI which has an optional dependency
on third_party/WebKit. Chrome on iOS cannot depends on third_party/WebKit,
so only enable those optional dependencies if target OS is not iOS.
Fixes the following build failures when building with clang Xcode (which
is only done by downstream release bots as we do not have capacity to run
it on each commit):
error: unable to load plugin
'third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib':
'dlopen(third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib, 9):
Symbol not found: __ZN4llvm8RegistryIN5clang15PluginASTActionEE4HeadE
Referenced from:
third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib
To prevent further dependencies on third_party/WebKit to creep in change
third_party/WebKit/Source/{config.gyp,wtf/wtf.gyp} to not define any
target when target OS is iOS (in order to generate errors when running
"gclient runhooks").
This is similar to corresponding GN changes in issue
https://codereview.chromium.org/2004743002/.
BUG=None
==========
sdefresne@chromium.org changed reviewers: + brettw@chromium.org, thakis@chromium.org
Patchset #1 (id:1) has been deleted
Please take a look as OWNERS for the following files. If this looks good I'd like to land it quickly as it prevent producing canary builds of Chrome on iOS and will delay our testing. The issue was only spotted recently because we only recently started to integrate mojo on iOS. thakis: third_party/WebKit/Source/config.gyp third_party/WebKit/Source/wtf/wtf.gyp tools/clang/scripts/blink_gc_plugin_flags.py brettw: mojo/mojo_public.gyp url/url.gyp bzanotti: FYI
Description was changed from
==========
[iOS] Remove dependency third_party/WebKit on iOS.
Chrome on iOS now requires mojo for WebUI which has an optional dependency
on third_party/WebKit. Chrome on iOS cannot depends on third_party/WebKit,
so only enable those optional dependencies if target OS is not iOS.
Fixes the following build failures when building with clang Xcode (which
is only done by downstream release bots as we do not have capacity to run
it on each commit):
error: unable to load plugin
'third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib':
'dlopen(third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib, 9):
Symbol not found: __ZN4llvm8RegistryIN5clang15PluginASTActionEE4HeadE
Referenced from:
third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib
To prevent further dependencies on third_party/WebKit to creep in change
third_party/WebKit/Source/{config.gyp,wtf/wtf.gyp} to not define any
target when target OS is iOS (in order to generate errors when running
"gclient runhooks").
This is similar to corresponding GN changes in issue
https://codereview.chromium.org/2004743002/.
BUG=None
==========
to
==========
[iOS] Remove dependency third_party/WebKit on iOS.
Chrome on iOS now requires mojo for WebUI which has an optional dependency
on third_party/WebKit. Chrome on iOS cannot depends on third_party/WebKit,
so only enable those optional dependencies if target OS is not iOS.
Fixes the following build failures when building with clang Xcode (which
is only done by downstream release bots as we do not have capacity to run
it on each commit):
error: unable to load plugin
'third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib':
'dlopen(third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib, 9):
Symbol not found: __ZN4llvm8RegistryIN5clang15PluginASTActionEE4HeadE
Referenced from:
third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib
To prevent further dependencies on third_party/WebKit to creep in change
third_party/WebKit/Source/{config.gyp,wtf/wtf.gyp} to not define any
target when target OS is iOS (in order to generate errors when running
"gclient runhooks").
This is similar to corresponding GN changes in issue
https://codereview.chromium.org/2004743002/.
BUG=616750
==========
On 2016/06/02 11:27:30, sdefresne wrote: > Please take a look as OWNERS for the following files. If this looks good I'd > like to land it quickly as it prevent producing canary builds of Chrome on iOS > and will delay our testing. > > The issue was only spotted recently because we only recently started to > integrate mojo on iOS. > > thakis: > third_party/WebKit/Source/config.gyp > third_party/WebKit/Source/wtf/wtf.gyp > tools/clang/scripts/blink_gc_plugin_flags.py > brettw: > mojo/mojo_public.gyp > url/url.gyp > > bzanotti: FYI Can this be reviewed/resolved ASAP? This has broken our canary builds.
Ping?
Ping. This is blocking Chrome on iOS release process.
sdefresne@chromium.org changed reviewers: + justincohen@chromium.org
Justin: can you review as an iOS reviewer?
https://codereview.chromium.org/2036683002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/config.gyp (right): https://codereview.chromium.org/2036683002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/config.gyp:34: 'variables': { why is the change in this file needed? this is a config-only target. (if there's some good reason for this, the config target should exist unconditionally and whatever causes the problem in the target shouldn't be set for ios) https://codereview.chromium.org/2036683002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/wtf.gyp (right): https://codereview.chromium.org/2036683002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/wtf.gyp:41: 'target_name': 'wtf_config', ditto
https://codereview.chromium.org/2036683002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/config.gyp (right): https://codereview.chromium.org/2036683002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/config.gyp:34: 'variables': { On 2016/06/06 13:16:19, Nico wrote: > why is the change in this file needed? this is a config-only target. (if there's > some good reason for this, the config target should exist unconditionally and > whatever causes the problem in the target shouldn't be set for ios) The goal is to have the target not exists when OS=="ios" in order to cause a gyp failure if a target depends on it when OS=="ios". If I leave this target defined, then the same error will creep back again (as there is no way in gyp to forbid including a file or to forbid depending on a target except by conditionally defining the target). I explained in the CL description that the target have been changed to be conditionally defined on iOS so that we get a failure when running "gclient runhooks".
ah, thanks https://codereview.chromium.org/2036683002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/config.gyp (right): https://codereview.chromium.org/2036683002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/config.gyp:33: ['OS!="ios"', { please add a comment here too then. ("# Normally, targets should exist unconditionally and only their contents should be conditional. This target is intentionally visible only conditionally, to make sure targets using blink aren't part of the ios build. If you get an error about this target not existing, then you need to make the target you get the error for exist only on non-ios", or something like that. in the other file where you make a config not visible too)
lgtm from iOS
PTAL https://codereview.chromium.org/2036683002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/config.gyp (right): https://codereview.chromium.org/2036683002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/config.gyp:33: ['OS!="ios"', { On 2016/06/06 13:28:10, Nico wrote: > please add a comment here too then. ("# Normally, targets should exist > unconditionally and only their contents should be conditional. This target is > intentionally visible only conditionally, to make sure targets using blink > aren't part of the ios build. If you get an error about this target not > existing, then you need to make the target you get the error for exist only on > non-ios", or something like that. in the other file where you make a config not > visible too) Done.
this lgtm, but the lhs in mojo looks a bit wrong to me -- do you know where that code was added? https://codereview.chromium.org/2036683002/diff/40001/mojo/mojo_public.gyp File mojo/mojo_public.gyp (left): https://codereview.chromium.org/2036683002/diff/40001/mojo/mojo_public.gyp#ol... mojo/mojo_public.gyp:257: '../third_party/WebKit/Source/config.gyp:config', btw, do you know who added these? things outside of blink depending on WebKit without going through the webkit api seems strange to me. https://codereview.chromium.org/2036683002/diff/40001/mojo/mojo_public.gyp#ol... mojo/mojo_public.gyp:266: 'clang_warning_flags_unset': [ '-Wglobal-constructors' ], and this looks definitely very incorrect too :-/ https://codereview.chromium.org/2036683002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/config.gyp (right): https://codereview.chromium.org/2036683002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/config.gyp:37: # existing, then you must make the target depending on iOS conditional on *the target depending on _blink_ conditional on OS not being iOS https://codereview.chromium.org/2036683002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/wtf.gyp (right): https://codereview.chromium.org/2036683002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/wtf.gyp:40: # existing, then you must make the target depending on iOS conditional on ditto
thakis/justincohen: Thank you for the review. brettw: ping for OWNERS approval. https://codereview.chromium.org/2036683002/diff/40001/mojo/mojo_public.gyp File mojo/mojo_public.gyp (left): https://codereview.chromium.org/2036683002/diff/40001/mojo/mojo_public.gyp#ol... mojo/mojo_public.gyp:257: '../third_party/WebKit/Source/config.gyp:config', On 2016/06/06 14:06:27, Nico wrote: > btw, do you know who added these? things outside of blink depending on WebKit > without going through the webkit api seems strange to me. It was added by https://codereview.chromium.org/1751563002. https://codereview.chromium.org/2036683002/diff/40001/mojo/mojo_public.gyp#ol... mojo/mojo_public.gyp:266: 'clang_warning_flags_unset': [ '-Wglobal-constructors' ], On 2016/06/06 14:06:27, Nico wrote: > and this looks definitely very incorrect too :-/ Same CL. https://codereview.chromium.org/2036683002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/config.gyp (right): https://codereview.chromium.org/2036683002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/config.gyp:37: # existing, then you must make the target depending on iOS conditional on On 2016/06/06 14:06:27, Nico wrote: > *the target depending on _blink_ conditional on OS not being iOS Done. https://codereview.chromium.org/2036683002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/wtf.gyp (right): https://codereview.chromium.org/2036683002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/wtf.gyp:40: # existing, then you must make the target depending on iOS conditional on On 2016/06/06 14:06:27, Nico wrote: > ditto Done.
lgtm
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2036683002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by sdefresne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from justincohen@chromium.org, thakis@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2036683002/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2036683002/80001
Message was sent while issue was closed.
Description was changed from
==========
[iOS] Remove dependency third_party/WebKit on iOS.
Chrome on iOS now requires mojo for WebUI which has an optional dependency
on third_party/WebKit. Chrome on iOS cannot depends on third_party/WebKit,
so only enable those optional dependencies if target OS is not iOS.
Fixes the following build failures when building with clang Xcode (which
is only done by downstream release bots as we do not have capacity to run
it on each commit):
error: unable to load plugin
'third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib':
'dlopen(third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib, 9):
Symbol not found: __ZN4llvm8RegistryIN5clang15PluginASTActionEE4HeadE
Referenced from:
third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib
To prevent further dependencies on third_party/WebKit to creep in change
third_party/WebKit/Source/{config.gyp,wtf/wtf.gyp} to not define any
target when target OS is iOS (in order to generate errors when running
"gclient runhooks").
This is similar to corresponding GN changes in issue
https://codereview.chromium.org/2004743002/.
BUG=616750
==========
to
==========
[iOS] Remove dependency third_party/WebKit on iOS.
Chrome on iOS now requires mojo for WebUI which has an optional dependency
on third_party/WebKit. Chrome on iOS cannot depends on third_party/WebKit,
so only enable those optional dependencies if target OS is not iOS.
Fixes the following build failures when building with clang Xcode (which
is only done by downstream release bots as we do not have capacity to run
it on each commit):
error: unable to load plugin
'third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib':
'dlopen(third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib, 9):
Symbol not found: __ZN4llvm8RegistryIN5clang15PluginASTActionEE4HeadE
Referenced from:
third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib
To prevent further dependencies on third_party/WebKit to creep in change
third_party/WebKit/Source/{config.gyp,wtf/wtf.gyp} to not define any
target when target OS is iOS (in order to generate errors when running
"gclient runhooks").
This is similar to corresponding GN changes in issue
https://codereview.chromium.org/2004743002/.
BUG=616750
==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from
==========
[iOS] Remove dependency third_party/WebKit on iOS.
Chrome on iOS now requires mojo for WebUI which has an optional dependency
on third_party/WebKit. Chrome on iOS cannot depends on third_party/WebKit,
so only enable those optional dependencies if target OS is not iOS.
Fixes the following build failures when building with clang Xcode (which
is only done by downstream release bots as we do not have capacity to run
it on each commit):
error: unable to load plugin
'third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib':
'dlopen(third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib, 9):
Symbol not found: __ZN4llvm8RegistryIN5clang15PluginASTActionEE4HeadE
Referenced from:
third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib
To prevent further dependencies on third_party/WebKit to creep in change
third_party/WebKit/Source/{config.gyp,wtf/wtf.gyp} to not define any
target when target OS is iOS (in order to generate errors when running
"gclient runhooks").
This is similar to corresponding GN changes in issue
https://codereview.chromium.org/2004743002/.
BUG=616750
==========
to
==========
[iOS] Remove dependency third_party/WebKit on iOS.
Chrome on iOS now requires mojo for WebUI which has an optional dependency
on third_party/WebKit. Chrome on iOS cannot depends on third_party/WebKit,
so only enable those optional dependencies if target OS is not iOS.
Fixes the following build failures when building with clang Xcode (which
is only done by downstream release bots as we do not have capacity to run
it on each commit):
error: unable to load plugin
'third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib':
'dlopen(third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib, 9):
Symbol not found: __ZN4llvm8RegistryIN5clang15PluginASTActionEE4HeadE
Referenced from:
third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib
To prevent further dependencies on third_party/WebKit to creep in change
third_party/WebKit/Source/{config.gyp,wtf/wtf.gyp} to not define any
target when target OS is iOS (in order to generate errors when running
"gclient runhooks").
This is similar to corresponding GN changes in issue
https://codereview.chromium.org/2004743002/.
BUG=616750
Committed: https://crrev.com/aa9125d2a9f1417d069a99d178f86784f8210215
Cr-Commit-Position: refs/heads/master@{#398272}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/aa9125d2a9f1417d069a99d178f86784f8210215 Cr-Commit-Position: refs/heads/master@{#398272} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
