Don't set -fvisibility=hidden for iOS Debug.

Don't set -fvisibility=hidden for iOS Debug.

Remove -fvisibility=hidden for Debug configuration on iOS at the
top level (e.g. common.gypi), so we don't remove symbols from
dependencies.

BUG=569073

[baxley, 2015-12-11 17:28:30]

baxley@chromium.org changed reviewers:
+ dpranke@chromium.org, sdefresne@chromium.org

[baxley, 2015-12-11 17:28:30]

dpranke@ - Could you advise on where in GN I could make a change analogous to
what I have in GYP? Additionally, feel free to ask questions or mention any
issues with the GYP change. Thanks!

[Dirk Pranke, 2015-12-11 23:16:36]

On 2015/12/11 17:28:30, baxley wrote:
> dpranke@ - Could you advise on where in GN I could make a change analogous to
> what I have in GYP? Additionally, feel free to ask questions or mention any
> issues with the GYP change. Thanks!

We set -fvisibility=hidden here:

https://code.google.com/p/chromium/codesearch?q=BUILDCONFIG.gn#chromium/src/b...

so I would tweak that to be (!is_ios || is_debug)

[Dirk Pranke, 2015-12-11 23:20:06]

https://codereview.chromium.org/1514423003/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1514423003/diff/1/build/common.gypi#newcode5158
build/common.gypi:5158: '-Wno-shorten-64-to-32',
GN will only support ninja, so maybe this flag isn't needed there?

[baxley, 2015-12-12 00:33:21]

Thanks for the pointer! Are you an appropriate reviewer for this? or should I
send it to someone else?

https://codereview.chromium.org/1514423003/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1514423003/diff/1/build/common.gypi#newcode5158
build/common.gypi:5158: '-Wno-shorten-64-to-32',
On 2015/12/11 23:20:06, Dirk Pranke wrote:
> GN will only support ninja, so maybe this flag isn't needed there?

Yeah, probably not. This isn't new code, it just got moved with my condition for
iOS.

[Dirk Pranke, 2015-12-12 00:38:28]

dpranke@chromium.org changed reviewers:
+ brettw@chromium.org

[Dirk Pranke, 2015-12-12 00:38:29]

Yes, I can review this, but brettw@ needs to review the change to BUILDCONFIG.gn
as well.

lgtm

[brettw, 2015-12-12 23:53:18]

https://codereview.chromium.org/1514423003/diff/20001/build/config/BUILDCONFI...
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/1514423003/diff/20001/build/config/BUILDCONFI...
build/config/BUILDCONFIG.gn:480: [ "//build/config/gcc:symbol_visibility_hidden"
]
Let's do this: make a new config in build/config/gcc like:

config("default_symbol_visibility") {
  if (!is_posix || is_release) {
    configs = [ ":symbol_visibility_hidden" ]
  }
}

Then you can unconditionally reference the default symbol visibility from here.

Can you also write a better comment? I don't know what XCTests are or what this
might have to do with debug vs. release.

[baxley, 2015-12-14 18:46:52]

Comments addressed. Let me know if anything doesn't look right or if there are
any local tests I should run. PTAL.

https://codereview.chromium.org/1514423003/diff/20001/build/config/BUILDCONFI...
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/1514423003/diff/20001/build/config/BUILDCONFI...
build/config/BUILDCONFIG.gn:480: [ "//build/config/gcc:symbol_visibility_hidden"
]
On 2015/12/12 23:53:18, brettw wrote:
> Let's do this: make a new config in build/config/gcc like:
> 
> config("default_symbol_visibility") {
>   if (!is_posix || is_release) {
>     configs = [ ":symbol_visibility_hidden" ]
>   }
> }
> 
> Then you can unconditionally reference the default symbol visibility from
here.
> 
> Can you also write a better comment? I don't know what XCTests are or what
this
> might have to do with debug vs. release.

Done. Thanks for the detailed explanation.

[brettw, 2015-12-14 22:30:05]

lgtm

[baxley, 2015-12-14 22:41:59]

The CQ bit was checked by baxley@chromium.org

[baxley, 2015-12-14 22:41:59]

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/1514423003/#ps60001
(title: "put blank line back in")

[commit-bot: I haz the power, 2015-12-14 22:44:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1514423003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1514423003/60001

[commit-bot: I haz the power, 2015-12-14 22:51:25]

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

[commit-bot: I haz the power, 2015-12-14 22:51:26]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[bm386100, 2015-12-15 00:43:28]

bm386100@gmail.com changed reviewers:
+ bm386100@gmail.com

[baxley, 2015-12-15 15:04:51]

The CQ bit was checked by baxley@chromium.org

[baxley, 2015-12-15 15:04:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1514423003/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2015-12-15 15:05:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1514423003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1514423003/80001

[commit-bot: I haz the power, 2015-12-15 15:17:22]

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

[commit-bot: I haz the power, 2015-12-15 15:17:23]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[baxley, 2015-12-15 19:23:05]

Hi Brett,

There was a problem with transitive configs not being able to be removed.

The original CL added (and set) "default_symbol_visibility", but then other
build files would try to remove the old "symbol_visibility_hidden", which was
transitively set by "default_symbol_visibility", and they could not find it. For
example:
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...

Would it make sense to just use one config and rename it
"default_symbol_visibility"? The most recent patch includes that, except for the
name change (I wanted to hold off on that until I verify it's the correct
approach since it involves coordinating with other repos pulled in via DEPS). If
you have other suggestions, please let me know.

Thanks,
Mike

[baxley, 2015-12-17 15:16:47]

On 2015/12/15 19:23:05, baxley wrote:
> Hi Brett,
> 
> There was a problem with transitive configs not being able to be removed.
> 
> The original CL added (and set) "default_symbol_visibility", but then other
> build files would try to remove the old "symbol_visibility_hidden", which was
> transitively set by "default_symbol_visibility", and they could not find it.
For
> example:
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...
> 
> Would it make sense to just use one config and rename it
> "default_symbol_visibility"? The most recent patch includes that, except for
the
> name change (I wanted to hold off on that until I verify it's the correct
> approach since it involves coordinating with other repos pulled in via DEPS).
If
> you have other suggestions, please let me know.
> 
> Thanks,
> Mike

Ping. Do you have any input on my suggestion to combine the two configs from the
original CL into one?

[sdefresne, 2016-01-07 11:12:03]

https://codereview.chromium.org/1514423003/diff/80001/build/config/gcc/BUILD.gn
File build/config/gcc/BUILD.gn (right):

https://codereview.chromium.org/1514423003/diff/80001/build/config/gcc/BUILD....
build/config/gcc/BUILD.gn:10: # which functiosn we want to export from
components.
nit: s/functiosn/functions/

[baxley, 2016-03-18 22:07:28]

baxley@chromium.org changed reviewers:
- bm386100@gmail.com, brettw@chromium.org, dpranke@chromium.org

[baxley, 2016-03-18 22:07:28]

Sylvain,

I want to revisit this to make sure gn is working for iOS tests (I didn't
forget).

Patch 3 was the preferred approach, but has issues with other code needing to
know whether it should remove the config "default_symbol_visibility" or
"symbol_visibility_hidden".

The latest patch simply never adds -fvisilibity=hidden for iOS debug within
config("symbol_visibility_hidden").


Let me know if you have any thoughts, or you want me to make this a new CL, the
initial gyp portion was landed separately.