add proxy support in install-chroot.sh

add proxy support in install-chroot.sh


BUG=126477
TEST=


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

[Wei James(wistoch), 2012-05-07 16:00:01]

use http_proxy var in host env to set debootstrap and apt-get proxy.

[Markus (顧孟勤), 2012-05-07 18:36:26]

Does this work correctly, even if http_proxy is not currently set? Maybe,
you should only set these new flags, if http_proxy exists and is non-empty.


Markus

On Mon, May 7, 2012 at 9:00 AM, <james.wei@intel.com> wrote:

> Reviewers: Markus (顧孟勤),
>
> Message:
> use http_proxy var in host env to set debootstrap and apt-get proxy.
>
> Description:
> add proxy support in install-chroot.sh
>
>
> BUG=126477
> TEST=
>
>
> Please review this at
https://chromiumcodereview.**appspot.com/10375036/<https://chromiumcodereview...
>
> SVN Base:
http://git.chromium.org/git/**chromium.git@trunk<http://git.chromium.org/git/...
>
> Affected files:
>  M build/install-chroot.sh
>
>
> Index: build/install-chroot.sh
> diff --git a/build/install-chroot.sh b/build/install-chroot.sh
> index 82caff2385ee87d160cbda474484fc**9b182c340b..**
> 9478944aa25c0a53445f24dd738717**d777f16cad 100755
> --- a/build/install-chroot.sh
> +++ b/build/install-chroot.sh
> @@ -339,8 +339,10 @@ if [ -z "${mirror}" ]; then
>   
mirror="http://archive.ubuntu.**com/ubuntu<http://archive.ubuntu.com/ubuntu>"
> ||
>    mirror="http://ftp.us.debian.**org/debian<http://ftp.us.debian.org/debian>
> "
>  fi
> - sudo debootstrap ${archflag} "${distname}" /var/lib/chroot/"${target}"
>      \
> -                  "$mirror"
> +
> +PROXY_FLAG="http_proxy=${**http_proxy}"
> +sudo ${PROXY_FLAG} debootstrap ${archflag} "${distname}"   \
> +    "/var/lib/chroot/${target}"  "$mirror"
>
>  # Add new entry to /etc/schroot/schroot.conf
>  grep -qs ubuntu.com /usr/share/debootstrap/**scripts/"${distname}" &&
> @@ -552,6 +554,11 @@ sudo sed -i '/^deb[^-]/p
>              s/^deb\([^-]\)/deb-src\1/' \
>          "/var/lib/chroot/${target}/**etc/apt/sources.list"
>
> +# Set apt proxy
> +sudo sh -c '
> +  echo "Acquire::http::proxy \"'${http_proxy}'\";" \
> +      >>"/var/lib/chroot/'"${target}**"'/etc/apt/apt.conf"'
> +
>  # Update packages
>  sudo "/usr/local/bin/${target%bit}" /bin/sh -c '
>   apt-get update; apt-get -y dist-upgrade' || :
>
>
>

[Wei James(wistoch), 2012-05-08 01:46:19]

On 2012/05/07 18:36:26, Markus (顧孟勤) wrote:
> Does this work correctly, even if http_proxy is not currently set? Maybe,
> you should only set these new flags, if http_proxy exists and is non-empty.
> 
> 
> Markus
> 
> On Mon, May 7, 2012 at 9:00 AM, <mailto:james.wei@intel.com> wrote:
> 
> > Reviewers: Markus (顧孟勤),
> >
> > Message:
> > use http_proxy var in host env to set debootstrap and apt-get proxy.
> >
> > Description:
> > add proxy support in install-chroot.sh
> >
> >
> > BUG=126477
> > TEST=
> >
> >
> > Please review this at
>
https://chromiumcodereview.**appspot.com/10375036/%3Chttps://chromiumcoderevi...>
> >
> > SVN Base:
>
http://git.chromium.org/git/**chromium.git%40trunk%3Chttp://git.chromium.org/...>
> >
> > Affected files:
> >  M build/install-chroot.sh
> >
> >
> > Index: build/install-chroot.sh
> > diff --git a/build/install-chroot.sh b/build/install-chroot.sh
> > index 82caff2385ee87d160cbda474484fc**9b182c340b..**
> > 9478944aa25c0a53445f24dd738717**d777f16cad 100755
> > --- a/build/install-chroot.sh
> > +++ b/build/install-chroot.sh
> > @@ -339,8 +339,10 @@ if [ -z "${mirror}" ]; then
> >   
> mirror="http://archive.ubuntu.**com/ubuntu<http://archive.ubuntu.com/ubuntu>"
> > ||
> >   
mirror="http://ftp.us.debian.**org/debian<http://ftp.us.debian.org/debian>
> > "
> >  fi
> > - sudo debootstrap ${archflag} "${distname}" /var/lib/chroot/"${target}"
> >      \
> > -                  "$mirror"
> > +
> > +PROXY_FLAG="http_proxy=${**http_proxy}"
> > +sudo ${PROXY_FLAG} debootstrap ${archflag} "${distname}"   \
> > +    "/var/lib/chroot/${target}"  "$mirror"
> >
> >  # Add new entry to /etc/schroot/schroot.conf
> >  grep -qs http://ubuntu.com /usr/share/debootstrap/**scripts/"${distname}"
&&
> > @@ -552,6 +554,11 @@ sudo sed -i '/^deb[^-]/p
> >              s/^deb\([^-]\)/deb-src\1/' \
> >          "/var/lib/chroot/${target}/**etc/apt/sources.list"
> >
> > +# Set apt proxy
> > +sudo sh -c '
> > +  echo "Acquire::http::proxy \"'${http_proxy}'\";" \
> > +      >>"/var/lib/chroot/'"${target}**"'/etc/apt/apt.conf"'
> > +
> >  # Update packages
> >  sudo "/usr/local/bin/${target%bit}" /bin/sh -c '
> >   apt-get update; apt-get -y dist-upgrade' || :
> >
> >
> >

thanks for your review. 

I cannot access ubuntu mirror without proxy, so I cannot test the situation
without proxy. 

I just added the check for $http_proxy. patch updated. 

could you help to review it? thanks

[Markus (顧孟勤), 2012-05-08 02:09:57]

lgtm

Some minor nit-picks. But overall this is pretty close to what we want, I think.

http://chromiumcodereview.appspot.com/10375036/diff/1002/build/install-chroot.sh
File build/install-chroot.sh (right):

http://chromiumcodereview.appspot.com/10375036/diff/1002/build/install-chroot...
build/install-chroot.sh:348: sudo ${PROXY_FLAG} debootstrap ${archflag}
"${distname}"   \
You could probably write this more concisely as:

sudo ${http_proxy:+http_proxy="${http_proxy}"} debootstrap ...

This also has the advantage that it gets the quoting right. If "${http_proxy}"
contains white-space, we'll still forward it correctly.

But if you strongly feel your version is better, I won't fight you over it --
although I'd love if you could then fix the quoting somehow.

http://chromiumcodereview.appspot.com/10375036/diff/1002/build/install-chroot...
build/install-chroot.sh:564: echo "Acquire::http::proxy \"'${http_proxy}'\";" \
While it probably is benign in this particular case, we try to always put
double-quotes around any environment variable, especially if it was provided by
the user. This is not always possible, but here you could trivially add it.
Please do so, as it will make things easier for future reviewers -- they won't
have to scratch their heads whether there was a reason why we left of the
quotes.

[Wei James(wistoch), 2012-05-08 02:47:58]

http://chromiumcodereview.appspot.com/10375036/diff/1002/build/install-chroot.sh
File build/install-chroot.sh (right):

http://chromiumcodereview.appspot.com/10375036/diff/1002/build/install-chroot...
build/install-chroot.sh:348: sudo ${PROXY_FLAG} debootstrap ${archflag}
"${distname}"   \
On 2012/05/08 02:09:57, Markus (顧孟勤) wrote:
> You could probably write this more concisely as:
> 
> sudo ${http_proxy:+http_proxy="${http_proxy}"} debootstrap ...
> 
> This also has the advantage that it gets the quoting right. If "${http_proxy}"
> contains white-space, we'll still forward it correctly.
> 
> But if you strongly feel your version is better, I won't fight you over it --
> although I'd love if you could then fix the quoting somehow.

done. thanks

http://chromiumcodereview.appspot.com/10375036/diff/1002/build/install-chroot...
build/install-chroot.sh:564: echo "Acquire::http::proxy \"'${http_proxy}'\";" \
On 2012/05/08 02:09:57, Markus (顧孟勤) wrote:
> While it probably is benign in this particular case, we try to always put
> double-quotes around any environment variable, especially if it was provided
by
> the user. This is not always possible, but here you could trivially add it.
> Please do so, as it will make things easier for future reviewers -- they won't
> have to scratch their heads whether there was a reason why we left of the
> quotes.

for the syntax requirement, we need a double-quotes in apt.conf such as: 
Acquire::http::proxy "http://proxy.com:8080";

So i used it in this way. 

I am not very familar with shell, can you tell me how can I get double-quotes in
the echo statement around the  environment variable? thanks

[Wei James(wistoch), 2012-05-08 02:51:34]

Markus, 

I tried serveral ways and at last get the correct output in this way:

 sudo sh -c '
    echo "Acquire::http::proxy \"'"${http_proxy}"'\";" \
        >>"/var/lib/chroot/'"${target}"'/etc/apt/apt.conf"'

patch updated. 

is it ok? thanks

[Wei James(wistoch), 2012-05-08 03:19:02]

On 2012/05/08 02:51:34, James Wei wrote:
> Markus, 
> 
> I tried serveral ways and at last get the correct output in this way:
> 
>  sudo sh -c '
>     echo "Acquire::http::proxy \"'"${http_proxy}"'\";" \
>         >>"/var/lib/chroot/'"${target}"'/etc/apt/apt.conf"'
> 
> patch updated. 
> 
> is it ok? thanks

tested the patch and it is ok. so commit it. 

thanks

[commit-bot: I haz the power, 2012-05-08 03:19:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/james.wei@intel.com/10375036/3002

[commit-bot: I haz the power, 2012-05-08 06:52:17]

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

[commit-bot: I haz the power, 2012-05-08 07:15:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/james.wei@intel.com/10375036/3002

[commit-bot: I haz the power, 2012-05-08 07:27:04]

Try job failure for 10375036-3002 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-08 07:41:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/james.wei@intel.com/10375036/3002

[commit-bot: I haz the power, 2012-05-08 10:32:09]

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

[commit-bot: I haz the power, 2012-05-08 12:36:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/james.wei@intel.com/10375036/3002

[commit-bot: I haz the power, 2012-05-08 15:10:01]

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

[commit-bot: I haz the power, 2012-05-09 00:38:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/james.wei@intel.com/10375036/3002

[commit-bot: I haz the power, 2012-05-09 02:54:44]

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

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-09 14:10:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/james.wei@intel.com/10375036/3002

[commit-bot: I haz the power, 2012-05-09 18:31:57]

Change committed as 136084

[cmp, 2012-05-09 18:40:31]

Very cool change, thanks for making it James!

[Wei James(wistoch), 2012-05-10 00:13:14]

On 2012/05/09 18:40:31, cmp wrote:
> Very cool change, thanks for making it James!

thanks :)