Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2665333002:
Add a BUILD target and a script to build a public static library for ios/web_view. (Closed)
|
Created:
3 years, 10 months ago by Hiroshi Ichikawa Modified:
3 years, 8 months ago CC:
chromium-reviews, Eugene But (OOO till 7-30) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a BUILD target and a script to build a public static library for
ios/web_view.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Apply review comments. #
Total comments: 2
Patch Set 3 : Let ios_web_view_shell use the generated public static library. #Patch Set 4 : Slightly simplified a build rule. #
Total comments: 6
Patch Set 5 : Apply review comments. #Patch Set 6 : Make it work after the separation of ARC-enabled/disabled targets. #
Total comments: 2
Patch Set 7 : Rebase. #
Messages
Total messages: 36 (11 generated)
Description was changed from ========== Add a BUILD target and a script to build a public static library for ios/web_view. BUG= ========== to ========== Add a BUILD target and a script to build a public static library for ios/web_view. BUG= ==========
ichikawa@chromium.org changed reviewers: + michaeldo@chromium.org
Let me know if there's a better way to write BUILD rules and structure things for this. I'm not familiar with GN...
Also it may be better to generate a framework for this, which can bundle resource files and header files as well. (Currently users need to copy those separately.) I can add TODO for it if it makes sense.
michaeldo@chromium.org changed reviewers: + sdefresne@chromium.org
+ Sylvain for input on linker error and static_lib correctness. Please wrap the description at 72 chars. (During git-cl upload, when the description is opened for editing, the top lines have a guide for the max line length. You can change the editor which is used with "export EXIDTOR=..." in your ~/.profile or other shell dotfile. If you pass the description on the command line, I don't believe it will auto-wrap) Overall this looks OK to me, but I noticed two things when trying this patch locally. 1. When building (ninja -C out/Debug-iphonesimulator/ ios/web_view:web_view_public) I noticed lots of warnings of the following format, but I am unsure the cause. ld: warning: URGENT: building for OSX, but linking in object file (obj/ios/web_view/internal/libdeps_complete.a(Sk4fGradientBase.o)) built for iOS. Note: This will be an error in the future. 2. Eventually, web_view/shell should depend on this library target. In order to keep CLs simple and clean, that change should not be included in this CL. However, I used it to test the correct-ness of the library that is generated. ios_web_view_shell didn't link correctly when I changed the dep from //ios/web_view to //ios/web_view:web_view_public I understand not all symbols are yet marked with the attribute, but the error I saw was about CRIWV which is marked. Please take a look to ensure the symbols are correctly stripped. https://codereview.chromium.org/2665333002/diff/1/ios/web_view/BUILD.gn File ios/web_view/BUILD.gn (right): https://codereview.chromium.org/2665333002/diff/1/ios/web_view/BUILD.gn#newco... ios/web_view/BUILD.gn:19: copy("web_view_public") { I think lib_web_view is a better name for now. Public seems to me like it belongs in the public directory rather than here.
Description was changed from ========== Add a BUILD target and a script to build a public static library for ios/web_view. BUG= ========== to ========== Add a BUILD target and a script to build a public static library for ios/web_view. ==========
On 2017/02/03 17:27:52, michaeldo wrote: > + Sylvain for input on linker error and static_lib correctness. > > Please wrap the description at 72 chars. (During git-cl upload, when the > description is opened for editing, the top lines have a guide for the max line > length. You can change the editor which is used with "export EXIDTOR=..." in > your ~/.profile or other shell dotfile. If you pass the description on the > command line, I don't believe it will auto-wrap) Done. > > Overall this looks OK to me, but I noticed two things when trying this patch > locally. > > 1. When building (ninja -C out/Debug-iphonesimulator/ > ios/web_view:web_view_public) I noticed lots of warnings of the following > format, but I am unsure the cause. > ld: warning: URGENT: building for OSX, but linking in object file > (obj/ios/web_view/internal/libdeps_complete.a(Sk4fGradientBase.o)) built for > iOS. Note: This will be an error in the future. I found that it was due to missing -arch parameter. Added. > > 2. Eventually, web_view/shell should depend on this library target. In order to > keep CLs simple and clean, that change should not be included in this CL. > However, I used it to test the correct-ness of the library that is generated. > ios_web_view_shell didn't link correctly when I changed the dep from > //ios/web_view to //ios/web_view:web_view_public I understand not all symbols > are yet marked with the attribute, but the error I saw was about CRIWV which is > marked. Please take a look to ensure the symbols are correctly stripped. I found that it is because it doesn't link libweb_view_public.a if you add //ios/web_view:web_view_public to deps. It just makes sure that the rule //ios/web_view:web_view_public is executed. I guess it is because web_view_public is "copy" rule, not "static_library". Do you know how to link to a static library generated by "copy" or "action" rule? I made sure that libweb_view.a exports CRIWV: $ nm -gU out/Debug-iphonesimulator/obj/ios/web_view/libweb_view.a out/Debug-iphonesimulator/obj/ios/web_view/libweb_view.a(libweb_view.o): 00000000026c1118 S _OBJC_CLASS_$_CRIWV 00000000026c1140 S _OBJC_METACLASS_$_CRIWV ... Here's command executed by building ios_web_shell after changing its dependency to //ios/web_view:lib_web_view: TOOL_VERSION=1481791277 ../../build/toolchain/mac/linker_driver.py ../../third_party/llvm-build/Release+Asserts/bin/clang++ -Xlinker -rpath -Xlinker @executable_path/Frameworks -Xlinker -objc_abi_version -Xlinker 2 -Xlinker -sectcreate -Xlinker __TEXT -Xlinker __entitlements -Xlinker obj/ios/web_view/shell/ios_web_view_shell.xcent -arch x86_64 -Werror -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator10.2.sdk -stdlib=libc++ -mios-simulator-version-min=9.0 -Wl,-ObjC -o "obj/ios/web_view/shell/x64/ios_web_view_shell" -Wl,-filelist,"obj/ios/web_view/shell/x64/ios_web_view_shell.rsp" -framework CFNetwork -framework CoreFoundation -framework CoreGraphics -framework CoreText -framework Foundation -framework ImageIO -framework MobileCoreServices -framework Security -framework SystemConfiguration -framework UIKit -framework WebKit -lresolv $ cat out/Debug-iphonesimulator/obj/ios/web_view/shell/x64/ios_web_view_shell.rsp obj/ios/web_view/shell/ios_web_view_shell_arch_executable_sources/shell_app_delegate.o obj/ios/web_view/shell/ios_web_view_shell_arch_executable_sources/shell_delegate.o obj/ios/web_view/shell/ios_web_view_shell_arch_executable_sources/shell_exe_main.o obj/ios/web_view/shell/ios_web_view_shell_arch_executable_sources/shell_view_controller.o obj/ios/web_view/shell/ios_web_view_shell_arch_executable_sources/translate_controller.o obj/base/libbase.a obj/base/libbase_paths.a obj/base/libbase_static.a obj/base/third_party/dynamic_annotations/libdynamic_annotations.a obj/third_party/modp_b64/libmodp_b64.a obj/base/third_party/libevent/libevent.a As you can see, ios_web_view_shell.rsp contains .a files in //base, but not libweb_view.a.
ichikawa@google.com changed reviewers: + ichikawa@google.com
https://codereview.chromium.org/2665333002/diff/1/ios/web_view/BUILD.gn File ios/web_view/BUILD.gn (right): https://codereview.chromium.org/2665333002/diff/1/ios/web_view/BUILD.gn#newco... ios/web_view/BUILD.gn:19: copy("web_view_public") { On 2017/02/03 17:27:52, michaeldo wrote: > I think lib_web_view is a better name for now. Public seems to me like it > belongs in the public directory rather than here. Done.
On 2017/02/06 05:46:29, ichikawa wrote:
> On 2017/02/03 17:27:52, michaeldo wrote:
> > + Sylvain for input on linker error and static_lib correctness.
> >
> > Please wrap the description at 72 chars. (During git-cl upload, when the
> > description is opened for editing, the top lines have a guide for the max
line
> > length. You can change the editor which is used with "export EXIDTOR=..." in
> > your ~/.profile or other shell dotfile. If you pass the description on the
> > command line, I don't believe it will auto-wrap)
>
> Done.
>
> >
> > Overall this looks OK to me, but I noticed two things when trying this patch
> > locally.
> >
> > 1. When building (ninja -C out/Debug-iphonesimulator/
> > ios/web_view:web_view_public) I noticed lots of warnings of the following
> > format, but I am unsure the cause.
> > ld: warning: URGENT: building for OSX, but linking in object file
> > (obj/ios/web_view/internal/libdeps_complete.a(Sk4fGradientBase.o)) built for
> > iOS. Note: This will be an error in the future.
>
> I found that it was due to missing -arch parameter. Added.
>
> >
> > 2. Eventually, web_view/shell should depend on this library target. In order
> to
> > keep CLs simple and clean, that change should not be included in this CL.
> > However, I used it to test the correct-ness of the library that is
generated.
> > ios_web_view_shell didn't link correctly when I changed the dep from
> > //ios/web_view to //ios/web_view:web_view_public I understand not all
symbols
> > are yet marked with the attribute, but the error I saw was about CRIWV which
> is
> > marked. Please take a look to ensure the symbols are correctly stripped.
>
> I found that it is because it doesn't link libweb_view_public.a if you add
> //ios/web_view:web_view_public to deps. It just makes sure that the rule
> //ios/web_view:web_view_public is executed. I guess it is because
> web_view_public is "copy" rule, not "static_library".
>
> Do you know how to link to a static library generated by "copy" or "action"
> rule?
The simplest would be to declare a config that add it to libs and then have a
public_configs (but not sure if you can do this with a copy/action target, you
may have to introduce a group target).
config("libweb_view_config") {
libs = [ "libweb_view"]
lib_dirs = [ "$root_out_dir" ]
}
copy("libweb_view") {
...
public_configs = [":libweb_view_config" ]
}
If this does not work, add a group target:
config("libweb_view_config") {
libs = [ "libweb_view"]
lib_dirs = [ "$root_out_dir" ]
}
copy("libweb_view_copy") {
...
}
group("libweb_view") {
deps = ":libweb_view_copy"
public_configs = [":libweb_view_config"]
}
> I made sure that libweb_view.a exports CRIWV:
>
> $ nm -gU out/Debug-iphonesimulator/obj/ios/web_view/libweb_view.a
>
> out/Debug-iphonesimulator/obj/ios/web_view/libweb_view.a(libweb_view.o):
> 00000000026c1118 S _OBJC_CLASS_$_CRIWV
> 00000000026c1140 S _OBJC_METACLASS_$_CRIWV
> ...
>
> Here's command executed by building ios_web_shell after changing its
dependency
> to //ios/web_view:lib_web_view:
>
> TOOL_VERSION=1481791277 ../../build/toolchain/mac/linker_driver.py
> ../../third_party/llvm-build/Release+Asserts/bin/clang++ -Xlinker -rpath
> -Xlinker @executable_path/Frameworks -Xlinker -objc_abi_version -Xlinker 2
> -Xlinker -sectcreate -Xlinker __TEXT -Xlinker __entitlements -Xlinker
> obj/ios/web_view/shell/ios_web_view_shell.xcent -arch x86_64 -Werror -isysroot
>
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator10.2.sdk
> -stdlib=libc++ -mios-simulator-version-min=9.0 -Wl,-ObjC -o
> "obj/ios/web_view/shell/x64/ios_web_view_shell"
> -Wl,-filelist,"obj/ios/web_view/shell/x64/ios_web_view_shell.rsp" -framework
> CFNetwork -framework CoreFoundation -framework CoreGraphics -framework
CoreText
> -framework Foundation -framework ImageIO -framework MobileCoreServices
> -framework Security -framework SystemConfiguration -framework UIKit -framework
> WebKit -lresolv
>
> $ cat
> out/Debug-iphonesimulator/obj/ios/web_view/shell/x64/ios_web_view_shell.rsp
>
obj/ios/web_view/shell/ios_web_view_shell_arch_executable_sources/shell_app_delegate.o
>
obj/ios/web_view/shell/ios_web_view_shell_arch_executable_sources/shell_delegate.o
>
obj/ios/web_view/shell/ios_web_view_shell_arch_executable_sources/shell_exe_main.o
>
obj/ios/web_view/shell/ios_web_view_shell_arch_executable_sources/shell_view_controller.o
>
obj/ios/web_view/shell/ios_web_view_shell_arch_executable_sources/translate_controller.o
> obj/base/libbase.a
> obj/base/libbase_paths.a
> obj/base/libbase_static.a
> obj/base/third_party/dynamic_annotations/libdynamic_annotations.a
> obj/third_party/modp_b64/libmodp_b64.a
> obj/base/third_party/libevent/libevent.a
>
> As you can see, ios_web_view_shell.rsp contains .a files in //base, but not
> libweb_view.a.
https://codereview.chromium.org/2665333002/diff/20001/ios/web_view/internal/B... File ios/web_view/internal/BUILD.gn (right): https://codereview.chromium.org/2665333002/diff/20001/ios/web_view/internal/B... ios/web_view/internal/BUILD.gn:5: DEPS = [ nit: I would name this "_deps" to mark that this is a private name
https://codereview.chromium.org/2665333002/diff/20001/ios/web_view/internal/B... File ios/web_view/internal/BUILD.gn (right): https://codereview.chromium.org/2665333002/diff/20001/ios/web_view/internal/B... ios/web_view/internal/BUILD.gn:5: DEPS = [ On 2017/02/06 14:22:44, sdefresne wrote: > nit: I would name this "_deps" to mark that this is a private name Done.
PTAL.
On 2017/02/06 14:22:38, sdefresne wrote:
> On 2017/02/06 05:46:29, ichikawa wrote:
> > On 2017/02/03 17:27:52, michaeldo wrote:
> > > + Sylvain for input on linker error and static_lib correctness.
> > >
> > > Please wrap the description at 72 chars. (During git-cl upload, when the
> > > description is opened for editing, the top lines have a guide for the max
> line
> > > length. You can change the editor which is used with "export EXIDTOR=..."
in
> > > your ~/.profile or other shell dotfile. If you pass the description on the
> > > command line, I don't believe it will auto-wrap)
> >
> > Done.
> >
> > >
> > > Overall this looks OK to me, but I noticed two things when trying this
patch
> > > locally.
> > >
> > > 1. When building (ninja -C out/Debug-iphonesimulator/
> > > ios/web_view:web_view_public) I noticed lots of warnings of the following
> > > format, but I am unsure the cause.
> > > ld: warning: URGENT: building for OSX, but linking in object file
> > > (obj/ios/web_view/internal/libdeps_complete.a(Sk4fGradientBase.o)) built
for
> > > iOS. Note: This will be an error in the future.
> >
> > I found that it was due to missing -arch parameter. Added.
> >
> > >
> > > 2. Eventually, web_view/shell should depend on this library target. In
order
> > to
> > > keep CLs simple and clean, that change should not be included in this CL.
> > > However, I used it to test the correct-ness of the library that is
> generated.
> > > ios_web_view_shell didn't link correctly when I changed the dep from
> > > //ios/web_view to //ios/web_view:web_view_public I understand not all
> symbols
> > > are yet marked with the attribute, but the error I saw was about CRIWV
which
> > is
> > > marked. Please take a look to ensure the symbols are correctly stripped.
> >
> > I found that it is because it doesn't link libweb_view_public.a if you add
> > //ios/web_view:web_view_public to deps. It just makes sure that the rule
> > //ios/web_view:web_view_public is executed. I guess it is because
> > web_view_public is "copy" rule, not "static_library".
> >
> > Do you know how to link to a static library generated by "copy" or "action"
> > rule?
>
>
> The simplest would be to declare a config that add it to libs and then have a
> public_configs (but not sure if you can do this with a copy/action target, you
> may have to introduce a group target).
>
> config("libweb_view_config") {
> libs = [ "libweb_view"]
> lib_dirs = [ "$root_out_dir" ]
> }
>
> copy("libweb_view") {
> ...
> public_configs = [":libweb_view_config" ]
> }
Thanks, it worked!
I replaced //ios/web_view:web_view with the copy target lib_web_view so that
ios_web_view_shell also depends on the generated libweb_view.a. Let me know if
we should keep an option to link to //ios/web_view in the traditional way.
>
> If this does not work, add a group target:
>
> config("libweb_view_config") {
> libs = [ "libweb_view"]
> lib_dirs = [ "$root_out_dir" ]
> }
>
> copy("libweb_view_copy") {
> ...
> }
>
> group("libweb_view") {
> deps = ":libweb_view_copy"
> public_configs = [":libweb_view_config"]
> }
>
>
> > I made sure that libweb_view.a exports CRIWV:
> >
> > $ nm -gU out/Debug-iphonesimulator/obj/ios/web_view/libweb_view.a
> >
> > out/Debug-iphonesimulator/obj/ios/web_view/libweb_view.a(libweb_view.o):
> > 00000000026c1118 S _OBJC_CLASS_$_CRIWV
> > 00000000026c1140 S _OBJC_METACLASS_$_CRIWV
> > ...
> >
> > Here's command executed by building ios_web_shell after changing its
> dependency
> > to //ios/web_view:lib_web_view:
> >
> > TOOL_VERSION=1481791277 ../../build/toolchain/mac/linker_driver.py
> > ../../third_party/llvm-build/Release+Asserts/bin/clang++ -Xlinker -rpath
> > -Xlinker @executable_path/Frameworks -Xlinker -objc_abi_version -Xlinker 2
> > -Xlinker -sectcreate -Xlinker __TEXT -Xlinker __entitlements -Xlinker
> > obj/ios/web_view/shell/ios_web_view_shell.xcent -arch x86_64 -Werror
-isysroot
> >
>
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator10.2.sdk
> > -stdlib=libc++ -mios-simulator-version-min=9.0 -Wl,-ObjC -o
> > "obj/ios/web_view/shell/x64/ios_web_view_shell"
> > -Wl,-filelist,"obj/ios/web_view/shell/x64/ios_web_view_shell.rsp"
-framework
> > CFNetwork -framework CoreFoundation -framework CoreGraphics -framework
> CoreText
> > -framework Foundation -framework ImageIO -framework MobileCoreServices
> > -framework Security -framework SystemConfiguration -framework UIKit
-framework
> > WebKit -lresolv
> >
> > $ cat
> > out/Debug-iphonesimulator/obj/ios/web_view/shell/x64/ios_web_view_shell.rsp
> >
>
obj/ios/web_view/shell/ios_web_view_shell_arch_executable_sources/shell_app_delegate.o
> >
>
obj/ios/web_view/shell/ios_web_view_shell_arch_executable_sources/shell_delegate.o
> >
>
obj/ios/web_view/shell/ios_web_view_shell_arch_executable_sources/shell_exe_main.o
> >
>
obj/ios/web_view/shell/ios_web_view_shell_arch_executable_sources/shell_view_controller.o
> >
>
obj/ios/web_view/shell/ios_web_view_shell_arch_executable_sources/translate_controller.o
> > obj/base/libbase.a
> > obj/base/libbase_paths.a
> > obj/base/libbase_static.a
> > obj/base/third_party/dynamic_annotations/libdynamic_annotations.a
> > obj/third_party/modp_b64/libmodp_b64.a
> > obj/base/third_party/libevent/libevent.a
> >
> > As you can see, ios_web_view_shell.rsp contains .a files in //base, but not
> > libweb_view.a.
Thanks for the updates! LGTM I think it's good that //ios/web_view:web_view is replaced with the copy target. This ensures that our shell uses the lib and not some other dependency path. sdefresne, can you PTAL at the latest patch and provide commiter's approval? https://codereview.chromium.org/2665333002/diff/60001/ios/web_view/BUILD.gn File ios/web_view/BUILD.gn (right): https://codereview.chromium.org/2665333002/diff/60001/ios/web_view/BUILD.gn#n... ios/web_view/BUILD.gn:17: # The generated libweb_view.a can be also linked to apps outside Chromium The second sentence provides enough information, please remove first sentence. https://codereview.chromium.org/2665333002/diff/60001/ios/web_view/internal/B... File ios/web_view/internal/BUILD.gn (right): https://codereview.chromium.org/2665333002/diff/60001/ios/web_view/internal/B... ios/web_view/internal/BUILD.gn:51: # Used to build "web_view". Please remove the second comment line ("Used to build "web_view"."). This could easily become a stale comment in the future. https://codereview.chromium.org/2665333002/diff/60001/ios/web_view/internal/B... ios/web_view/internal/BUILD.gn:58: # A static library expected to be used by apps outside Chromium code base. I suggest removing these comments. The public interface to the library is in //ios/web and you have similar comments there. You can leave the comments in //ios/web. You can leave the TODO here, but please create bug and update format to include bug number: // TODO(crbug.com/######): Something that needs doing.
https://codereview.chromium.org/2665333002/diff/60001/ios/web_view/BUILD.gn File ios/web_view/BUILD.gn (right): https://codereview.chromium.org/2665333002/diff/60001/ios/web_view/BUILD.gn#n... ios/web_view/BUILD.gn:17: # The generated libweb_view.a can be also linked to apps outside Chromium On 2017/02/07 18:14:20, michaeldo wrote: > The second sentence provides enough information, please remove first sentence. Done. https://codereview.chromium.org/2665333002/diff/60001/ios/web_view/internal/B... File ios/web_view/internal/BUILD.gn (right): https://codereview.chromium.org/2665333002/diff/60001/ios/web_view/internal/B... ios/web_view/internal/BUILD.gn:51: # Used to build "web_view". On 2017/02/07 18:14:20, michaeldo wrote: > Please remove the second comment line ("Used to build "web_view"."). This could > easily become a stale comment in the future. Done. https://codereview.chromium.org/2665333002/diff/60001/ios/web_view/internal/B... ios/web_view/internal/BUILD.gn:58: # A static library expected to be used by apps outside Chromium code base. On 2017/02/07 18:14:20, michaeldo wrote: > I suggest removing these comments. The public interface to the library is in > //ios/web and you have similar comments there. You can leave the comments in > //ios/web. You can leave the TODO here, but please create bug and update format > to include bug number: > > // TODO(crbug.com/######): Something that needs doing. Done.
I rebased to the latest head. It got a bit tricky after Mike separated :internal into :internal and :internal_arc because static_library only contains its direct "sources", not its dependencies. I made both :internal and :internal_arc static libraries (renamed to :web_view_non_arc_incomplete and :web_view_arc_incomplete) and modified hide_symbols.py to accept multiple static libraries. Let me know if you prefer any other solution. in_reply_to: 5754903989321728 send_mail: 1 subject: Add a BUILD target and a script to build a public static library for ios/web_view.
On 2017/02/10 06:36:45, Hiroshi Ichikawa wrote: > I rebased to the latest head. > > It got a bit tricky after Mike separated :internal into :internal and > :internal_arc because static_library only contains its direct "sources", not its > dependencies. > > I made both :internal and :internal_arc static libraries (renamed to > :web_view_non_arc_incomplete and :web_view_arc_incomplete) and modified > hide_symbols.py to accept multiple static libraries. > > Let me know if you prefer any other solution. Sorry about this, the splitting of the arc/non arc is commonplace in the build files and I didn't realize it would case trouble for this patch. I am ok with the changes needed here if sdefresne is. I'd prefer not to wait for all the files to be converted to ARC before we land this as this CL will rot quickly as files are moved/renamed and the prefix is changed, which will happen soon.
lgtm https://codereview.chromium.org/2665333002/diff/100001/ios/web_view/BUILD.gn File ios/web_view/BUILD.gn (right): https://codereview.chromium.org/2665333002/diff/100001/ios/web_view/BUILD.gn#... ios/web_view/BUILD.gn:19: copy("web_view") { Note: if you want to support fat builds, this may be the correct place to run lipo (change the copy target to an action target that depends on //ios/web_view/internal:lib_web_view($toolchain) for all toolchain of the fat build). Best done in a followup CL.
https://codereview.chromium.org/2665333002/diff/100001/ios/web_view/BUILD.gn File ios/web_view/BUILD.gn (right): https://codereview.chromium.org/2665333002/diff/100001/ios/web_view/BUILD.gn#... ios/web_view/BUILD.gn:19: copy("web_view") { On 2017/02/10 09:58:15, sdefresne wrote: > Note: if you want to support fat builds, this may be the correct place to run > lipo (change the copy target to an action target that depends on > //ios/web_view/internal:lib_web_view($toolchain) for all toolchain of the fat > build). Best done in a followup CL. Acknowledged.
The CQ bit was checked by ichikawa@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeldo@chromium.org Link to the patchset: https://codereview.chromium.org/2665333002/#ps100001 (title: "Make it work after the separation of ARC-enabled/disabled targets.")
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
Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Unfortunately the test has failed, and I won't have time to fix it before my vacation. Sorry for inconvenience, but please take it over by copying the CL, or just leave it until I come back. 2017年2月10日(金) 19:08 commit-bot@chromium.org via codereview.chromium.org < reply@chromiumcodereview-hr.appspotmail.com>: > Try jobs failed on following builders: > ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco... > ) > > https://codereview.chromium.org/2665333002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/10 14:20:51, chromium-reviews wrote: > Unfortunately the test has failed, and I won't have time to fix it before > my vacation. Sorry for inconvenience, but please take it over by copying > the CL, or just leave it until I come back. > I rebased this CL here: https://codereview.chromium.org/2690913004/, but I never got the chance to fix the compile error ("ld: scattered reloc r_address too large for architecture i386"), please feel free to take back work on this CL.
The CQ bit was checked by ichikawa@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeldo@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2665333002/#ps120001 (title: "Rebase.")
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
Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
On 2017/02/27 00:01:45, michaeldo wrote: > On 2017/02/10 14:20:51, chromium-reviews wrote: > > Unfortunately the test has failed, and I won't have time to fix it before > > my vacation. Sorry for inconvenience, but please take it over by copying > > the CL, or just leave it until I come back. > > > > I rebased this CL here: https://codereview.chromium.org/2690913004/, but I never > got the chance to fix the compile error ("ld: scattered reloc r_address too > large for architecture i386"), please feel free to take back work on this CL. Hmm I googled the error message but I couldn't find a solution. I only found that it can happen when building a large library with OSX. And the error only happens with commit bot, not when I build it locally. Maybe commit bot uses an older SDK? (I use XCode 8.2.1) Is it possible to exclude a specific target from the presubmit check, because it's not a real error? We can let web_view_shell directly link to libweb_view_incomplete so that we can still test the functionality. Otherwise maybe we should switch to dynamic framework for now? We may not care much weather to use a static library or a dynamic framework. We may revisit this issue again after we conclude that we really want to go with a static library.
Decided to switch to dynamic framework for now because we couldn't easily find a solution for the build issue above. https://codereview.chromium.org/2745653010/ It's possible to revisit this CL again later, though.
Message was sent while issue was closed.
On 2017/03/13 03:40:44, Hiroshi Ichikawa wrote: > Decided to switch to dynamic framework for now because we couldn't easily find a > solution for the build issue above. > https://codereview.chromium.org/2745653010/ > > It's possible to revisit this CL again later, though. FWIW I've got the same error trying to build cronet as static library (https://codereview.chromium.org/2807283002): ld: scattered reloc r_address too large for architecture i386 It happens only in build for x86 in Debug iphonesimulator configuration. All other configurations (release, x64, arm, arm64) seem to be ok.
Message was sent while issue was closed.
On 2017/04/11 20:50:26, mef wrote: > On 2017/03/13 03:40:44, Hiroshi Ichikawa wrote: > > Decided to switch to dynamic framework for now because we couldn't easily find > a > > solution for the build issue above. > > https://codereview.chromium.org/2745653010/ > > > > It's possible to revisit this CL again later, though. > > > FWIW I've got the same error trying to build cronet as static library > (https://codereview.chromium.org/2807283002): > > ld: scattered reloc r_address too large for architecture i386 > > It happens only in build for x86 in Debug iphonesimulator configuration. > All other configurations (release, x64, arm, arm64) seem to be ok. Does it happen only on trybot, or does it happen on local build too? For me, it worked fine with local build, and the issue happened only on trybot. If it's also the case for you, one possible workaround is to stop integrating hide_symbols.py into the BUILD.gn file, and run the script manually locally to build the static library. It's not a great solution, though. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
