[ObjC ARC] Converts ios/chrome/test/earl_grey:ios_chrome_device_check_egtests to ARC.

[ObjC ARC] Converts ios/chrome/test/earl_grey:ios_chrome_device_check_egtests to ARC.

The automatic arc script doesn't work for this target because it simply
adds configs += [ "//build/config/compiler:enable_arc" ] to 
chrome_ios_eg_test("ios_chrome_device_check_egtests"), however, the 
body of a  template invocation doesn't take configs.

This CL manually moves "device_check_egtest.mm" to a separate 
source_set and adds configs += [ "//build/config/compiler:enable_arc" ]
to that source_set.

Notable issues:None
BUG=624363
TEST=None

Review-Url: https://codereview.chromium.org/2882253002
Cr-Commit-Position: refs/heads/master@{#472430}
Committed: https://chromium.googlesource.com/chromium/src/+/8677520a8befbd56e37e6a864ec395eb9146f540

[liaoyuke, 2017-05-15 17:23:35]

The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 17:24:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 20:56:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 20:56:08]

Dry run: This issue passed the CQ dry run.

[liaoyuke, 2017-05-16 06:06:17]

Description was changed from

==========
[ObjC ARC] Converts ios/chrome/test/earl_grey:ios_chrome_device_check_egtests to
ARC.

Automatically generated ARCMigrate commit
Notable issues:None
BUG=624363
TEST=None
==========

to

==========
[ObjC ARC] Converts ios/chrome/test/earl_grey:ios_chrome_device_check_egtests to
ARC.

The automatic arc script doesn't work for this target because it simply
adds configs += [ "//build/config/compiler:enable_arc" ] to 
chrome_ios_eg_test("ios_chrome_device_check_egtests"), however, the 
body of a  template invocation doesn't take configs.

This CL manually moves "device_check_egtest.mm" to a separate 
source_set and adds configs += [ "//build/config/compiler:enable_arc" ]
to that source_set.

Notable issues:None
BUG=624363
TEST=None
==========

[liaoyuke, 2017-05-16 06:06:18]

liaoyuke@chromium.org changed reviewers:
+ sdefresne@chromium.org, stkhapugin@chromium.org

[liaoyuke, 2017-05-16 06:06:58]

Hi Stepan, Sylvain,

PTAL. Please let me know if you can come up with a better solution.

Thank you very much!

[sdefresne, 2017-05-16 08:26:20]

https://codereview.chromium.org/2882253002/diff/1/ios/chrome/test/earl_grey/B...
File ios/chrome/test/earl_grey/BUILD.gn (right):

https://codereview.chromium.org/2882253002/diff/1/ios/chrome/test/earl_grey/B...
ios/chrome/test/earl_grey/BUILD.gn:101:
chrome_ios_eg_test("ios_chrome_device_check_egtests") {
I think a better change would be to allow overriding "configs" for
"chrome_ios_eg_tests" template. You should add the following to
ios/chrome/test/earl_grey/chrome_ios_eg_test.gni, below the definition of
"chrome_ios_eg_test" template:

set_defaults("chrome_ios_eg_test") {
  configs = default_executable_configs
}

That should be enough to allow you to change this file to the following:

chrome_ios_eg_test("ios_chrome_device_check_egtests") {
  configs += [ "//build/config/compiler:enable_arc" ]
  sources = [
    "device_check_egtest.mm",
  ]
  deps = [
    ":test_support",
    "//ios/third_party/earl_grey",
    "//url",
  ]
}

[sdefresne, 2017-05-16 08:27:17]

https://codereview.chromium.org/2882253002/diff/1/ios/chrome/test/earl_grey/B...
File ios/chrome/test/earl_grey/BUILD.gn (right):

https://codereview.chromium.org/2882253002/diff/1/ios/chrome/test/earl_grey/B...
ios/chrome/test/earl_grey/BUILD.gn:110: "device_check_egtest.mm",
This CL needs to add guards to "device_check_egtest.mm" file to prevent
compiling it without ARC.

[liaoyuke, 2017-05-16 16:02:48]

Patchset #2 (id:20001) has been deleted

[liaoyuke, 2017-05-16 16:08:31]

Hi Sylvain,

PTAL. Thank you very much!

https://codereview.chromium.org/2882253002/diff/1/ios/chrome/test/earl_grey/B...
File ios/chrome/test/earl_grey/BUILD.gn (right):

https://codereview.chromium.org/2882253002/diff/1/ios/chrome/test/earl_grey/B...
ios/chrome/test/earl_grey/BUILD.gn:101:
chrome_ios_eg_test("ios_chrome_device_check_egtests") {
On 2017/05/16 08:26:20, sdefresne wrote:
> I think a better change would be to allow overriding "configs" for
> "chrome_ios_eg_tests" template. You should add the following to
> ios/chrome/test/earl_grey/chrome_ios_eg_test.gni, below the definition of
> "chrome_ios_eg_test" template:
> 
> set_defaults("chrome_ios_eg_test") {
>   configs = default_executable_configs
> }
> 
> That should be enough to allow you to change this file to the following:
> 
> chrome_ios_eg_test("ios_chrome_device_check_egtests") {
>   configs += [ "//build/config/compiler:enable_arc" ]
>   sources = [
>     "device_check_egtest.mm",
>   ]
>   deps = [
>     ":test_support",
>     "//ios/third_party/earl_grey",
>     "//url",
>   ]
> }

Hi Sylvain,

Thank you for the detailed explanation!

I've updated the CL according to your comments, however, it seems that [
"//build/config/compiler:enable_arc" ] still doesn't apply to
"device_check_egtest.mm".

I debugged a little bit, but couldn't understand, do you have any thoughts?

https://codereview.chromium.org/2882253002/diff/1/ios/chrome/test/earl_grey/B...
ios/chrome/test/earl_grey/BUILD.gn:110: "device_check_egtest.mm",
On 2017/05/16 08:27:17, sdefresne wrote:
> This CL needs to add guards to "device_check_egtest.mm" file to prevent
> compiling it without ARC.

Done.

[sdefresne, 2017-05-17 09:20:53]

The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 09:21:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sdefresne, 2017-05-17 09:25:17]

On 2017/05/16 16:08:31, liaoyuke wrote:
> Hi Sylvain,
> 
> PTAL. Thank you very much!
> 
>
https://codereview.chromium.org/2882253002/diff/1/ios/chrome/test/earl_grey/B...
> File ios/chrome/test/earl_grey/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2882253002/diff/1/ios/chrome/test/earl_grey/B...
> ios/chrome/test/earl_grey/BUILD.gn:101:
> chrome_ios_eg_test("ios_chrome_device_check_egtests") {
> On 2017/05/16 08:26:20, sdefresne wrote:
> > I think a better change would be to allow overriding "configs" for
> > "chrome_ios_eg_tests" template. You should add the following to
> > ios/chrome/test/earl_grey/chrome_ios_eg_test.gni, below the definition of
> > "chrome_ios_eg_test" template:
> > 
> > set_defaults("chrome_ios_eg_test") {
> >   configs = default_executable_configs
> > }
> > 
> > That should be enough to allow you to change this file to the following:
> > 
> > chrome_ios_eg_test("ios_chrome_device_check_egtests") {
> >   configs += [ "//build/config/compiler:enable_arc" ]
> >   sources = [
> >     "device_check_egtest.mm",
> >   ]
> >   deps = [
> >     ":test_support",
> >     "//ios/third_party/earl_grey",
> >     "//url",
> >   ]
> > }
> 
> Hi Sylvain,
> 
> Thank you for the detailed explanation!
> 
> I've updated the CL according to your comments, however, it seems that [
> "//build/config/compiler:enable_arc" ] still doesn't apply to
> "device_check_egtest.mm".
> 
> I debugged a little bit, but couldn't understand, do you have any thoughts?

I'm unable to reproduce your error. With your patch applied, I'm able to confirm
that -fobjc-arc is passed to the compiler when device_check_egtest.mm is
compiled:

$ git fetch origin
$ git checkout origin/master
$ git cl patch 2882253002
$ ninja -C out/Debug-iphonesimulator
'../../ios/chrome/test/earl_grey/device_check_egtest.mm^' -v
[1/1] /Users/sdefresne/Tools/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/ios/chrome/test/earl_grey/ios_chrome_device_check_egtests_arch_executable_sources/device_check_egtest.o.d
-DV8_DEPRECATION_WARNINGS -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD
-DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=\"301384-2\"
-DCR_XCODE_VERSION=0831 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_DEBUG
-DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -DGTEST_API_=
-DGTEST_HAS_POSIX_RE=0 -DGTEST_LANG_CXX11=1 -I../.. -Igen
-I../../third_party/googletest/src/googletest/include
-I../../third_party/libwebp -I../../ios/third_party/earl_grey/src/EarlGrey
-fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector
-Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__=
-fcolor-diagnostics -arch x86_64 -Wall -Werror -Wextra -Wundeclared-selector
-Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing
-Wno-covered-switch-default -Wno-unneeded-internal-declaration
-Wno-inconsistent-missing-override -Wno-undefined-var-template
-Wno-nonportable-include-path -Wno-address-of-packed-member
-Wno-unused-lambda-capture -Wno-user-defined-warnings -O0
-fno-omit-frame-pointer -gdwarf-2 -isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator10.3.sdk
-stdlib=libc++ -mios-simulator-version-min=9.0 -fvisibility=hidden -Xclang -load
-Xclang
../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-Xclang -add-plugin -Xclang find-bad-constructs -Xclang
-plugin-arg-find-bad-constructs -Xclang check-auto-raw-pointer -Wheader-hygiene
-Wstring-conversion -Wtautological-overlap-compare -F
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/Library/Frameworks
-F . -F . -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare
-std=c++11 -fobjc-call-cxx-cdtors -Wobjc-missing-property-synthesis -fno-rtti
-fno-exceptions -fobjc-arc -c
../../ios/chrome/test/earl_grey/device_check_egtest.mm -o
obj/ios/chrome/test/earl_grey/ios_chrome_device_check_egtests_arch_executable_sources/device_check_egtest.o

BTW, if the parameter -fobjc-arc was not passed when the file is compiled, then
compilation would fail due to the ARC guards that have been added to the file,
but the compilation of ios_chrome_device_check_egtests do succeed with the patch
applied:

$ ninja -C out/Debug-iphonesimulator ios_chrome_device_check_egtests
[3916/3916] STAMP
obj/ios/chrome/test/earl_grey/ios_chrome_device_check_egtests.stamp

>
https://codereview.chromium.org/2882253002/diff/1/ios/chrome/test/earl_grey/B...
> ios/chrome/test/earl_grey/BUILD.gn:110: "device_check_egtest.mm",
> On 2017/05/16 08:27:17, sdefresne wrote:
> > This CL needs to add guards to "device_check_egtest.mm" file to prevent
> > compiling it without ARC.
> 
> Done.

[sdefresne, 2017-05-17 09:28:34]

lgtm

[sdefresne, 2017-05-17 09:28:38]

The CQ bit was unchecked by sdefresne@chromium.org

[sdefresne, 2017-05-17 09:28:41]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2017-05-17 09:28:55]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 12:39:24]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495013321145670,
"parent_rev": "512409a928840e8076cc67e8c739f72b58bc9fe2", "commit_rev":
"8677520a8befbd56e37e6a864ec395eb9146f540"}

[commit-bot: I haz the power, 2017-05-17 12:39:34]

Description was changed from

==========
[ObjC ARC] Converts ios/chrome/test/earl_grey:ios_chrome_device_check_egtests to
ARC.

The automatic arc script doesn't work for this target because it simply
adds configs += [ "//build/config/compiler:enable_arc" ] to 
chrome_ios_eg_test("ios_chrome_device_check_egtests"), however, the 
body of a  template invocation doesn't take configs.

This CL manually moves "device_check_egtest.mm" to a separate 
source_set and adds configs += [ "//build/config/compiler:enable_arc" ]
to that source_set.

Notable issues:None
BUG=624363
TEST=None
==========

to

==========
[ObjC ARC] Converts ios/chrome/test/earl_grey:ios_chrome_device_check_egtests to
ARC.

The automatic arc script doesn't work for this target because it simply
adds configs += [ "//build/config/compiler:enable_arc" ] to 
chrome_ios_eg_test("ios_chrome_device_check_egtests"), however, the 
body of a  template invocation doesn't take configs.

This CL manually moves "device_check_egtest.mm" to a separate 
source_set and adds configs += [ "//build/config/compiler:enable_arc" ]
to that source_set.

Notable issues:None
BUG=624363
TEST=None

Review-Url: https://codereview.chromium.org/2882253002
Cr-Commit-Position: refs/heads/master@{#472430}
Committed:
https://chromium.googlesource.com/chromium/src/+/8677520a8befbd56e37e6a864ec3...
==========

[commit-bot: I haz the power, 2017-05-17 12:39:36]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8677520a8befbd56e37e6a864ec3...

[liaoyuke, 2017-05-17 15:19:41]

On 2017/05/17 12:39:36, commit-bot: I haz the power wrote:
> Committed patchset #2 (id:40001) as
>
https://chromium.googlesource.com/chromium/src/+/8677520a8befbd56e37e6a864ec3...

hmmm, that's odd, but anyway, thank you for helping confirm that it works!

[sdefresne, 2017-05-17 15:25:16]

On 2017/05/17 15:19:41, liaoyuke wrote:
> On 2017/05/17 12:39:36, commit-bot: I haz the power wrote:
> > Committed patchset #2 (id:40001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/8677520a8befbd56e37e6a864ec3...
> 
> hmmm, that's odd, but anyway, thank you for helping confirm that it works!

There is a downstream target that independently compile this file. The target
had to be fixed as it prevented the roll to land. Maybe this is what you were
experiencing.

[liaoyuke, 2017-05-17 15:34:23]

Yeah, I think so, that clears everything! Thank you for the downstream fix!

On Wed, May 17, 2017 at 8:25 AM <sdefresne@chromium.org> wrote:

> On 2017/05/17 15:19:41, liaoyuke wrote:
> > On 2017/05/17 12:39:36, commit-bot: I haz the power wrote:
> > > Committed patchset #2 (id:40001) as
> > >
> >
>
>
https://chromium.googlesource.com/chromium/src/+/8677520a8befbd56e37e6a864ec3...
> >
> > hmmm, that's odd, but anyway, thank you for helping confirm that it
> works!
>
> There is a downstream target that independently compile this file. The
> target
> had to be fixed as it prevented the roll to land. Maybe this is what you
> were
> experiencing.
>
> https://codereview.chromium.org/2882253002/
>

-- 
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.