Skip downloading of nacl toolchain for android. Nacl is not enabled in android and it takes too lon…

Skip downloading of nacl toolchain for android. 
Nacl is not enabled in android and it takes too long to download the toolchain.

BUG=329646
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242405

[maheshkk, 2013-12-19 15:57:31]

proposal

[maheshkk, 2013-12-19 17:04:22]

please review

[atreat, 2013-12-20 19:55:00]

https://codereview.chromium.org/116833006/diff/20001/build/android/envsetup_f...
File build/android/envsetup_functions.sh (right):

https://codereview.chromium.org/116833006/diff/20001/build/android/envsetup_f...
build/android/envsetup_functions.sh:261: 
Unnecessary whitespace change.

https://codereview.chromium.org/116833006/diff/20001/build/download_nacl_tool...
File build/download_nacl_toolchains.py (right):

https://codereview.chromium.org/116833006/diff/20001/build/download_nacl_tool...
build/download_nacl_toolchains.py:15: print 'NACL is disabled. Skipping
NativeClient toolchain download.'
Note sure we need extra verbosity here.  It seems disable_nacl is already used
and thus for some folks the toolchain download is already being skipped.  Why
extra verbosity when you are just enlarging the scope of this to android
builders?

[navabi, 2013-12-20 20:36:59]

lgtm with nit.

https://codereview.chromium.org/116833006/diff/20001/build/android/envsetup_f...
File build/android/envsetup_functions.sh (right):

https://codereview.chromium.org/116833006/diff/20001/build/android/envsetup_f...
build/android/envsetup_functions.sh:261: 
nit: remove this space

[commit-bot: I haz the power, 2013-12-23 16:45:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mahesh.kk@samsung.com/116833006/90001

[commit-bot: I haz the power, 2013-12-23 19:03:51]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-12-23 19:55:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mahesh.kk@samsung.com/116833006/90001

[commit-bot: I haz the power, 2013-12-23 21:07:22]

Change committed as 242405

[Nico, 2013-12-24 23:56:47]

Hi,

have you signed the CLA? See
http://www.chromium.org/developers/contributing-code/external-contributor-che...
I can't see you in there.

Also, the change is technically wrong: DEFINES shouldn't have a -D,
build/common.gypi already defaults disable_nacl to 1 on android builds, and
keying off the env var in the download_toolchain script doesn't sound right
either (since that misses non-envvar ways of setting this variable).

I'll revert this for now.

[maheshkk, 2013-12-26 21:01:35]

On 2013/12/24 23:56:47, Nico wrote:
> Hi,
> 
> have you signed the CLA? See
>
http://www.chromium.org/developers/contributing-code/external-contributor-che...
> I can't see you in there.
> 

Thanks for comments! My CLA is under progress.  

> Also, the change is technically wrong: DEFINES shouldn't have a -D,

Good point, that's unnecessary. 

> build/common.gypi already defaults disable_nacl to 1 on android builds, and
> keying off the env var in the download_toolchain script doesn't sound right
> either (since that misses non-envvar ways of setting this variable).
> 

I thought the build/common.gypi is involved only while running gyp_chromium.
However runhooks still downloads the nacl tar file of 80mb which I was trying to
avoid. download_nacl_toolchains.py script already checks for the disable_nalc
env variable which I think is to avoid toolchain download. 

> I'll revert this for now.

Any points on the better way of doing this? I will come back to this patch after
my CLA is signed. 

Thanks.

[Nico, 2013-12-27 21:28:34]

On Thu, Dec 26, 2013 at 1:01 PM, <mahesh.kk@samsung.com> wrote:

> On 2013/12/24 23:56:47, Nico wrote:
>
>> Hi,
>>
>
>  have you signed the CLA? See
>>
>
> http://www.chromium.org/developers/contributing-code/
> external-contributor-checklist
>
>> I can't see you in there.
>>
>
>
> Thanks for comments! My CLA is under progress.
>
>
>  Also, the change is technically wrong: DEFINES shouldn't have a -D,
>>
>
> Good point, that's unnecessary.
>
>
>  build/common.gypi already defaults disable_nacl to 1 on android builds,
>> and
>> keying off the env var in the download_toolchain script doesn't sound
>> right
>> either (since that misses non-envvar ways of setting this variable).
>>
>
>
> I thought the build/common.gypi is involved only while running
> gyp_chromium.
> However runhooks still downloads the nacl tar file of 80mb which I was
> trying to
> avoid. download_nacl_toolchains.py script already checks for the
> disable_nalc
> env variable which I think is to avoid toolchain download.


Acknowledged. I think download_nacl_toolchains.py checking for this is
somewhat questionable too. The motivation here is that some people
explicitly disable nacl (for desktop even), to get slightly faster builds.
But since there are other ways of setting gyp defines (~/.gyp/include.gypi,
or a supplemental.gypi) it doesn't seem like a great approach. gyp defines
are for communicating stuff to gyp, and nothing else.

(Also, we're trying to get rid of envsetup.sh medium-term, so anything
that's added to it won't be around for long.)


>
>
>  I'll revert this for now.
>>
>
> Any points on the better way of doing this? I will come back to this patch
> after
> my CLA is signed.
>

It depends on what you're trying to do.

If you just want to make sure you're not downloading this locally, you can
just manually add this to GYP_DEFINES and it'll work with the current
download_nacl_toolchains.py script.

If your motivation is to not make anyone who builds chromium/android
download this, then it should be attacked at a different level. We already
have 1.5 existing ways of handling this (os-specific DEPS, and the hooks
stuff), so it should be done there somewhere. But my reading of the
chromium-dev thread "Using None in deps_os in DEPS" is that os-specific
deps shouldn't remove stuff that could be needed for another build that's
hostable on the same OS (on linux that means desktop chromium, which does
build with nacl), so I'd work on community consensus first.


>
> Thanks.
>
>
> https://codereview.chromium.org/116833006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[maheshkk, 2014-01-28 21:20:37]

I'm reopening this. For android builds NACL toolchain consumes unnecessary time
to development.
nacl toolchain gets an update few times a week and downloads the entire
toolchain,
which I'm trying to avoid by having this commit locally in my branch which is
causing me trouble too :)


> It depends on what you're trying to do.
> 
> If you just want to make sure you're not downloading this locally, you can
> just manually add this to GYP_DEFINES and it'll work with the current
> download_nacl_toolchains.py script.

This doesn't work. When we run setenv shell script for android, GYP_DEFINES gets
overridden.
As per your previous commit, some of you are already planning to get rid of
setenv, in which case, 
Do you think, if we let this CL avoid download nacl until we get rid of the
script? (Then manually defining nacl_disabled in GYP_DEFINES would work too) 
If not, I can spend more time to see how we can solve using DEPS and hooks.
> 
> If your motivation is to not make anyone who builds chromium/android
> download this, then it should be attacked at a different level. We already
> have 1.5 existing ways of handling this (os-specific DEPS, and the hooks
> stuff), so it should be done there somewhere. But my reading of the
> chromium-dev thread "Using None in deps_os in DEPS" is that os-specific
> deps shouldn't remove stuff that could be needed for another build that's
> hostable on the same OS (on linux that means desktop chromium, which does
> build with nacl), so I'd work on community consensus first.
>

[Nico, 2014-01-28 21:23:06]

On 2014/01/28 21:20:37, maheshkk wrote:
> I'm reopening this. For android builds NACL toolchain consumes unnecessary
time
> to development.
> nacl toolchain gets an update few times a week and downloads the entire
> toolchain,
> which I'm trying to avoid by having this commit locally in my branch which is
> causing me trouble too :)

Add `export GYP_DEFINES=disable_nacl=1` to your ~/.bash_rc and you should be set
locally.

envsetup is going away (http://crbug.com/330631), so adding anything to it does
not lgtm.

> 
> 
> > It depends on what you're trying to do.
> > 
> > If you just want to make sure you're not downloading this locally, you can
> > just manually add this to GYP_DEFINES and it'll work with the current
> > download_nacl_toolchains.py script.
> 
> This doesn't work. When we run setenv shell script for android, GYP_DEFINES
gets
> overridden.
> As per your previous commit, some of you are already planning to get rid of
> setenv, in which case, 
> Do you think, if we let this CL avoid download nacl until we get rid of the
> script? (Then manually defining nacl_disabled in GYP_DEFINES would work too) 
> If not, I can spend more time to see how we can solve using DEPS and hooks.
> > 
> > If your motivation is to not make anyone who builds chromium/android
> > download this, then it should be attacked at a different level. We already
> > have 1.5 existing ways of handling this (os-specific DEPS, and the hooks
> > stuff), so it should be done there somewhere. But my reading of the
> > chromium-dev thread "Using None in deps_os in DEPS" is that os-specific
> > deps shouldn't remove stuff that could be needed for another build that's
> > hostable on the same OS (on linux that means desktop chromium, which does
> > build with nacl), so I'd work on community consensus first.
> >

[maheshkk, 2014-01-28 21:32:07]

On 2014/01/28 21:23:06, Nico wrote:
> On 2014/01/28 21:20:37, maheshkk wrote:
> > I'm reopening this. For android builds NACL toolchain consumes unnecessary
> time
> > to development.
> > nacl toolchain gets an update few times a week and downloads the entire
> > toolchain,
> > which I'm trying to avoid by having this commit locally in my branch which
is
> > causing me trouble too :)
> 
> Add `export GYP_DEFINES=disable_nacl=1` to your ~/.bash_rc and you should be
set
> locally.
> 

Nico, I tried that. build/android/envsetup.sh overrides anything and everything
written to GYP_DEFINES in .bash_rc. 

> envsetup is going away (http://crbug.com/330631), so adding anything to it
does
> not lgtm.
> 

Alright, as we will be getting rid of envsetup altogether, this issue won't
trouble me for very long! :) 

Thanks for quick comments.