Refactoring ./webrtc/modules/BUILD.gn for gn check

Refactoring ./webrtc/modules/BUILD.gn for gn check

BUG=webrtc:6828
NOTRY=True

[kjellander_webrtc, 2016-12-30 13:22:43]

kjellander@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[kjellander_webrtc, 2016-12-30 13:22:44]

https://codereview.webrtc.org/2609443003/diff/440001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2609443003/diff/440001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:731:
"//testing/android/native_test:native_test_support",
Can you try with native_test_native_code instead?
Maybe it can help solving the libc++ problem.

Otherwise I suggest you add agrieve@chromium.org to this review and ask for
advice.

https://codereview.webrtc.org/2609443003/diff/440001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2609443003/diff/440001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:836:
"//webrtc/modules:_modules_tests__library",
Please make this //webrtc/modules:* instead.
All targets with __ are template-generated ones.
Keep ":*" though.

[mbonadei, 2016-12-30 13:36:10]

https://codereview.webrtc.org/2609443003/diff/20001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2609443003/diff/20001/webrtc/modules/BUILD.gn#n...
webrtc/modules/BUILD.gn:297: #TODO(mbonadei): Remove (bugs.webrtc.org/6828)
I've disabled the check on this target because it depends on
"//webrtc/modules/audio_processing:audioproc_debug_proto" which is a generated
target and I don't know how to change the visibility config of that target to
whitelist ":modules_unittests".
I've found something at "third_party/protobuf/proto_library.gni" but I am still
unable to change the visibility.

https://codereview.webrtc.org/2609443003/diff/440001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2609443003/diff/440001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:731:
"//testing/android/native_test:native_test_support",
On 2016/12/30 13:22:44, kjellander_webrtc wrote:
> Can you try with native_test_native_code instead?
> Maybe it can help solving the libc++ problem.
> 
> Otherwise I suggest you add mailto:agrieve@chromium.org to this review and ask
for
> advice.

Acknowledged.

https://codereview.webrtc.org/2609443003/diff/440001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2609443003/diff/440001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:836:
"//webrtc/modules:_modules_tests__library",
On 2016/12/30 13:22:44, kjellander_webrtc wrote:
> Please make this //webrtc/modules:* instead.
> All targets with __ are template-generated ones.
> Keep ":*" though.

Acknowledged.

[kjellander_webrtc, 2016-12-30 13:39:30]

https://codereview.webrtc.org/2609443003/diff/460001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2609443003/diff/460001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:302: #
//webrtc/modules/audio_processing:audioproc_debug_proto
Actually, that target is not generated (only the ones with two underscores in
the names are). 
You can change its visibility here:
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_process...

[mbonadei, 2016-12-30 13:47:26]

https://codereview.webrtc.org/2609443003/diff/460001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2609443003/diff/460001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:302: #
//webrtc/modules/audio_processing:audioproc_debug_proto
On 2016/12/30 13:39:30, kjellander_webrtc wrote:
> Actually, that target is not generated (only the ones with two underscores in
> the names are). 
> You can change its visibility here:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_process...

Oh thanks. I don't remember why I ended up with such a conclusion. It is also an
accessible target. :)

[mbonadei, 2016-12-30 13:53:59]

mbonadei@webrtc.org changed reviewers:
+ agrieve@chromium.org

[mbonadei, 2016-12-30 13:54:00]

Hi agrieve@, I am trying to enable "gn check" for all the targets in
"//webrtc/modules".

To do so I had to change some visibility configurations and some declare some
dependencies in the relative BUILD.gn files.

When I try to build on android I get a strange error (Error: 'libc++.so': No
such file), you can find the log here:
https://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/buil....
Do you have some advice about this kind of error?

Thanks you,

Mirko

[agrieve, 2017-01-03 13:34:37]

On 2016/12/30 13:54:00, mbonadei wrote:
> Hi agrieve@, I am trying to enable "gn check" for all the targets in
> "//webrtc/modules".
> 
> To do so I had to change some visibility configurations and some declare some
> dependencies in the relative BUILD.gn files.
> 
> When I try to build on android I get a strange error (Error: 'libc++.so': No
> such file), you can find the log here:
>
https://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/buil....
> Do you have some advice about this kind of error?
> 
> Thanks you,
> 
> Mirko

I'm not sure what's going on, but I can point out some hints:

* There is no libc++.so on android. The lib there is called "libc++_shared.so"
* Sanitizers build their own copy of libc++.so, and use that instead of
libc++_shared.so, but it doesn't look like the bot has them enabled
  
https://cs.chromium.org/chromium/src/buildtools/third_party/libc%2B%2B/BUILD....
* Nacl has a: libs = ["c++"], so if that somehow got pulled in via a new deps,
that could explain it.
  
https://cs.chromium.org/chromium/src/build/config/nacl/BUILD.gn?q=%22c%2B%2B%...

I might try and build the target locally with ninja -v, and see what linker
flags are being passed. Maybe do that with & without your change to prove out
the theory that one of the added deps is adding a linker flag (look for -lc++ or
similar)

[kjellander_webrtc, 2017-02-15 07:28:55]

Thanks for all the input agrieve@!
Mirko: did you have any chance to look at this? With so much effort spent on
this bug it would be sad to not finish it.

On 2017/01/03 13:34:37, agrieve wrote:
> On 2016/12/30 13:54:00, mbonadei wrote:
> > Hi agrieve@, I am trying to enable "gn check" for all the targets in
> > "//webrtc/modules".
> > 
> > To do so I had to change some visibility configurations and some declare
some
> > dependencies in the relative BUILD.gn files.
> > 
> > When I try to build on android I get a strange error (Error: 'libc++.so': No
> > such file), you can find the log here:
> >
>
https://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/buil....
> > Do you have some advice about this kind of error?
> > 
> > Thanks you,
> > 
> > Mirko
> 
> I'm not sure what's going on, but I can point out some hints:
> 
> * There is no libc++.so on android. The lib there is called "libc++_shared.so"
> * Sanitizers build their own copy of libc++.so, and use that instead of
> libc++_shared.so, but it doesn't look like the bot has them enabled
>   
>
https://cs.chromium.org/chromium/src/buildtools/third_party/libc%2B%2B/BUILD....
> * Nacl has a: libs = ["c++"], so if that somehow got pulled in via a new deps,
> that could explain it.
>   
>
https://cs.chromium.org/chromium/src/build/config/nacl/BUILD.gn?q=%22c%2B%2B%...
> 
> I might try and build the target locally with ninja -v, and see what linker
> flags are being passed. Maybe do that with & without your change to prove out
> the theory that one of the added deps is adding a linker flag (look for -lc++
or
> similar)

[mbonadei, 2017-02-15 07:34:16]

On 2017/02/15 07:28:55, kjellander_webrtc wrote:
> Thanks for all the input agrieve@!
> Mirko: did you have any chance to look at this? With so much effort spent on
> this bug it would be sad to not finish it.

Yep, I tried to follow the input but without any luck.

I agree with you that I have to allocate a couple of hours to work on this,
maybe before the end of the week. :)

[kjellander_webrtc, 2017-02-15 07:43:33]

I see. And there's no way to just modify the CL (and skip GN check on some
targets) to make it land-able for now?

Then we could deal with the Android problem later. Or we have no idea which
change in this CL that causes it?
For that, you could reach out and ask magjed/sakal as they might be able to
debug further on Android if it's so easy to reproduce the error.

https://codereview.webrtc.org/2609443003/diff/480001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2609443003/diff/480001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:818:
"//webrtc/modules:_modules_tests__library",
isn't this redundant due to the rule below?

[mbonadei, 2017-03-08 00:09:34]

On 2017/02/15 07:43:33, kjellander_webrtc wrote:
> I see. And there's no way to just modify the CL (and skip GN check on some
> targets) to make it land-able for now?
> 
> Then we could deal with the Android problem later. Or we have no idea which
> change in this CL that causes it?
> For that, you could reach out and ask magjed/sakal as they might be able to
> debug further on Android if it's so easy to reproduce the error.
> 
>
https://codereview.webrtc.org/2609443003/diff/480001/webrtc/modules/audio_cod...
> File webrtc/modules/audio_coding/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2609443003/diff/480001/webrtc/modules/audio_cod...
> webrtc/modules/audio_coding/BUILD.gn:818:
> "//webrtc/modules:_modules_tests__library",
> isn't this redundant due to the rule below?

I close this CL because things are changed since I was trying to land it and all
the package boundary 
violations that I was trying to fix has been resolved by Edward's work.

This CL [1] just enables 'gn check' on all the targets in '//webrtc/modules'.

[1] - https://codereview.webrtc.org/2732423002/

[mbonadei, 2017-03-08 00:15:20]

On 2017/03/08 00:09:34, mbonadei wrote:
> On 2017/02/15 07:43:33, kjellander_webrtc wrote:
> > I see. And there's no way to just modify the CL (and skip GN check on some
> > targets) to make it land-able for now?
> > 
> > Then we could deal with the Android problem later. Or we have no idea which
> > change in this CL that causes it?
> > For that, you could reach out and ask magjed/sakal as they might be able to
> > debug further on Android if it's so easy to reproduce the error.
> > 
> >
>
https://codereview.webrtc.org/2609443003/diff/480001/webrtc/modules/audio_cod...
> > File webrtc/modules/audio_coding/BUILD.gn (right):
> > 
> >
>
https://codereview.webrtc.org/2609443003/diff/480001/webrtc/modules/audio_cod...
> > webrtc/modules/audio_coding/BUILD.gn:818:
> > "//webrtc/modules:_modules_tests__library",
> > isn't this redundant due to the rule below?
> 
> I close this CL because things are changed since I was trying to land it and
all
> the package boundary 
> violations that I was trying to fix has been resolved by Edward's work.
> 
> This CL [1] just enables 'gn check' on all the targets in '//webrtc/modules'.
> 
> [1] - https://codereview.webrtc.org/2732423002/

I have also just noticed that I didn't answer to agrieve@ a couple of months
ago. 
I tried your advice and I thought I answered but I didn't. 
My bad, sorry and thanks for the review.