Passing AUTOSERV_TEST_ARGS as arguments to autoserv so spaces are preserved between test arguments.

Passing AUTOSERV_TEST_ARGS as arguments to autoserv so spaces are preserved between test arguments.

[whywhat, 2010-04-28 12:44:37]

Please, take a look. This is for http://codereview.chromium.org/1794004/show

[ericli, 2010-04-28 15:40:39]

Hello, Anton,

I am not sure to push an extra env var into ebuild is the right most
solution. I take a look at src/script/autotest, and I believe the issue here
is line 193
environ['AUTOSERV_ARGS'] = ' '.join(args),

so if you have a space in the your args value, you screwed.
Since the args will be reformatted as  --args Hello World and passed into
autoserv command.

So I think the right solution is to put quote around arg values:

some thing like this:

args1 = []
for arg in args:
  if args.startwith('-'):    # this is an args name
     pass
  else:   # this is an args value
     arg = '"%s"' % arg
  args1.append(arg)
environ['AUTOSERV_ARGS'] = ' '.join(args1)



On Wed, Apr 28, 2010 at 5:44 AM, <avayvod@chromium.org> wrote:

> Reviewers: petkov, ericli,
>
> Message:
> Please, take a look. This is for
> http://codereview.chromium.org/1794004/show
>
> Description:
> Passing AUTOSERV_TEST_ARGS as arguments to autoserv so spaces are preserved
> between test arguments.
>
> Please review this at http://codereview.chromium.org/1787008/show
>
> SVN Base: ssh://git@chromiumos-git/chromiumos-overlay.git
>
> Affected files:
>  M chromeos-base/autotest/autotest-0.0.1.ebuild
>
>
> Index: chromeos-base/autotest/autotest-0.0.1.ebuild
> diff --git a/chromeos-base/autotest/autotest-0.0.1.ebuild
> b/chromeos-base/autotest/autotest-0.0.1.ebuild
> index
>
004b5086606a10c8001f23f650885f94c24718bf..89f160ef66b9057931289cedac96be441d543c21
> 100644
> --- a/chromeos-base/autotest/autotest-0.0.1.ebuild
> +++ b/chromeos-base/autotest/autotest-0.0.1.ebuild
> @@ -127,10 +127,15 @@ src_test() {
>
>        setup_cross_toolchain
>
> +        local args=()
> +        if [[ -n ${AUTOSERV_TEST_ARGS} ]] ; then
> +          args=("-a" "${AUTOSERV_TEST_ARGS}")
> +        fi
> +
>        local timestamp=$(date +%Y-%m-%d-%H.%M.%S)
>        # Do not use sudo, it'll unset all your environment
>        LOGNAME=${SUDO_USER} ./server/autoserv -r /tmp/results.${timestamp}
> \
> -               ${AUTOSERV_ARGS}
> +               ${AUTOSERV_ARGS} "${args[@]}"
>        teardown_ssh
>  }
>
>
>
>


-- 
Eric Li
李咏竹
Google Kirkland

[whywhat, 2010-04-28 15:56:43]

Hi Eric,

I tried something like that before adding new variable. "" in
${AUTOSERV_ARGS} is ignored so I was getting |"param1| and |param2"| in args
of the test.

This is the minimal working solution that I found. The other way would be
finding -a and the next param in AUTOSERV_ARGS in ebuild but I considered
doing it in Python as less effort thing.

On Wed, Apr 28, 2010 at 7:40 PM, Eric Li(李咏竹) <ericli@chromium.org> wrote:

> Hello, Anton,
>
> I am not sure to push an extra env var into ebuild is the right most
> solution. I take a look at src/script/autotest, and I believe the issue here
> is line 193
> environ['AUTOSERV_ARGS'] = ' '.join(args),
>
> so if you have a space in the your args value, you screwed.
> Since the args will be reformatted as  --args Hello World and passed into
> autoserv command.
>
> So I think the right solution is to put quote around arg values:
>
> some thing like this:
>
> args1 = []
> for arg in args:
>   if args.startwith('-'):    # this is an args name
>      pass
>   else:   # this is an args value
>      arg = '"%s"' % arg
>   args1.append(arg)
> environ['AUTOSERV_ARGS'] = ' '.join(args1)
>
>
>
> On Wed, Apr 28, 2010 at 5:44 AM, <avayvod@chromium.org> wrote:
>
>> Reviewers: petkov, ericli,
>>
>> Message:
>> Please, take a look. This is for
>> http://codereview.chromium.org/1794004/show
>>
>> Description:
>> Passing AUTOSERV_TEST_ARGS as arguments to autoserv so spaces are
>> preserved
>> between test arguments.
>>
>> Please review this at http://codereview.chromium.org/1787008/show
>>
>> SVN Base: ssh://git@chromiumos-git/chromiumos-overlay.git
>>
>> Affected files:
>>  M chromeos-base/autotest/autotest-0.0.1.ebuild
>>
>>
>> Index: chromeos-base/autotest/autotest-0.0.1.ebuild
>> diff --git a/chromeos-base/autotest/autotest-0.0.1.ebuild
>> b/chromeos-base/autotest/autotest-0.0.1.ebuild
>> index
>>
004b5086606a10c8001f23f650885f94c24718bf..89f160ef66b9057931289cedac96be441d543c21
>> 100644
>> --- a/chromeos-base/autotest/autotest-0.0.1.ebuild
>> +++ b/chromeos-base/autotest/autotest-0.0.1.ebuild
>> @@ -127,10 +127,15 @@ src_test() {
>>
>>        setup_cross_toolchain
>>
>> +        local args=()
>> +        if [[ -n ${AUTOSERV_TEST_ARGS} ]] ; then
>> +          args=("-a" "${AUTOSERV_TEST_ARGS}")
>> +        fi
>> +
>>        local timestamp=$(date +%Y-%m-%d-%H.%M.%S)
>>        # Do not use sudo, it'll unset all your environment
>>        LOGNAME=${SUDO_USER} ./server/autoserv -r /tmp/results.${timestamp}
>> \
>> -               ${AUTOSERV_ARGS}
>> +               ${AUTOSERV_ARGS} "${args[@]}"
>>        teardown_ssh
>>  }
>>
>>
>>
>>
>
>
> --
> Eric Li
> 李咏竹
> Google Kirkland
>
>
>

[ericli, 2010-04-28 16:43:12]

I am LGTM this for now in order to unblock your work. I will keep looking this.


On 2010/04/28 15:56:43, whywhat wrote:
> Hi Eric,
> 
> I tried something like that before adding new variable. "" in
> ${AUTOSERV_ARGS} is ignored so I was getting |"param1| and |param2"| in args
> of the test.
> 
> This is the minimal working solution that I found. The other way would be
> finding -a and the next param in AUTOSERV_ARGS in ebuild but I considered
> doing it in Python as less effort thing.
> 
> On Wed, Apr 28, 2010 at 7:40 PM, Eric Li(李咏竹) <mailto:ericli@chromium.org>
wrote:
> 
> > Hello, Anton,
> >
> > I am not sure to push an extra env var into ebuild is the right most
> > solution. I take a look at src/script/autotest, and I believe the issue here
> > is line 193
> > environ['AUTOSERV_ARGS'] = ' '.join(args),
> >
> > so if you have a space in the your args value, you screwed.
> > Since the args will be reformatted as  --args Hello World and passed into
> > autoserv command.
> >
> > So I think the right solution is to put quote around arg values:
> >
> > some thing like this:
> >
> > args1 = []
> > for arg in args:
> >   if args.startwith('-'):    # this is an args name
> >      pass
> >   else:   # this is an args value
> >      arg = '"%s"' % arg
> >   args1.append(arg)
> > environ['AUTOSERV_ARGS'] = ' '.join(args1)
> >
> >
> >
> > On Wed, Apr 28, 2010 at 5:44 AM, <mailto:avayvod@chromium.org> wrote:
> >
> >> Reviewers: petkov, ericli,
> >>
> >> Message:
> >> Please, take a look. This is for
> >> http://codereview.chromium.org/1794004/show
> >>
> >> Description:
> >> Passing AUTOSERV_TEST_ARGS as arguments to autoserv so spaces are
> >> preserved
> >> between test arguments.
> >>
> >> Please review this at http://codereview.chromium.org/1787008/show
> >>
> >> SVN Base: ssh://git@chromiumos-git/chromiumos-overlay.git
> >>
> >> Affected files:
> >>  M chromeos-base/autotest/autotest-0.0.1.ebuild
> >>
> >>
> >> Index: chromeos-base/autotest/autotest-0.0.1.ebuild
> >> diff --git a/chromeos-base/autotest/autotest-0.0.1.ebuild
> >> b/chromeos-base/autotest/autotest-0.0.1.ebuild
> >> index
> >>
>
004b5086606a10c8001f23f650885f94c24718bf..89f160ef66b9057931289cedac96be441d543c21
> >> 100644
> >> --- a/chromeos-base/autotest/autotest-0.0.1.ebuild
> >> +++ b/chromeos-base/autotest/autotest-0.0.1.ebuild
> >> @@ -127,10 +127,15 @@ src_test() {
> >>
> >>        setup_cross_toolchain
> >>
> >> +        local args=()
> >> +        if [[ -n ${AUTOSERV_TEST_ARGS} ]] ; then
> >> +          args=("-a" "${AUTOSERV_TEST_ARGS}")
> >> +        fi
> >> +
> >>        local timestamp=$(date +%Y-%m-%d-%H.%M.%S)
> >>        # Do not use sudo, it'll unset all your environment
> >>        LOGNAME=${SUDO_USER} ./server/autoserv -r /tmp/results.${timestamp}
> >> \
> >> -               ${AUTOSERV_ARGS}
> >> +               ${AUTOSERV_ARGS} "${args[@]}"
> >>        teardown_ssh
> >>  }
> >>
> >>
> >>
> >>
> >
> >
> > --
> > Eric Li
> > 李咏竹
> > Google Kirkland
> >
> >
> >
>

[petkov, 2010-04-28 16:53:14]

I guess you pushed this already but isn't passing "" to autoserv bad? I.e.,
doesn't it get confused by the empty argument?

If it doesn't, LGTM2.

[whywhat, 2010-04-28 16:55:21]

Works for me with no args. Please, take a look at the other change:
http://codereview.chromium.org/1794004/show