Allow specifying environment variables w/ enter_chroot again.

Allow specifying environment variables w/ enter_chroot again.

Examples of how people might be using enter_chroot:

1. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2 -- cmd arg1 arg2
   Set env vars and run cmd w/ args
2. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2
   Set env vars and run shell
3. ./enter_chroot [chroot_flags]   -- cmd arg1 arg2
   Run cmd w/ args
4. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2 cmd arg1 arg2
   Like #1 _if_ args aren't flags (if they are, enter_chroot will claim them)
5. ./enter_chroot [chroot_flags]   cmd arg1 arg2
   Like #3 _if_ args aren't flags (if they are, enter_chroot will claim them)

I also updated the help to indicate that whole-command quoting is no longer
supported.  If you really need whole-command quoting (maybe you want in-
chroot redirection), you'll need to use sh -c like:
  ./enter_chroot.sh -- sh -c "echo \"Save me\" > /tmp/save.txt"

You should avoid single quotes in the command and arguments.  This isn't 
a new limitation: it's shflags related.

Change-Id: I0452a8730ac9b8197834edc753b9eece69896135

BUG=chromium-os:7072
TEST=Tried a whole bunch of these commands.

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=9362fa8

[diandersAtChromium, 2010-12-16 22:08:53]

Jon,

I think this will fix things to re-allow specifying environment variables w/
enter_chroot, as documented.  PTAL and let me know.  Hopefully my solution
wasn't too hacky.


Chris / Scott: Can you also look over this and think of if there are any issues?
 I don't think there are, but I'm not quite the shell-script ninja that you guys
are.


** I will wait for all three LGTMs before submitting **

Thanks!

-Doug

[kliegs, 2010-12-16 22:26:33]

LGTM but make sure someone else signs off on the hackiness (or not) of the
implementation

From how you describe this, it brings back the functionality we had
previously which is perfect. Only question I had was the description for #2
- would that just be "runs cmd w/ args"?

My first pass at the code changes look good. It also works now with the
command lines I was testing with so is functioning correctly for my use
case.

I'm not familiar enough with other use cases to make sure this doesn't break
another workflow.  But it looks fine at a first pass.

On Thu, Dec 16, 2010 at 5:08 PM, <dianders@chromium.org> wrote:

> Reviewers: kliegs, sosa, scottz,
>
> Message:
> Jon,
>
> I think this will fix things to re-allow specifying environment variables
> w/
> enter_chroot, as documented.  PTAL and let me know.  Hopefully my solution
> wasn't too hacky.
>
>
> Chris / Scott: Can you also look over this and think of if there are any
> issues?
>  I don't think there are, but I'm not quite the shell-script ninja that you
> guys
> are.
>
>
> ** I will wait for all three LGTMs before submitting **
>
> Thanks!
>
> -Doug
>
> Description:
> Allow specifying environment variables w/ enter_chroot again.
>
> Examples of how people might be using enter_chroot:
>
> 1. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2 -- cmd arg1 arg2
>   Set env vars and run cmd w/ args
> 2. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2
>   Set env vars and run cmd w/ args
> 3. ./enter_chroot [chroot_flags]   -- cmd arg1 arg2
>   Run cmd w/ args
> 4. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2 cmd arg1 arg2
>   Like #1 _if_ args aren't flags (if they are, enter_chroot will claim
> them)
> 5. ./enter_chroot [chroot_flags]   cmd arg1 arg2
>   Like #3 _if_ args aren't flags (if they are, enter_chroot will claim
> them)
>
> I also updated the help to indicate that whole-command quoting is no longer
> supported.  If you really need whole-command quoting (maybe you want in-
> chroot redirection), you'll need to use sh -c like:
>  ./enter_chroot.sh -- sh -c "echo \"Save me\" > /tmp/save.txt"
>
> You should avoid single quotes in the command and arguments.  This isn't
> a new limitation: it's shflags related.
>
> Change-Id: I0452a8730ac9b8197834edc753b9eece69896135
>
> BUG=chromium-os:7072
> TEST=Tried a whole bunch of these commands.
>
> Please review this at http://codereview.chromium.org/5840003/
>
> SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master
>
> Affected files:
>  M enter_chroot.sh
>
>
> Index: enter_chroot.sh
> diff --git a/enter_chroot.sh b/enter_chroot.sh
> index
>
431afa3bab30d35ffc036b733946ef124e4f999f..187009b783c9caf0e7f0218cfbe24e9eb114c15f
> 100755
> --- a/enter_chroot.sh
> +++ b/enter_chroot.sh
> @@ -34,23 +34,53 @@ DEFINE_boolean unmount $FLAGS_FALSE "Only tear down
> mounts."
>  DEFINE_boolean ssh_agent $FLAGS_TRUE "Import ssh agent."
>
>  # More useful help
> -FLAGS_HELP="USAGE: $0 [flags] [VAR=value] [-- \"command\"]
> +FLAGS_HELP="USAGE: $0 [flags] [VAR=value] [-- command [arg1] [arg2] ...]
>
>  One or more VAR=value pairs can be specified to export variables into
>  the chroot environment.  For example:
>
>    $0 FOO=bar BAZ=bel
>
> -If [-- \"command\"] is present, runs the command inside the chroot,
> -after changing directory to /$USER/trunk/src/scripts.  Note that the
> -command should be enclosed in quotes to prevent interpretation by the
> -shell before getting into the chroot.  For example:
> +If [-- command] is present, runs the command inside the chroot,
> +after changing directory to /$USER/trunk/src/scripts.  Note that neither
> +the command nor args should include single quotes.  For example:
>
> -    $0 -- \"./build_platform_packages.sh\"
> +    $0 -- ./build_platform_packages.sh
>
>  Otherwise, provides an interactive shell.
>  "
>
> +# Double up on the first '--' argument.  Why?  For enter_chroot, we want
> to
> +# emulate the behavior of sudo for setting environment vars.  That is, we
> want:
> +#   ./enter_chroot [flags] [VAR=val] [-- command]
> +# ...but shflags ends up eating the '--' out of the command line and gives
> +# us back "VAR=val" and "command" together in one chunk.  By doubling up,
> we
> +# end up getting what we want back from shflags.
> +#
> +# Examples of how people might be using enter_chroot:
> +#  1. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2 -- cmd arg1 arg2
> +#     Set env vars and run cmd w/ args
> +#  2. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2
> +#     Set env vars and run cmd w/ args
> +#  3. ./enter_chroot [chroot_flags]   -- cmd arg1 arg2
> +#     Run cmd w/ args
> +#  4. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2 cmd arg1 arg2
> +#     Like #1 _if_ args aren't flags (if they are, enter_chroot will claim
> them)
> +#  5. ./enter_chroot [chroot_flags]   cmd arg1 arg2
> +#     Like #3 _if_ args aren't flags (if they are, enter_chroot will claim
> them)
> +_FLAGS_FIXED=''
> +_SAW_DASHDASH=0
> +while [ $# -gt 0 ]; do
> +  _FLAGS_FIXED="${_FLAGS_FIXED:+${_FLAGS_FIXED} }'$1'"
> +  if [[ "${_SAW_DASHDASH} $1" == "0 --" ]]; then
> +    _FLAGS_FIXED="${_FLAGS_FIXED:+${_FLAGS_FIXED} }'--'"
> +    _SAW_DASHDASH=1
> +  fi
> +  shift
> +done
> +eval set -- "${_FLAGS_FIXED}"
> +
> +
>  # Parse command line flags
>  FLAGS "$@" || exit 1
>  eval set -- "${FLAGS_ARGV}"
> @@ -303,9 +333,9 @@ git config -f ${FLAGS_chroot}/home/${USER}/.gitconfig
> --replace-all user.email \
>  # Run command or interactive shell.  Also include the non-chrooted path to
>  # the source trunk for scripts that may need to print it (e.g.
>  # build_image.sh).
> -sudo chroot "$FLAGS_chroot" sudo -i -u $USER $CHROOT_PASSTHRU \
> +sudo -- chroot "$FLAGS_chroot" sudo -i -u $USER $CHROOT_PASSTHRU \
>   EXTERNAL_TRUNK_PATH="${FLAGS_trunk}" LANG=C
> SSH_AGENT_PID="${SSH_AGENT_PID}" \
> -  SSH_AUTH_SOCK="${SSH_AUTH_SOCK}" -- "$@"
> +  SSH_AUTH_SOCK="${SSH_AUTH_SOCK}" "$@"
>
>  # Remove trap and explicitly unmount
>  trap - EXIT
>
>
>

[diandersAtChromium, 2010-12-16 22:30:21]

Jon,

Thanks!  Yes, I had a paste-o on the description for #2.  Fixed in the issue and
in the script itself.

Waiting for all three LGTMs just to be sure.

-Doug

---

On 2010/12/16 22:26:33, kliegs wrote:
> LGTM but make sure someone else signs off on the hackiness (or not) of the
> implementation
> 
> From how you describe this, it brings back the functionality we had
> previously which is perfect. Only question I had was the description for #2
> - would that just be "runs cmd w/ args"?
> 
> My first pass at the code changes look good. It also works now with the
> command lines I was testing with so is functioning correctly for my use
> case.
> 
> I'm not familiar enough with other use cases to make sure this doesn't break
> another workflow.  But it looks fine at a first pass.
> 
> On Thu, Dec 16, 2010 at 5:08 PM, <mailto:dianders@chromium.org> wrote:
> 
> > Reviewers: kliegs, sosa, scottz,
> >
> > Message:
> > Jon,
> >
> > I think this will fix things to re-allow specifying environment variables
> > w/
> > enter_chroot, as documented.  PTAL and let me know.  Hopefully my solution
> > wasn't too hacky.
> >
> >
> > Chris / Scott: Can you also look over this and think of if there are any
> > issues?
> >  I don't think there are, but I'm not quite the shell-script ninja that you
> > guys
> > are.
> >
> >
> > ** I will wait for all three LGTMs before submitting **
> >
> > Thanks!
> >
> > -Doug
> >
> > Description:
> > Allow specifying environment variables w/ enter_chroot again.
> >
> > Examples of how people might be using enter_chroot:
> >
> > 1. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2 -- cmd arg1 arg2
> >   Set env vars and run cmd w/ args
> > 2. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2
> >   Set env vars and run cmd w/ args
> > 3. ./enter_chroot [chroot_flags]   -- cmd arg1 arg2
> >   Run cmd w/ args
> > 4. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2 cmd arg1 arg2
> >   Like #1 _if_ args aren't flags (if they are, enter_chroot will claim
> > them)
> > 5. ./enter_chroot [chroot_flags]   cmd arg1 arg2
> >   Like #3 _if_ args aren't flags (if they are, enter_chroot will claim
> > them)
> >
> > I also updated the help to indicate that whole-command quoting is no longer
> > supported.  If you really need whole-command quoting (maybe you want in-
> > chroot redirection), you'll need to use sh -c like:
> >  ./enter_chroot.sh -- sh -c "echo \"Save me\" > /tmp/save.txt"
> >
> > You should avoid single quotes in the command and arguments.  This isn't
> > a new limitation: it's shflags related.
> >
> > Change-Id: I0452a8730ac9b8197834edc753b9eece69896135
> >
> > BUG=chromium-os:7072
> > TEST=Tried a whole bunch of these commands.
> >
> > Please review this at http://codereview.chromium.org/5840003/
> >
> > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master
> >
> > Affected files:
> >  M enter_chroot.sh
> >
> >
> > Index: enter_chroot.sh
> > diff --git a/enter_chroot.sh b/enter_chroot.sh
> > index
> >
>
431afa3bab30d35ffc036b733946ef124e4f999f..187009b783c9caf0e7f0218cfbe24e9eb114c15f
> > 100755
> > --- a/enter_chroot.sh
> > +++ b/enter_chroot.sh
> > @@ -34,23 +34,53 @@ DEFINE_boolean unmount $FLAGS_FALSE "Only tear down
> > mounts."
> >  DEFINE_boolean ssh_agent $FLAGS_TRUE "Import ssh agent."
> >
> >  # More useful help
> > -FLAGS_HELP="USAGE: $0 [flags] [VAR=value] [-- \"command\"]
> > +FLAGS_HELP="USAGE: $0 [flags] [VAR=value] [-- command [arg1] [arg2] ...]
> >
> >  One or more VAR=value pairs can be specified to export variables into
> >  the chroot environment.  For example:
> >
> >    $0 FOO=bar BAZ=bel
> >
> > -If [-- \"command\"] is present, runs the command inside the chroot,
> > -after changing directory to /$USER/trunk/src/scripts.  Note that the
> > -command should be enclosed in quotes to prevent interpretation by the
> > -shell before getting into the chroot.  For example:
> > +If [-- command] is present, runs the command inside the chroot,
> > +after changing directory to /$USER/trunk/src/scripts.  Note that neither
> > +the command nor args should include single quotes.  For example:
> >
> > -    $0 -- \"./build_platform_packages.sh\"
> > +    $0 -- ./build_platform_packages.sh
> >
> >  Otherwise, provides an interactive shell.
> >  "
> >
> > +# Double up on the first '--' argument.  Why?  For enter_chroot, we want
> > to
> > +# emulate the behavior of sudo for setting environment vars.  That is, we
> > want:
> > +#   ./enter_chroot [flags] [VAR=val] [-- command]
> > +# ...but shflags ends up eating the '--' out of the command line and gives
> > +# us back "VAR=val" and "command" together in one chunk.  By doubling up,
> > we
> > +# end up getting what we want back from shflags.
> > +#
> > +# Examples of how people might be using enter_chroot:
> > +#  1. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2 -- cmd arg1 arg2
> > +#     Set env vars and run cmd w/ args
> > +#  2. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2
> > +#     Set env vars and run cmd w/ args
> > +#  3. ./enter_chroot [chroot_flags]   -- cmd arg1 arg2
> > +#     Run cmd w/ args
> > +#  4. ./enter_chroot [chroot_flags]   VAR1=val1 VAR2=val2 cmd arg1 arg2
> > +#     Like #1 _if_ args aren't flags (if they are, enter_chroot will claim
> > them)
> > +#  5. ./enter_chroot [chroot_flags]   cmd arg1 arg2
> > +#     Like #3 _if_ args aren't flags (if they are, enter_chroot will claim
> > them)
> > +_FLAGS_FIXED=''
> > +_SAW_DASHDASH=0
> > +while [ $# -gt 0 ]; do
> > +  _FLAGS_FIXED="${_FLAGS_FIXED:+${_FLAGS_FIXED} }'$1'"
> > +  if [[ "${_SAW_DASHDASH} $1" == "0 --" ]]; then
> > +    _FLAGS_FIXED="${_FLAGS_FIXED:+${_FLAGS_FIXED} }'--'"
> > +    _SAW_DASHDASH=1
> > +  fi
> > +  shift
> > +done
> > +eval set -- "${_FLAGS_FIXED}"
> > +
> > +
> >  # Parse command line flags
> >  FLAGS "$@" || exit 1
> >  eval set -- "${FLAGS_ARGV}"
> > @@ -303,9 +333,9 @@ git config -f ${FLAGS_chroot}/home/${USER}/.gitconfig
> > --replace-all user.email \
> >  # Run command or interactive shell.  Also include the non-chrooted path to
> >  # the source trunk for scripts that may need to print it (e.g.
> >  # build_image.sh).
> > -sudo chroot "$FLAGS_chroot" sudo -i -u $USER $CHROOT_PASSTHRU \
> > +sudo -- chroot "$FLAGS_chroot" sudo -i -u $USER $CHROOT_PASSTHRU \
> >   EXTERNAL_TRUNK_PATH="${FLAGS_trunk}" LANG=C
> > SSH_AGENT_PID="${SSH_AGENT_PID}" \
> > -  SSH_AUTH_SOCK="${SSH_AUTH_SOCK}" -- "$@"
> > +  SSH_AUTH_SOCK="${SSH_AUTH_SOCK}" "$@"
> >
> >  # Remove trap and explicitly unmount
> >  trap - EXIT
> >
> >
> >

[sosa, 2010-12-16 22:32:44]

Can you make this less bash-y and more sh-y?

http://codereview.chromium.org/5840003/diff/1/enter_chroot.sh
File enter_chroot.sh (right):

http://codereview.chromium.org/5840003/diff/1/enter_chroot.sh#newcode74
enter_chroot.sh:74: _FLAGS_FIXED="${_FLAGS_FIXED:+${_FLAGS_FIXED} }'$1'"
can you do this less bashy?  i.e. _FLAGS_FIXED="${_FLAGS_FIXED} $1"

http://codereview.chromium.org/5840003/diff/1/enter_chroot.sh#newcode75
enter_chroot.sh:75: if [[ "${_SAW_DASHDASH} $1" == "0 --" ]]; then
Prefer if you split this into two checks that are and'd i.e. [$_SAW_DASHDASH -eq
0 ] && [[ $1 == "--" ]]; then

[scottz, 2010-12-16 22:32:47]

This also seems fine to me. I don't foresee any problems but I know this code is
far reaching so just make sure you test it in as many cases as you find. 

LGTM

[diandersAtChromium, 2010-12-16 22:43:01]

Thanks to everyone for the quick reviews.  I'm going to push this...

http://codereview.chromium.org/5840003/diff/1/enter_chroot.sh
File enter_chroot.sh (right):

http://codereview.chromium.org/5840003/diff/1/enter_chroot.sh#newcode74
enter_chroot.sh:74: _FLAGS_FIXED="${_FLAGS_FIXED:+${_FLAGS_FIXED} }'$1'"
Going to leave this, since it's copied almost verbatim from shflags.

On 2010/12/16 22:32:45, sosa wrote:
> can you do this less bashy?  i.e. _FLAGS_FIXED="${_FLAGS_FIXED} $1"

http://codereview.chromium.org/5840003/diff/1/enter_chroot.sh#newcode75
enter_chroot.sh:75: if [[ "${_SAW_DASHDASH} $1" == "0 --" ]]; then
On 2010/12/16 22:32:45, sosa wrote:
> Prefer if you split this into two checks that are and'd i.e. [$_SAW_DASHDASH
-eq
> 0 ] && [[ $1 == "--" ]]; then

Done.