Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1180583002:
GN updates to build base on iOS (Closed)
|
Created:
5 years, 6 months ago by Chinmay Modified:
5 years, 6 months ago CC:
abarth-chromium, chromium-reviews, erikwright+watch_chromium.org, eseidel, sdefresne Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGN updates to build base on iOS
Committed: https://crrev.com/533669d71d8ffc0a6eb74fdee1a87692b01f59bc
Cr-Commit-Position: refs/heads/master@{#334650}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address concerns raised in https://codereview.chromium.org/1180583002 #
Total comments: 5
Patch Set 3 : Address concerns raised in CL #Messages
Total messages: 27 (5 generated)
chinmaygarde@google.com changed reviewers: + dpranke@chromium.org, thakis@chromium.org
eseidel@chromium.org changed reviewers: + eseidel@chromium.org
https://codereview.chromium.org/1180583002/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1180583002/diff/1/base/BUILD.gn#newcode832 base/BUILD.gn:832: set_sources_assignment_filter([]) This is slightly sad-panda. Would be nice to know what files it's wrongly filtering.
https://codereview.chromium.org/1180583002/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1180583002/diff/1/base/BUILD.gn#newcode832 base/BUILD.gn:832: set_sources_assignment_filter([]) On 2015/06/10 at 21:42:44, eseidel wrote: > This is slightly sad-panda. Would be nice to know what files it's wrongly filtering. By default, all "mac" specific files will get filtered away. But most of them serve fine for iOS. The filter is temporarily disabled to add a subset of the mac specific file.
lgtm, but you'll probably need owners approval from Brett or a //base owner.
https://codereview.chromium.org/1180583002/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1180583002/diff/1/base/BUILD.gn#newcode17 base/BUILD.gn:17: cflags = [ "-Wno-deprecated-declarations" ] I don't see this in the gyp build files for base. How come? https://codereview.chromium.org/1180583002/diff/1/base/BUILD.gn#newcode784 base/BUILD.gn:784: ] I thought the gn way was to not add these in the first place if is_ios is true instead of adding and then removing them. https://codereview.chromium.org/1180583002/diff/1/base/BUILD.gn#newcode832 base/BUILD.gn:832: set_sources_assignment_filter([]) Since it's early for the ios gn build, it's probably better to get this right instead of putting in some temporary workaround. I think in gyp all the _mac files are used on ios by default, right? should gn just work the same? https://codereview.chromium.org/1180583002/diff/1/base/process/process_handle... File base/process/process_handle_mac.cc (right): https://codereview.chromium.org/1180583002/diff/1/base/process/process_handle... base/process/process_handle_mac.cc:7: #if !defined(OS_IOS) base builds fine for ios in the gyp build. why do you need source changes?
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
https://codereview.chromium.org/1180583002/diff/1/base/process/process_handle... File base/process/process_handle_mac.cc (right): https://codereview.chromium.org/1180583002/diff/1/base/process/process_handle... base/process/process_handle_mac.cc:7: #if !defined(OS_IOS) On 2015/06/11 15:37:59, Nico wrote: > base builds fine for ios in the gyp build. why do you need source changes? I suspect you miss some system directory with gn. Can you compare command-line when building with gn vs gyp? On my machine libproc.h is found in the SDK, so the issue may be that -isysroot is incorrectly set with gn.
https://codereview.chromium.org/1180583002/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1180583002/diff/1/base/BUILD.gn#newcode17 base/BUILD.gn:17: cflags = [ "-Wno-deprecated-declarations" ] On 2015/06/11 at 15:37:59, Nico wrote: > I don't see this in the gyp build files for base. How come? The Sky builds set the target to 8.0 manually. Do you have suggestions on a more elegant fix? Admittedly, this is a very hacky workaround. https://codereview.chromium.org/1180583002/diff/1/base/BUILD.gn#newcode832 base/BUILD.gn:832: set_sources_assignment_filter([]) On 2015/06/11 at 15:37:59, Nico wrote: > Since it's early for the ios gn build, it's probably better to get this right instead of putting in some temporary workaround. I think in gyp all the _mac files are used on ios by default, right? should gn just work the same? In the GN system, "mac" files are not built by default on "ios" (and are filtered away). I am not sure what the GN way is regarding these rules. But a majority (though not all) of the "mac" files are fine for use on iOS. I am not sure what the elegant way to do this would be though. Since there are always going to be exceptions for both Desktop Mac and iOS builds. All the existing GN files treat the "is_mac" case as if it was "is_mac_desktop". https://codereview.chromium.org/1180583002/diff/1/base/process/process_handle... File base/process/process_handle_mac.cc (right): https://codereview.chromium.org/1180583002/diff/1/base/process/process_handle... base/process/process_handle_mac.cc:7: #if !defined(OS_IOS) On 2015/06/11 at 17:05:17, sdefresne wrote: > On 2015/06/11 15:37:59, Nico wrote: > > base builds fine for ios in the gyp build. why do you need source changes? > > I suspect you miss some system directory with gn. Can you compare command-line when building with gn vs gyp? On my machine libproc.h is found in the SDK, so the issue may be that -isysroot is incorrectly set with gn. libproc.h is only present on the iPhone Simulator SDK. I am not sure if it was present once for the device and subsequently removed. I have been scouring the API changelogs but cant find any mention of the same. Sky builds use the iOS 8 SDK. I found a relevant Radar http://www.openradar.appspot.com/12934352. Looking for libproc.h yields ``` ➜ Xcode.app find /Applications/Xcode.app | grep libproc.h$ /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk/usr/include/libproc.h /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/usr/include/libproc.h /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/include/libproc.h ``` I just re-verified the sysroot flags too. They seem to be in order.
https://codereview.chromium.org/1180583002/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1180583002/diff/1/base/BUILD.gn#newcode832 base/BUILD.gn:832: set_sources_assignment_filter([]) On 2015/06/11 17:44:35, Chinmay wrote: > On 2015/06/11 at 15:37:59, Nico wrote: > > Since it's early for the ios gn build, it's probably better to get this right > instead of putting in some temporary workaround. I think in gyp all the _mac > files are used on ios by default, right? should gn just work the same? > > In the GN system, "mac" files are not built by default on "ios" (and are > filtered away). I am not sure what the GN way is regarding these rules. But a > majority (though not all) of the "mac" files are fine for use on iOS. I am not > sure what the elegant way to do this would be though. Since there are always > going to be exceptions for both Desktop Mac and iOS builds. All the existing GN > files treat the "is_mac" case as if it was "is_mac_desktop". According to build/filename_rules.gypi, we do not build _mac.* files on iOS either when using gyp and we use sources/ include rules to add those files we do want to build, so I think it make sense to do the same thing with gn. https://codereview.chromium.org/1180583002/diff/1/base/process/BUILD.gn File base/process/BUILD.gn (right): https://codereview.chromium.org/1180583002/diff/1/base/process/BUILD.gn#newco... base/process/BUILD.gn:107: "process_handle_mac.cc", I think we don't build those two files on iOS when using gyp: $ ninja -C out/Debug-iphoneos -v -n base|grep process_iterator_mac $ ninja -C out/Debug-iphoneos -v -n base|grep process_handle_mac https://codereview.chromium.org/1180583002/diff/1/base/process/process_handle... File base/process/process_handle_mac.cc (right): https://codereview.chromium.org/1180583002/diff/1/base/process/process_handle... base/process/process_handle_mac.cc:7: #if !defined(OS_IOS) On 2015/06/11 17:44:36, Chinmay wrote: > On 2015/06/11 at 17:05:17, sdefresne wrote: > > On 2015/06/11 15:37:59, Nico wrote: > > > base builds fine for ios in the gyp build. why do you need source changes? > > > > I suspect you miss some system directory with gn. Can you compare command-line > when building with gn vs gyp? On my machine libproc.h is found in the SDK, so > the issue may be that -isysroot is incorrectly set with gn. > > libproc.h is only present on the iPhone Simulator SDK. I am not sure if it was > present once for the device and subsequently removed. I have been scouring the > API changelogs but cant find any mention of the same. Sky builds use the iOS 8 > SDK. I found a relevant Radar http://www.openradar.appspot.com/12934352. Looking > for libproc.h yields > > ``` > ➜ Xcode.app find /Applications/Xcode.app | grep libproc.h$ > /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk/usr/include/libproc.h > /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/usr/include/libproc.h > /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/include/libproc.h > > ``` > > I just re-verified the sysroot flags too. They seem to be in order. I've checked my gyp build and process_handle_mac.cc is not build when building for iOS (neither for simulator nor device): $ ninja -C out/Debug-iphoneos -v -n base|grep process_handle_mac $ ninja -C out/Debug-iphonesimulator -v -n base|grep process_handle_mac
If gn's filename rules don't match gyp's, they should probably be changed to match gyp… https://code.google.com/p/chromium/codesearch#chromium/src/build/filename_rul... https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD... …hm, that seems to match. mac files aren't included by default in gyp either from what i can tell. I can't see anything in a gyp file that includes base_paths_mac.mm in ios builds in gyp (am I just missing it?) Why do you need this file in the gn build? (In general, I think the gyp build should be considered source of truth at the moment, and the gn build should match it. This implies that you shouldn't need any source changes to bring up targets.)
On 2015/06/11 18:02:34, Nico wrote: > (In general, I think the gyp build should be considered source of truth at the > moment, and the gn build should match it. This implies that you shouldn't need > any source changes to bring up targets.) I would generally agree that we shouldn't need source changes to bring up targets, but there have certainly been cases where we've chosen to fix the source instead of working around problems in the source through the build system. I don't have any examples I can point to offhand, but I can definitely dig up some if need be. However, the converse is not true, and you usually should not need to change the source in order to work around a problem in the build system; perhaps that's more what you're referring to here?
On 2015/06/11 at 18:29:04, dpranke wrote: > On 2015/06/11 18:02:34, Nico wrote: > > (In general, I think the gyp build should be considered source of truth at the > > moment, and the gn build should match it. This implies that you shouldn't need > > any source changes to bring up targets.) > > I would generally agree that we shouldn't need source changes to bring up targets, > but there have certainly been cases where we've chosen to fix the source instead of > working around problems in the source through the build system. > > I don't have any examples I can point to offhand, but I can definitely dig up some > if need be. > > However, the converse is not true, and you usually should not need to change the > source in order to work around a problem in the build system; perhaps that's > more what you're referring to here? I have updated the patch to more closely match the GYP builds. The patch now makes no source changes.
https://codereview.chromium.org/1180583002/diff/20001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1180583002/diff/20001/base/BUILD.gn#newcode17 base/BUILD.gn:17: cflags = [ "-Wno-deprecated-declarations" ] This isn't in gyp. Did I understand correctly that you're using a different iOS SDK version than regular chromium/ios? (I'm pretty sure they use the iOS 8 SDK too). Can you fix the deprecation warnings instead of suppressing them? https://codereview.chromium.org/1180583002/diff/20001/base/BUILD.gn#newcode836 base/BUILD.gn:836: "base_paths_mac.mm", I still don't see this included in the gyp files
https://codereview.chromium.org/1180583002/diff/20001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1180583002/diff/20001/base/BUILD.gn#newcode17 base/BUILD.gn:17: cflags = [ "-Wno-deprecated-declarations" ] On 2015/06/11 at 21:45:04, Nico wrote: > This isn't in gyp. Did I understand correctly that you're using a different iOS SDK version than regular chromium/ios? (I'm pretty sure they use the iOS 8 SDK too). > > Can you fix the deprecation warnings instead of suppressing them? Disabling `-Wno-deprecated-declarations` gives me errors regarding deprecation of methods related to `CFGregorianDate` in `base/third_party/nspr/prtime.cc`. Both that file and base_paths_mac.mm mentioned below are present in base.gypi as part of the target sources. prtime.cc in particular does not seem to be part of a separate target (on which the warning flags might be specifically disabled to get around this issue). So I am now rather confused about how gyp uses the iOS 8 SDK successfully. Fixing the use of deprecated methods seemed like a yak shaving exercise given my larger goal of trying to get a build of Sky running on iOS (which this patch allows me to do). But I understand if you feel like this is cutting a corner. Let me know if this is a blocker and I can work on avoiding the use of `CFGregorianDate` and friends.
On 2015/06/11 22:32:16, Chinmay wrote: > https://codereview.chromium.org/1180583002/diff/20001/base/BUILD.gn > File base/BUILD.gn (right): > > https://codereview.chromium.org/1180583002/diff/20001/base/BUILD.gn#newcode17 > base/BUILD.gn:17: cflags = [ "-Wno-deprecated-declarations" ] > On 2015/06/11 at 21:45:04, Nico wrote: > > This isn't in gyp. Did I understand correctly that you're using a different > iOS SDK version than regular chromium/ios? (I'm pretty sure they use the iOS 8 > SDK too). > > > > Can you fix the deprecation warnings instead of suppressing them? > > Disabling `-Wno-deprecated-declarations` gives me errors regarding deprecation > of methods related to `CFGregorianDate` in `base/third_party/nspr/prtime.cc`. > Both that file and base_paths_mac.mm mentioned below are present in base.gypi as > part of the target sources. But files called _mac are filtered out automatically for ios builds in gyp, no? > prtime.cc in particular does not seem to be part of > a separate target (on which the warning flags might be specifically disabled to > get around this issue). So I am now rather confused about how gyp uses the iOS 8 > SDK successfully. Fixing the use of deprecated methods seemed like a yak shaving > exercise given my larger goal of trying to get a build of Sky running on iOS > (which this patch allows me to do). But I understand if you feel like this is > cutting a corner. Let me know if this is a blocker and I can work on avoiding > the use of `CFGregorianDate` and friends.
On 2015/06/11 at 22:52:10, thakis wrote: > On 2015/06/11 22:32:16, Chinmay wrote: > > https://codereview.chromium.org/1180583002/diff/20001/base/BUILD.gn > > File base/BUILD.gn (right): > > > > https://codereview.chromium.org/1180583002/diff/20001/base/BUILD.gn#newcode17 > > base/BUILD.gn:17: cflags = [ "-Wno-deprecated-declarations" ] > > On 2015/06/11 at 21:45:04, Nico wrote: > > > This isn't in gyp. Did I understand correctly that you're using a different > > iOS SDK version than regular chromium/ios? (I'm pretty sure they use the iOS 8 > > SDK too). > > > > > > Can you fix the deprecation warnings instead of suppressing them? > > > > Disabling `-Wno-deprecated-declarations` gives me errors regarding deprecation > > of methods related to `CFGregorianDate` in `base/third_party/nspr/prtime.cc`. > > Both that file and base_paths_mac.mm mentioned below are present in base.gypi as > > part of the target sources. > > But files called _mac are filtered out automatically for ios builds in gyp, no? > > > prtime.cc in particular does not seem to be part of > > a separate target (on which the warning flags might be specifically disabled to > > get around this issue). So I am now rather confused about how gyp uses the iOS 8 > > SDK successfully. Fixing the use of deprecated methods seemed like a yak shaving > > exercise given my larger goal of trying to get a build of Sky running on iOS > > (which this patch allows me to do). But I understand if you feel like this is > > cutting a corner. Let me know if this is a blocker and I can work on avoiding > > the use of `CFGregorianDate` and friends. You are correct, _mac files are filtered out automatically. But the base.gypi files seems to be explicitly adding it back https://code.google.com/p/chromium/codesearch#chromium/src/base/base.gypi&q=b...
Cool, I missed that, thanks. The only thing left I don't understand is the deprecation warning. The chromium ios bots do build with the 8.3 sdk (see "-isysroot /Applications/Xcode6.3.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator8.3.sdk" in http://build.chromium.org/p/chromium.mac/builders/iOS_Simulator_%28dbg%29/bui... for example). What's your deployment target? I think that can affect deprecation messages too. Our bots use IPHONEOS_DEPLOYMENT_TARGET=7.0, do you use something higher? If so, could you use 7.0 as deployment target? https://codereview.chromium.org/1180583002/diff/20001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1180583002/diff/20001/base/BUILD.gn#newcode834 base/BUILD.gn:834: sources += [ Do you want to add atomicops_internals_mac.h too? (The gyp files do this) https://codereview.chromium.org/1180583002/diff/20001/base/BUILD.gn#newcode843 base/BUILD.gn:843: "mac/mach_logging.h", The gyp file also adds objc_property_releaser.h/mm …in general, the section you linked to lists several more files that you don't list here.
On 2015/06/12 at 00:42:08, thakis wrote: > Cool, I missed that, thanks. > > The only thing left I don't understand is the deprecation warning. The chromium ios bots do build with the 8.3 sdk (see "-isysroot /Applications/Xcode6.3.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator8.3.sdk" in http://build.chromium.org/p/chromium.mac/builders/iOS_Simulator_%28dbg%29/bui... for example). > > What's your deployment target? I think that can affect deprecation messages too. Our bots use IPHONEOS_DEPLOYMENT_TARGET=7.0, do you use something higher? If so, could you use 7.0 as deployment target? > > https://codereview.chromium.org/1180583002/diff/20001/base/BUILD.gn > File base/BUILD.gn (right): > > https://codereview.chromium.org/1180583002/diff/20001/base/BUILD.gn#newcode834 > base/BUILD.gn:834: sources += [ > Do you want to add atomicops_internals_mac.h too? (The gyp files do this) > > https://codereview.chromium.org/1180583002/diff/20001/base/BUILD.gn#newcode843 > base/BUILD.gn:843: "mac/mach_logging.h", > The gyp file also adds objc_property_releaser.h/mm > > …in general, the section you linked to lists several more files that you don't list here. I reduced the deployment target to 7.0. Excellent catch on the missing files that needed be manually added. It was building as a component so I was working off of linker issues only. Patch has been updated.
Nico, can you take another look at this?
lgtm!
The CQ bit was checked by chinmaygarde@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/1180583002/#ps40001 (title: "Address concerns raised in CL")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180583002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/533669d71d8ffc0a6eb74fdee1a87692b01f59bc Cr-Commit-Position: refs/heads/master@{#334650} |
