Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1806513002:
Add config to enable ARC on iOS/OS X to //build/config. (Closed)
DescriptionAdd config to enable ARC on iOS/OS X to //build/config.
Add a new config //build/config/compiler:enable_arc that adds the
required flag to enable ARC (automatic reference counting).
BUG=594519
Committed: https://crrev.com/2461a73bb77217ef3bbb35a3f32ab5e73eae640b
Cr-Commit-Position: refs/heads/master@{#381686}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Remove //build/config/mac:macosx_version_min_10_9 config #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 19 (5 generated)
sdefresne@chromium.org changed reviewers: + dpranke@chromium.org
Please take a look.
dpranke@chromium.org changed reviewers: + erikchen@chromium.org, thakis@chromium.org
+erikchen, +thakis for thoughts on the new configs ... https://codereview.chromium.org/1806513002/diff/1/build/config/mac/BUILD.gn File build/config/mac/BUILD.gn (right): https://codereview.chromium.org/1806513002/diff/1/build/config/mac/BUILD.gn#n... build/config/mac/BUILD.gn:98: } Is this actually used in this CL? This config makes me a bit nervous, in that I don't much like the idea of potentially doing a single toolchain build that produces targets that require two different min versions of the OS. Also this flag is set by default on all targets in the :runtime_library config, and I don't like having the flag set twice on the command line if we can afford it. Can we simply require macosx-version-min=10.9 if we're doing host toolchain builds in an ios build? Then we could change the default in //build_overrides/build.gni (which is pulled in by mac_sdk.gni to create the $mac_deployment_target that is used).
https://codereview.chromium.org/1806513002/diff/1/build/config/mac/BUILD.gn File build/config/mac/BUILD.gn (right): https://codereview.chromium.org/1806513002/diff/1/build/config/mac/BUILD.gn#n... build/config/mac/BUILD.gn:92: # than that. Your comment doesn't match the behavior of your code. """ On Mac, some tools needs to be build with Mac OS X 10.9 or greater SDK. """ Currently, on Mac, everything should build against the 10.10 SDK. -mmacosx-version-min is the deployment target (the lowest version of OSX that the target will run on). https://codereview.chromium.org/1806513002/diff/1/build/config/mac/BUILD.gn#n... build/config/mac/BUILD.gn:98: } On 2016/03/16 01:34:30, Dirk Pranke wrote: > Is this actually used in this CL? > > This config makes me a bit nervous, in that I don't much like the idea of > potentially doing a single toolchain build that produces targets that require > two different min versions of the OS. Also this flag is set by default on all > targets in the :runtime_library config, and I don't like having the flag set > twice on the command line if we can afford it. > > Can we simply require macosx-version-min=10.9 if we're doing host toolchain > builds in an ios build? > > Then we could change the default in //build_overrides/build.gni (which is pulled > in by mac_sdk.gni to create the $mac_deployment_target that is used). I assume this gets used in a private, downstream repository? The flag could theoretically be set twice, with two different values, which definitely seems wrong. Finding a way to update/override mac_deployment_target seems like the right approach.
https://codereview.chromium.org/1806513002/diff/1/build/config/mac/BUILD.gn File build/config/mac/BUILD.gn (right): https://codereview.chromium.org/1806513002/diff/1/build/config/mac/BUILD.gn#n... build/config/mac/BUILD.gn:98: } On 2016/03/16 01:59:22, erikchen wrote: > On 2016/03/16 01:34:30, Dirk Pranke wrote: > > Is this actually used in this CL? > > > > This config makes me a bit nervous, in that I don't much like the idea of > > potentially doing a single toolchain build that produces targets that require > > two different min versions of the OS. Also this flag is set by default on all > > targets in the :runtime_library config, and I don't like having the flag set > > twice on the command line if we can afford it. > > > > Can we simply require macosx-version-min=10.9 if we're doing host toolchain > > builds in an ios build? > > > > Then we could change the default in //build_overrides/build.gni (which is > pulled > > in by mac_sdk.gni to create the $mac_deployment_target that is used). > > I assume this gets used in a private, downstream repository? You're asking about the variable that is defined in //build_overrides/build.gni ? That's done so that we can set this in different upstream repos, actually, that (re-)use //build (webrtc, in this case, which requires a newer version in their standalone builds).
https://codereview.chromium.org/1806513002/diff/1/build/config/mac/BUILD.gn File build/config/mac/BUILD.gn (right): https://codereview.chromium.org/1806513002/diff/1/build/config/mac/BUILD.gn#n... build/config/mac/BUILD.gn:98: } On 2016/03/16 at 02:25:38, Dirk Pranke wrote: > On 2016/03/16 01:59:22, erikchen wrote: > > On 2016/03/16 01:34:30, Dirk Pranke wrote: > > > Is this actually used in this CL? > > > > > > This config makes me a bit nervous, in that I don't much like the idea of > > > potentially doing a single toolchain build that produces targets that require > > > two different min versions of the OS. Also this flag is set by default on all > > > targets in the :runtime_library config, and I don't like having the flag set > > > twice on the command line if we can afford it. > > > > > > Can we simply require macosx-version-min=10.9 if we're doing host toolchain > > > builds in an ios build? > > > > > > Then we could change the default in //build_overrides/build.gni (which is > > pulled > > > in by mac_sdk.gni to create the $mac_deployment_target that is used). > > > > I assume this gets used in a private, downstream repository? > > You're asking about the variable that is defined in //build_overrides/build.gni ? > > That's done so that we can set this in different upstream repos, actually, that (re-)use //build (webrtc, in this case, which requires a newer version in their standalone builds). I don't like it either, but it is required to get iossim and class-dump to compile. It is used in those two depends CLs, in those new files: https://codereview.chromium.org/1806523002/diff/1/third_party/class-dump/BUIL... https://codereview.chromium.org/1797253002/diff/1/testing/iossim/BUILD.gn Neither tool depends on anything from Chromium, so I can instead override the -mmac-osx-version-min in the tool config directly instead of putting it there in the more common location. I did put it there in order to follow DRY, but if you think it is more dangerous than beneficial, then I can remove it.
Description was changed from ========== Add iOS/OS X specific config to //build/config. Add a new config //build/config/compiler:enable_arc that adds the required flag to enable ARC (automatic reference counting). Add an OS X specific config //build/config/mac:macosx_version_min_10_9 to force the OS X SDK version to 10.9 or higher. This is required by some host tools (iossim, class-dump) required by the iOS build. BUG=594519 ========== to ========== Add config to enable ARC on iOS/OS X to //build/config. Add a new config //build/config/compiler:enable_arc that adds the required flag to enable ARC (automatic reference counting). BUG=594519 ==========
I've removed //build/config/mac:macosx_version_min_10_9. As you've pointed out, it it too risky to have such a config, instead I'll add the override to each tool that need it (and will put an assert_no_deps to prevent using anything shared with Chromium to limit the risk of using different minimum SDK). Could you please take another look?
lgtm
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1806513002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1806513002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add config to enable ARC on iOS/OS X to //build/config. Add a new config //build/config/compiler:enable_arc that adds the required flag to enable ARC (automatic reference counting). BUG=594519 ========== to ========== Add config to enable ARC on iOS/OS X to //build/config. Add a new config //build/config/compiler:enable_arc that adds the required flag to enable ARC (automatic reference counting). BUG=594519 Committed: https://crrev.com/2461a73bb77217ef3bbb35a3f32ab5e73eae640b Cr-Commit-Position: refs/heads/master@{#381686} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2461a73bb77217ef3bbb35a3f32ab5e73eae640b Cr-Commit-Position: refs/heads/master@{#381686}
Message was sent while issue was closed.
Out of interest, why do iossim and class_dump need a 10.9 deployment target? (please cc me on the changes that add the flag there) https://codereview.chromium.org/1806513002/diff/20001/ios/third_party/gcdwebs... File ios/third_party/gcdwebserver/BUILD.gn (right): https://codereview.chromium.org/1806513002/diff/20001/ios/third_party/gcdwebs... ios/third_party/gcdwebserver/BUILD.gn:25: # This should be removed once all deprecation violations have been fixed. Somewhat drive-by: FWIW, I feel pretty strongly that this shouldn't have been added in the first place; see also our previous discussion of this on https://codereview.chromium.org/1180583002/ Please fix this very soon.
Message was sent while issue was closed.
https://codereview.chromium.org/1806513002/diff/20001/ios/third_party/gcdwebs... File ios/third_party/gcdwebserver/BUILD.gn (right): https://codereview.chromium.org/1806513002/diff/20001/ios/third_party/gcdwebs... ios/third_party/gcdwebserver/BUILD.gn:25: # This should be removed once all deprecation violations have been fixed. On 2016/03/18 at 21:52:07, Nico wrote: > Somewhat drive-by: FWIW, I feel pretty strongly that this shouldn't have been added in the first place; see also our previous discussion of this on https://codereview.chromium.org/1180583002/ > > Please fix this very soon. I agree. pkl@ is owning the removal of those temporary overrides.
Message was sent while issue was closed.
On 2016/03/18 at 21:52:07, thakis wrote:
> Out of interest, why do iossim and class_dump need a 10.9 deployment target?
(please cc me on the changes that add the flag there)
To be exact, they only requires 10.8 deployment target (but gyp forces 10.9 so I
used the same version for gn, see testing/iossim/iossim.gyp and
third_party/class-dump/class-dump.gyp).
For iossim, The 10.8 deployment target is required because the generated
CoreSimulator.h generated by dumping symbols from private
CoreSimulator.framework uses OS_dispatch_source which is only available from
10.8 SDK apparenly.
Without the deployment target, I'm getting the following errors building iossim:
clang_x64/gen/testing/iossim/CoreSimulator.h:35:68: error: no type or protocol
named 'OS_dispatch_queue'
- (unsigned long
long)registerNotificationHandlerOnQueue:(NSObject<OS_dispatch_queue> *)arg1
handler:(void (^)(NSDictionary *))arg2;
For class-dump, this is because the code 1. is compiled with -fobjc-arc (which
requires 10.7 or higher deployment target) and because the code uses the new
NSArray/NSDictionary subscripting syntax introducing with OS X 10.8 SDK. Without
this, we get the following kind of errors:
../../third_party/class-dump/src/Source/CDType.m:701:34: error:
'objectAtIndexedSubscript:' is partial: introduced in OS X 10.8
[-Werror,-Wpartial-availability]
CDType *thisMember = _members[index];
>
https://codereview.chromium.org/1806513002/diff/20001/ios/third_party/gcdwebs...
> File ios/third_party/gcdwebserver/BUILD.gn (right):
>
>
https://codereview.chromium.org/1806513002/diff/20001/ios/third_party/gcdwebs...
> ios/third_party/gcdwebserver/BUILD.gn:25: # This should be removed once all
deprecation violations have been fixed.
> Somewhat drive-by: FWIW, I feel pretty strongly that this shouldn't have been
added in the first place; see also our previous discussion of this on
https://codereview.chromium.org/1180583002/
>
> Please fix this very soon.
Message was sent while issue was closed.
On 2016/03/19 13:44:54, sdefresne wrote: > On 2016/03/18 at 21:52:07, thakis wrote: > > Out of interest, why do iossim and class_dump need a 10.9 deployment target? > (please cc me on the changes that add the flag there) > > To be exact, they only requires 10.8 deployment target (but gyp forces 10.9 so I > used the same version for gn, see testing/iossim/iossim.gyp and > third_party/class-dump/class-dump.gyp). > > For iossim, The 10.8 deployment target is required because the generated > CoreSimulator.h generated by dumping symbols from private > CoreSimulator.framework uses OS_dispatch_source which is only available from > 10.8 SDK apparenly. Ok. It looks like dispatch objects can be treated as ObjC objects, which requires a 10.8 deployment target: http://stackoverflow.com/questions/8618632/does-arc-support-dispatch-queues > > Without the deployment target, I'm getting the following errors building iossim: > > clang_x64/gen/testing/iossim/CoreSimulator.h:35:68: error: no type or protocol > named 'OS_dispatch_queue' > - (unsigned long > long)registerNotificationHandlerOnQueue:(NSObject<OS_dispatch_queue> *)arg1 > handler:(void (^)(NSDictionary *))arg2; > > For class-dump, this is because the code 1. is compiled with -fobjc-arc (which > requires 10.7 or higher deployment target) and because the code uses the new > NSArray/NSDictionary subscripting syntax introducing with OS X 10.8 SDK. Without > this, we get the following kind of errors: > > ../../third_party/class-dump/src/Source/CDType.m:701:34: error: > 'objectAtIndexedSubscript:' is partial: introduced in OS X 10.8 > [-Werror,-Wpartial-availability] > CDType *thisMember = _members[index]; > > > > > https://codereview.chromium.org/1806513002/diff/20001/ios/third_party/gcdwebs... > > File ios/third_party/gcdwebserver/BUILD.gn (right): > > > > > https://codereview.chromium.org/1806513002/diff/20001/ios/third_party/gcdwebs... > > ios/third_party/gcdwebserver/BUILD.gn:25: # This should be removed once all > deprecation violations have been fixed. > > Somewhat drive-by: FWIW, I feel pretty strongly that this shouldn't have been > added in the first place; see also our previous discussion of this on > https://codereview.chromium.org/1180583002/ > > > > Please fix this very soon. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
