Pass the build type to ant build

Pass the build type to ant build

Have it disabled now until the content shell apk is ready to handle
release mode.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146730

[John Grabowski, 2012-06-26 19:30:37]

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi#new...
content/content_shell.gypi:557: # 'ant_build_type%': '`if [
"<(CONFIGURATION_NAME)" = "Debug" ]; then echo "debug"; elif [
"<(CONFIGURATION_NAME)" = "Release" ]; then echo "release"; fi;`',
<!(cmd) instead of backticks

[yongsheng, 2012-06-27 03:36:04]

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi#new...
content/content_shell.gypi:557: # 'ant_build_type%': '`if [
"<(CONFIGURATION_NAME)" = "Debug" ]; then echo "debug"; elif [
"<(CONFIGURATION_NAME)" = "Release" ]; then echo "release"; fi;`',
On 2012/06/26 19:30:37, John Grabowski wrote:
> <!(cmd) instead of backticks
seems not workable. Because the gyp will execute it when generating .mk file.
However, this command could only run when making.

[John Grabowski, 2012-06-27 16:45:58]

Mark: can you comment on gyp use?

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi#new...
content/content_shell.gypi:557: # 'ant_build_type%': '`if [
"<(CONFIGURATION_NAME)" = "Debug" ]; then echo "debug"; elif [
"<(CONFIGURATION_NAME)" = "Release" ]; then echo "release"; fi;`',
On 2012/06/27 03:36:04, yongsheng.zhu wrote:
> On 2012/06/26 19:30:37, John Grabowski wrote:
> > <!(cmd) instead of backticks
> seems not workable. Because the gyp will execute it when generating .mk file.
> However, this command could only run when making.

Can you explain why it's a problem in a mk file?

[yongsheng, 2012-06-28 02:38:40]

On 2012/06/27 16:45:58, John Grabowski wrote:
> Mark: can you comment on gyp use?
> 
> http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi
> File content/content_shell.gypi (right):
> 
>
http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi#new...
> content/content_shell.gypi:557: # 'ant_build_type%': '`if [
> "<(CONFIGURATION_NAME)" = "Debug" ]; then echo "debug"; elif [
> "<(CONFIGURATION_NAME)" = "Release" ]; then echo "release"; fi;`',
> On 2012/06/27 03:36:04, yongsheng.zhu wrote:
> > On 2012/06/26 19:30:37, John Grabowski wrote:
> > > <!(cmd) instead of backticks
> > seems not workable. Because the gyp will execute it when generating .mk
file.
> > However, this command could only run when making.
> 
> Can you explain why it's a problem in a mk file?
<!(cmd) will execute the 'cmd' when the gyp generate makefiles. However, it
should be executed when running make.
Here is the error msg if using '<!(cmd)':
Exception: Call to 'if [ "$(BUILDTYPE)" = "Debug" ]; then echo "debug"; elif [
"$(BUILDTYPE)" = "Release" ]; then echo "release"; fi;' returned exit status 0.

[Mark Mentovai, 2012-06-28 14:24:18]

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi#new...
content/content_shell.gypi:583: # '<(ant_build_type)',
Why not just <(CONFIGURATION_NAME) here? It’s awful to depend on shell behavior
here and in ant_build_type. If ant can’t deal with Release and Debug with
capital letters, have this action call a helper script that transforms the
argument appropriately. That’s guaranteed to work. Writing shell garbage into an
action spec like this is not guaranteed to work. In fact, it shouldn’t work, and
if it does, it only does with certain generators and is buggy.

[yongsheng, 2012-06-29 02:01:46]

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi#new...
content/content_shell.gypi:583: # '<(ant_build_type)',
On 2012/06/28 14:24:18, Mark Mentovai wrote:
> Why not just <(CONFIGURATION_NAME) here? It’s awful to depend on shell
behavior
> here and in ant_build_type. If ant can’t deal with Release and Debug with
> capital letters, have this action call a helper script that transforms the
> argument appropriately. That’s guaranteed to work. Writing shell garbage into
an
> action spec like this is not guaranteed to work. In fact, it shouldn’t work,
and
> if it does, it only does with certain generators and is buggy.
Thanks for your comments.
How about replacing it with the below code?
`echo <(CONFIGURATION_NAME) | tr "[A-Z]" "[a-z]"`
It uses `` instead of '<!(cmd)' for it should execute it when doing make. 
Actually, i follow up another parameter in this action. See below: 
'-DPRODUCT_DIR=<(ant_build_out)'
Here is the definition of 'ant_build_out':
build/common.gypi:865:        'ant_build_out': '`cd <(PRODUCT_DIR) && pwd -P`',
I verified that they all work. 
make sense?

[Mark Mentovai, 2012-06-29 02:03:54]

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi#new...
content/content_shell.gypi:583: # '<(ant_build_type)',
Again, backticks aren’t guaranteed to work. I gave you my advice earlier today.

[yongsheng, 2012-06-29 02:08:21]

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi#new...
content/content_shell.gypi:583: # '<(ant_build_type)',
On 2012/06/29 02:03:54, Mark Mentovai wrote:
> Again, backticks aren’t guaranteed to work. I gave you my advice earlier
today.
yes, i see. Thanks. If it doesn't work, any hint about your suggestion for the
'helper script'?

[Mark Mentovai, 2012-06-29 03:20:18]

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi#new...
content/content_shell.gypi:583: # '<(ant_build_type)',
#!/bin/bash
set -eu
buildtype="$(tr '[A-Z]' '[a-z]' <<< "${5}")"
exec "${1}" "${2}" "${3}" "${4}" "${buildtype}"

[yongsheng, 2012-06-29 23:45:20]

On 2012/06/29 03:20:18, Mark Mentovai wrote:
> http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi
> File content/content_shell.gypi (right):
> 
>
http://codereview.chromium.org/10669030/diff/1/content/content_shell.gypi#new...
> content/content_shell.gypi:583: # '<(ant_build_type)',
> #!/bin/bash
> set -eu
> buildtype="$(tr '[A-Z]' '[a-z]' <<< "${5}")"
> exec "${1}" "${2}" "${3}" "${4}" "${buildtype}"

thanks, i'll refine the patch according to this.

[yongsheng, 2012-07-10 01:56:09]

so it's done, please help review.

[John Grabowski, 2012-07-10 03:43:43]

LGTM

http://codereview.chromium.org/10669030/diff/12001/content/content_shell_ant_...
File content/content_shell_ant_helper.sh (right):

http://codereview.chromium.org/10669030/diff/12001/content/content_shell_ant_...
content/content_shell_ant_helper.sh:7: set -eu
A comment in this file is warranted.  E.g. used from FOO; sample input is BAR;
sample output is BAZ.

[John Grabowski, 2012-07-10 03:45:34]

LGTM conditional on file movement.

http://codereview.chromium.org/10669030/diff/12001/content/content_shell_ant_...
File content/content_shell_ant_helper.sh (right):

http://codereview.chromium.org/10669030/diff/12001/content/content_shell_ant_...
content/content_shell_ant_helper.sh:3: # Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Uh, one more thing.
I don't think this script should go in the top level content/ directory.
build/android/content_shell_ant_helper.sh seems better.

[ivan_cobs.com.sg, 2012-07-10 03:49:38]

Pls remove the following email adds from your list NOW

ivan@cobs.com.sg
lynn@cobs.com.sg

-----Original Message-----
From: jrg@chromium.org [mailto:jrg@chromium.org] 
Sent: Tuesday, 10 July, 2012 11:44 AM
To: yongsheng.zhu@intel.com; navabi@chromium.org; mark@chromium.org
Cc: chromium-reviews@chromium.org; joi+watch-content@chromium.org;
darin-cc@chromium.org; jam@chromium.org; jochen+watch-content@chromium.org
Subject: [chromium-reviews] Re: Pass the build type to ant build (issue
10669030)

LGTM



http://codereview.chromium.org/10669030/diff/12001/content/content_shell_ant
_helper.sh
File content/content_shell_ant_helper.sh (right):

http://codereview.chromium.org/10669030/diff/12001/content/content_shell_ant
_helper.sh#newcode7
content/content_shell_ant_helper.sh:7: set -eu
A comment in this file is warranted.  E.g. used from FOO; sample input
is BAR; sample output is BAZ.

http://codereview.chromium.org/10669030/

[yongsheng, 2012-07-10 04:44:05]

ok, uploaded the new patch. Thanks for your review, jrg.

[John Grabowski, 2012-07-10 06:07:49]

Still LGTM

You'll need a content/ owner for final OWNERS approval.

[John Grabowski, 2012-07-10 06:07:51]

Still LGTM

You'll need a content/ owner for final OWNERS approval.

[yongsheng, 2012-07-10 06:10:32]

On 2012/07/10 06:07:51, John Grabowski wrote:
> Still LGTM
> 
> You'll need a content/ owner for final OWNERS approval.
thanks.
Darin, could you please review it? Thanks.

[John Grabowski, 2012-07-10 06:15:34]

Suggest jam@ as first choice for Android content OWNERS, and avi@ as a
second choice.



On Mon, Jul 9, 2012 at 11:10 PM, <yongsheng.zhu@intel.com> wrote:

> On 2012/07/10 06:07:51, John Grabowski wrote:
>
>> Still LGTM
>>
>
>  You'll need a content/ owner for final OWNERS approval.
>>
> thanks.
> Darin, could you please review it? Thanks.
>
>
>
http://codereview.chromium.**org/10669030/<http://codereview.chromium.org/106...
>

[yongsheng, 2012-07-10 06:30:57]

jam, could you please help review it?

[jochen (gone - plz use gerrit), 2012-07-10 09:14:24]

http://codereview.chromium.org/10669030/diff/15001/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10669030/diff/15001/content/content_shell.gypi...
content/content_shell.gypi:581: # '<(CONFIGURATION_NAME)',
What about disabling this (i.e. replacing the last parameter with "release") in
the content_shell_ant_helper.sh script instead of here? That way, the script is
already exercised.

[yongsheng, 2012-07-10 11:25:45]

http://codereview.chromium.org/10669030/diff/15001/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10669030/diff/15001/content/content_shell.gypi...
content/content_shell.gypi:581: # '<(CONFIGURATION_NAME)',
On 2012/07/10 09:14:24, jochen wrote:
> What about disabling this (i.e. replacing the last parameter with "release")
in
> the content_shell_ant_helper.sh script instead of here? That way, the script
is
> already exercised.
It's also ok for me. I'll change it. Thanks.

[yongsheng, 2012-07-10 11:36:49]

thanks for all your comments, please review it again.

[Mark Mentovai, 2012-07-10 12:21:37]

http://codereview.chromium.org/10669030/diff/23001/build/android/content_shel...
File build/android/content_shell_ant_helper.sh (right):

http://codereview.chromium.org/10669030/diff/23001/build/android/content_shel...
build/android/content_shell_ant_helper.sh:11: # It's used by
content/content_shell.gypi
Then why does it live here in some far-off directory, instead of next to
content_shell.gypi? Will this be useful for any other part of the build in the
short or even medium term?

[John Grabowski, 2012-07-10 14:16:05]

http://codereview.chromium.org/10669030/diff/23001/build/android/content_shel...
File build/android/content_shell_ant_helper.sh (right):

http://codereview.chromium.org/10669030/diff/23001/build/android/content_shel...
build/android/content_shell_ant_helper.sh:11: # It's used by
content/content_shell.gypi
On 2012/07/10 12:21:37, Mark Mentovai wrote:
> Then why does it live here in some far-off directory, instead of next to
> content_shell.gypi? Will this be useful for any other part of the build in the
> short or even medium term?

Because I suggested he move it?  There are no other scripts in the content/
directory.

I think you and I are asking for opposite things here.

[Mark Mentovai, 2012-07-10 14:19:34]

OK.

build is for scripts of general interest throughout chrome. For single-purpose
scripts, it is usual to place them next to the .gyp file that makes use of them.

[jam, 2012-07-10 16:10:33]

On 2012/07/10 14:19:34, Mark Mentovai wrote:
> OK.
> 
> build is for scripts of general interest throughout chrome. For single-purpose
> scripts, it is usual to place them next to the .gyp file that makes use of
them.

i agree it seems odd putting a file named content_shell in src/build. Why not
just put it in src/content/shell/ ?

[yongsheng, 2012-07-10 23:56:21]

On 2012/07/10 16:10:33, John Abd-El-Malek wrote:
> On 2012/07/10 14:19:34, Mark Mentovai wrote:
> > OK.
> > 
> > build is for scripts of general interest throughout chrome. For
single-purpose
> > scripts, it is usual to place them next to the .gyp file that makes use of
> them.
> 
> i agree it seems odd putting a file named content_shell in src/build. Why not
> just put it in src/content/shell/ ?
okay, i'll put it to here.

[yongsheng, 2012-07-11 01:40:49]

thanks for all of your comments. so done. need your 'lgtm' to land it.

[Mark Mentovai, 2012-07-11 13:51:43]

http://codereview.chromium.org/10669030/diff/4004/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10669030/diff/4004/content/content_shell.gypi#...
content/content_shell.gypi:573:
'<(DEPTH)/content/shell/content_shell_ant_helper.sh',
Don’t use <(DEPTH). This is a path relative to the .gyp file that uses it. The
.gyp file is in content, so this should just be
'shell/content_shell_ant_helper.sh'.

[yongsheng, 2012-07-12 01:54:36]

On 2012/07/11 13:51:43, Mark Mentovai wrote:
> http://codereview.chromium.org/10669030/diff/4004/content/content_shell.gypi
> File content/content_shell.gypi (right):
> 
>
http://codereview.chromium.org/10669030/diff/4004/content/content_shell.gypi#...
> content/content_shell.gypi:573:
> '<(DEPTH)/content/shell/content_shell_ant_helper.sh',
> Don’t use <(DEPTH). This is a path relative to the .gyp file that uses it. The
> .gyp file is in content, so this should just be
> 'shell/content_shell_ant_helper.sh'.

ok, thanks. it's done. so any other comments?

[Mark Mentovai, 2012-07-12 13:39:42]

LGTM otherwise

http://codereview.chromium.org/10669030/diff/25002/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10669030/diff/25002/content/content_shell.gypi...
content/content_shell.gypi:578:
'<(DEPTH)/content/shell/android/content_shell_apk.xml',
Well when you took the <(DEPTH) out above, you could have gotten rid of it here,
and in the 'inputs' specification as well.

[yongsheng, 2012-07-12 14:04:02]

done, so i'll commit it?

[Mark Mentovai, 2012-07-12 14:04:36]

LGTM

[yongsheng, 2012-07-12 14:05:33]

On 2012/07/12 14:04:36, Mark Mentovai wrote:
> LGTM
thanks a lot for all of your comments.

[commit-bot: I haz the power, 2012-07-12 14:05:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsheng.zhu@intel.com/10669030/9005

[commit-bot: I haz the power, 2012-07-12 14:06:06]

Failed to apply patch for content/content_shell.gypi:
While running patch -p1 --forward --force;
patching file content/content_shell.gypi
Hunk #1 FAILED at 549.
Hunk #2 FAILED at 567.
2 out of 2 hunks FAILED -- saving rejects to file content/content_shell.gypi.rej

[yongsheng, 2012-07-12 14:08:07]

On 2012/07/12 14:06:06, I haz the power (commit-bot) wrote:
> Failed to apply patch for content/content_shell.gypi:
> While running patch -p1 --forward --force;
> patching file content/content_shell.gypi
> Hunk #1 FAILED at 549.
> Hunk #2 FAILED at 567.
> 2 out of 2 hunks FAILED -- saving rejects to file
content/content_shell.gypi.rej
I'll rebase my code.

[commit-bot: I haz the power, 2012-07-12 14:22:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsheng.zhu@intel.com/10669030/12006

[commit-bot: I haz the power, 2012-07-12 14:22:58]

Presubmit check for 10669030-12006 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content

[yongsheng, 2012-07-12 14:25:31]

On 2012/07/12 14:22:58, I haz the power (commit-bot) wrote:
> Presubmit check for 10669030-12006 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit Messages **
> If this change has an associated bug, add BUG=[bug number].
> 
> If this change requires manual test instructions to QA team, add
> TEST=[instructions].
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     content

jam, need your OWNER LGTM, thanks.

[jam, 2012-07-12 18:39:18]

lgtm

[commit-bot: I haz the power, 2012-07-12 22:57:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsheng.zhu@intel.com/10669030/12006

[commit-bot: I haz the power, 2012-07-12 23:26:17]

Try job failure for 10669030-12006 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[yongsheng, 2012-07-13 01:30:42]

The error is 
"/bin/sh: shell/content_shell_ant_helper.sh: Permission denied"
I check this file's mode: 100755. seems ok.
So i'm wondering whether it's due to '!/bin/bash' in the file.
In some environments, 'buildtype="$(tr '[A-Z]' '[a-z]' <<< "${6}")" is not
supported by /bin/sh. So maybe use 'buildtype="$(echo ${6} | tr '[A-Z]'
'[a-z]')".
I'll have a try.

[commit-bot: I haz the power, 2012-07-13 03:20:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsheng.zhu@intel.com/10669030/4008

[commit-bot: I haz the power, 2012-07-13 03:48:15]

Try job failure for 10669030-4008 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[yongsheng, 2012-07-13 04:28:08]

still failed. any idea about this error: ""/bin/sh:
shell/content_shell_ant_helper.sh: Permission denied"

[Mark Mentovai, 2012-07-13 12:51:54]

I don’t know if the commit queue knows how to map the file mode in a git patch
to an svn:executable property.

[John Grabowski, 2012-07-13 17:12:42]

On 2012/07/13 12:51:54, Mark Mentovai wrote:
> I don’t know if the commit queue knows how to map the file mode in a git patch
> to an svn:executable property.

Suggest splitting this patch to work around the problem: 1 CL for shell script,
2nd for gyp file.

[yongsheng, 2012-07-14 02:32:41]

On 2012/07/13 17:12:42, John Grabowski wrote:
> On 2012/07/13 12:51:54, Mark Mentovai wrote:
> > I don’t know if the commit queue knows how to map the file mode in a git
patch
> > to an svn:executable property.
> 
> Suggest splitting this patch to work around the problem: 1 CL for shell
script,
> 2nd for gyp file.
ok, I'll do it. Thanks for the suggestion.

[commit-bot: I haz the power, 2012-07-14 03:18:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsheng.zhu@intel.com/10669030/29001

[commit-bot: I haz the power, 2012-07-14 03:18:44]

Failed to apply patch for content/content_shell.gypi:
While running patch -p1 --forward --force;
patching file content/content_shell.gypi
Hunk #1 FAILED at 582.
Hunk #2 succeeded at 605 with fuzz 1 (offset 5 lines).
1 out of 2 hunks FAILED -- saving rejects to file content/content_shell.gypi.rej

[commit-bot: I haz the power, 2012-07-14 05:11:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsheng.zhu@intel.com/10669030/30003

[commit-bot: I haz the power, 2012-07-14 06:15:26]

Try job failure for 10669030-30003 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-14 09:02:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsheng.zhu@intel.com/10669030/30003

[commit-bot: I haz the power, 2012-07-14 09:57:26]

Try job failure for 10669030-30003 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-14 12:08:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsheng.zhu@intel.com/10669030/30003

[commit-bot: I haz the power, 2012-07-14 13:33:36]

Change committed as 146730