Issue 6396 - Error creating non debug build config

Issue 6396 - Error creating non debug build config

This helps to build apk in Release mode with V8 as shared library (by default for Release is_component_build=false and all the libs are compiled into static libraries).

# BUT it crashes in runtime so one more code review is required to make it actually working (i'm switching to another task for now) and i or anyone else will return back to make it actually working. More information here: https://issues.adblockplus.org/ticket/6396#comment:15

# Closed in favour of more recent code review:
https://codereview.adblockplus.org/29733646/

[anton, 2018-03-12 11:23:20]

https://codereview.adblockplus.org/29720588/diff/29720589/gni/v8.gni
File gni/v8.gni (right):

https://codereview.adblockplus.org/29720588/diff/29720589/gni/v8.gni#newcode50
gni/v8.gni:50: v8_component_build = false
now we should add "v8_component_build=true":

gn gen --args='target_os="android" proprietary_codecs=true
is_official_build=true is_debug=false ffmpeg_branding="Chrome"
v8_component_build=true' out/ReleaseV8Shared

[sergei, 2018-03-12 12:43:32]

I have quickly run through the code without diving into details, it's very good
that you found the possibility to decouple V8.

Could you please put `#` before the comment:
"BUT it crashes in runtime so one more code review is required to make it
actually working (i'm switching to another task for now) and i or anyone else
will return back to make it actually working. More information here:
https://issues.adblockplus.org/ticket/6396#comment:15"
because it's not going to the commit message, it's just a message for reviewers.

https://codereview.adblockplus.org/29720588/diff/29720589/BUILD.gn
File BUILD.gn (left):

https://codereview.adblockplus.org/29720588/diff/29720589/BUILD.gn#oldcode772
BUILD.gn:772: root_build_dir),
Is it correct that `root_build_dir` is removed now? I have not looked into it
but it can be the reason of the crash, IMO. What is the result value when
rebase_path is called with the empty second argument?

https://codereview.adblockplus.org/29720588/diff/29720589/BUILD.gn
File BUILD.gn (right):

https://codereview.adblockplus.org/29720588/diff/29720589/BUILD.gn#newcode2637
BUILD.gn:2637: #"//build/config/gcc:executable_ldconfig"
The commented line.

https://codereview.adblockplus.org/29720588/diff/29720589/gni/v8.gni
File gni/v8.gni (right):

https://codereview.adblockplus.org/29720588/diff/29720589/gni/v8.gni#newcode176
gni/v8.gni:176: output_extension = "cr.so" # override default .so with .cr.so
it would be better to put the reason it's overridden.

[anton, 2018-03-12 12:51:23]

https://codereview.adblockplus.org/29720588/diff/29720589/BUILD.gn
File BUILD.gn (left):

https://codereview.adblockplus.org/29720588/diff/29720589/BUILD.gn#oldcode772
BUILD.gn:772: root_build_dir),
On 2018/03/12 12:43:31, sergei wrote:
> Is it correct that `root_build_dir` is removed now? I have not looked into it
> but it can be the reason of the crash, IMO. What is the result value when
> rebase_path is called with the empty second argument?

Previously it was relative path (relative to build folder which is in
`root_build_dir`). Now it's full absolute path. The reason is that for some
reason it can't link with `libv8..` in the same folder.  Probably we should
investigate more deeply why it did not work that way for Release build.

https://codereview.adblockplus.org/29720588/diff/29720589/BUILD.gn
File BUILD.gn (right):

https://codereview.adblockplus.org/29720588/diff/29720589/BUILD.gn#newcode2637
BUILD.gn:2637: #"//build/config/gcc:executable_ldconfig"
On 2018/03/12 12:43:31, sergei wrote:
> The commented line.

Right, it's because the task is not finished yet.
I've left it to use it if required.

https://codereview.adblockplus.org/29720588/diff/29720589/gni/v8.gni
File gni/v8.gni (right):

https://codereview.adblockplus.org/29720588/diff/29720589/gni/v8.gni#newcode176
gni/v8.gni:176: output_extension = "cr.so" # override default .so with .cr.so
On 2018/03/12 12:43:32, sergei wrote:
> it would be better to put the reason it's overridden.

yes, i agree the comment could be better.
Actually it's explained here:
https://gitlab.com/eyeo/adblockplus/chromium/blob/dev-64.0.3249.2/build/toolc...
Since it works inly for is_component_build by default removing
`is_component_build=true` hurts - all the libs are having ".so" and this brings
additional issues.

TBD

[anton, 2018-03-12 12:52:44]

> `is_component_build=true` hurts - all the libs are having ".so" and this
brings
> additional issues.

that was meant to be ".. all the libs are having ".cr.so" and this brings.. "

[jens, 2018-03-13 13:17:49]

https://codereview.adblockplus.org/29720588/diff/29720589/gni/v8.gni
File gni/v8.gni (right):

https://codereview.adblockplus.org/29720588/diff/29720589/gni/v8.gni#newcode176
gni/v8.gni:176: output_extension = "cr.so" # override default .so with .cr.so
On 2018/03/12 12:51:22, anton wrote:
> On 2018/03/12 12:43:32, sergei wrote:
> > it would be better to put the reason it's overridden.
> 
> yes, i agree the comment could be better.
> Actually it's explained here:
>
https://gitlab.com/eyeo/adblockplus/chromium/blob/dev-64.0.3249.2/build/toolc...
> Since it works inly for is_component_build by default removing
> `is_component_build=true` hurts - all the libs are having ".so" and this
brings
> additional issues.
> 
> TBD

You could just add a link to the ticket which explains why it needs to be done.

[anton, 2018-03-13 13:19:33]

On 2018/03/13 13:17:49, jens wrote:
> https://codereview.adblockplus.org/29720588/diff/29720589/gni/v8.gni
> File gni/v8.gni (right):
> 
>
https://codereview.adblockplus.org/29720588/diff/29720589/gni/v8.gni#newcode176
> gni/v8.gni:176: output_extension = "cr.so" # override default .so with .cr.so
> On 2018/03/12 12:51:22, anton wrote:
> > On 2018/03/12 12:43:32, sergei wrote:
> > > it would be better to put the reason it's overridden.
> > 
> > yes, i agree the comment could be better.
> > Actually it's explained here:
> >
>
https://gitlab.com/eyeo/adblockplus/chromium/blob/dev-64.0.3249.2/build/toolc...
> > Since it works inly for is_component_build by default removing
> > `is_component_build=true` hurts - all the libs are having ".so" and this
> brings
> > additional issues.
> > 
> > TBD
> 
> You could just add a link to the ticket which explains why it needs to be
done.

It's the same ticket. I've done 1 part (change build config to compile v8 as
shared library even in Chromium Release mode) of 2.
Now one have to make it running (as it crashes in runtime).

[jens, 2018-03-13 13:21:18]

On 2018/03/13 13:19:33, anton wrote:
> On 2018/03/13 13:17:49, jens wrote:
> > https://codereview.adblockplus.org/29720588/diff/29720589/gni/v8.gni
> > File gni/v8.gni (right):
> > 
> >
>
https://codereview.adblockplus.org/29720588/diff/29720589/gni/v8.gni#newcode176
> > gni/v8.gni:176: output_extension = "cr.so" # override default .so with
.cr.so
> > On 2018/03/12 12:51:22, anton wrote:
> > > On 2018/03/12 12:43:32, sergei wrote:
> > > > it would be better to put the reason it's overridden.
> > > 
> > > yes, i agree the comment could be better.
> > > Actually it's explained here:
> > >
> >
>
https://gitlab.com/eyeo/adblockplus/chromium/blob/dev-64.0.3249.2/build/toolc...
> > > Since it works inly for is_component_build by default removing
> > > `is_component_build=true` hurts - all the libs are having ".so" and this
> > brings
> > > additional issues.
> > > 
> > > TBD
> > 
> > You could just add a link to the ticket which explains why it needs to be
> done.
> 
> It's the same ticket. I've done 1 part (change build config to compile v8 as
> shared library even in Chromium Release mode) of 2.
> Now one have to make it running (as it crashes in runtime).

Yeah, I just mean as a extension to the existing comment, as Sergej and you
wrote that the comment could be improved to contain more information.