Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1603004:
remove build_autotest.sh and change run_remote_tests.sh to use new autotest script. (Closed)
Descriptionremove build_autotest.sh and change run_remote_tests.sh to use new autotest script.
This change should go with
http://codereview.chromium.org/1545014
together.
Patch Set 1 #Patch Set 2 : patch 1 #
Total comments: 9
Patch Set 3 : patch 3 #Patch Set 4 : patch 5. #Patch Set 5 : patch 5. #Patch Set 6 : patch 6 #Patch Set 7 : patch 7. #Patch Set 8 : patch 7 #
Total comments: 2
Patch Set 9 : patch #
Messages
Total messages: 14 (0 generated)
http://codereview.chromium.org/1603004/diff/2001/3003 File src/scripts/run_remote_tests.sh (left): http://codereview.chromium.org/1603004/diff/2001/3003#oldcode263 src/scripts/run_remote_tests.sh:263: ${enter_chroot} ./build_autotest.sh --board=${FLAGS_board} ${build_param} why did you remove this functionality? All you had to do is change build_autotest.sh to autotest. http://codereview.chromium.org/1603004/diff/2001/3003 File src/scripts/run_remote_tests.sh (right): http://codereview.chromium.org/1603004/diff/2001/3003#newcode27 src/scripts/run_remote_tests.sh:27: assert_inside_chroot why? http://codereview.chromium.org/1603004/diff/2001/3003#newcode139 src/scripts/run_remote_tests.sh:139: '^[[:space:]]*TEST_TYPE[[:space:]]*=' "${autotest_dir}/${control_file}") 80 chars - though I'm not sure why you don't just pass this in as part of the first param. http://codereview.chromium.org/1603004/diff/2001/3003#newcode254 src/scripts/run_remote_tests.sh:254: ${autotest} --board "${FLAGS_board}" -m "${FLAGS_remote}" "${option}" "${control_file}" \ 80 chars http://codereview.chromium.org/1603004/diff/2001/3003#newcode255 src/scripts/run_remote_tests.sh:255: -r "${results_dir}" "${verbose}" If verbose isn't set this passes an extra empty param in which is why I didn't quote it.
I am also fixing an autotest issue, when no command line argument provided, should print out help message. http://codereview.chromium.org/1603004/diff/2001/3003 File src/scripts/run_remote_tests.sh (right): http://codereview.chromium.org/1603004/diff/2001/3003#newcode27 src/scripts/run_remote_tests.sh:27: assert_inside_chroot discussed offline. On 2010/04/01 23:48:28, kmixter1 wrote: > why? http://codereview.chromium.org/1603004/diff/2001/3003#newcode139 src/scripts/run_remote_tests.sh:139: '^[[:space:]]*TEST_TYPE[[:space:]]*=' "${autotest_dir}/${control_file}") On 2010/04/01 23:48:28, kmixter1 wrote: > 80 chars - though I'm not sure why you don't just pass this in as part of the > first param. Done. http://codereview.chromium.org/1603004/diff/2001/3003#newcode254 src/scripts/run_remote_tests.sh:254: ${autotest} --board "${FLAGS_board}" -m "${FLAGS_remote}" "${option}" "${control_file}" \ On 2010/04/01 23:48:28, kmixter1 wrote: > 80 chars Done. http://codereview.chromium.org/1603004/diff/2001/3003#newcode255 src/scripts/run_remote_tests.sh:255: -r "${results_dir}" "${verbose}" On 2010/04/01 23:48:28, kmixter1 wrote: > If verbose isn't set this passes an extra empty param in which is why I didn't > quote it. Done.
For this CL, I would prefer if you didn't delete build_autotest.sh. During the test fix-it we taught / devs taught themselves to use build_autotest if things didn't work. I want to keep the support of the entire old workflow until we are ready to move to a whole new one (vs. piece-wise) On Thu, Apr 1, 2010 at 9:43 PM, <ericli@chromium.org> wrote: > I am also fixing an autotest issue, when no command line argument provided, > should print out help message. > > > http://codereview.chromium.org/1603004/diff/2001/3003 > File src/scripts/run_remote_tests.sh (right): > > http://codereview.chromium.org/1603004/diff/2001/3003#newcode27 > src/scripts/run_remote_tests.sh:27: assert_inside_chroot > discussed offline. > > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> why? > > http://codereview.chromium.org/1603004/diff/2001/3003#newcode139 > src/scripts/run_remote_tests.sh:139: > '^[[:space:]]*TEST_TYPE[[:space:]]*=' "${autotest_dir}/${control_file}") > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> 80 chars - though I'm not sure why you don't just pass this in as part > > of the >> >> first param. > > Done. > > http://codereview.chromium.org/1603004/diff/2001/3003#newcode254 > src/scripts/run_remote_tests.sh:254: ${autotest} --board > "${FLAGS_board}" -m "${FLAGS_remote}" "${option}" "${control_file}" \ > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> 80 chars > > Done. > > http://codereview.chromium.org/1603004/diff/2001/3003#newcode255 > src/scripts/run_remote_tests.sh:255: -r "${results_dir}" "${verbose}" > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> If verbose isn't set this passes an extra empty param in which is why > > I didn't >> >> quote it. > > Done. > > http://codereview.chromium.org/1603004 >
This is reasonable request. Done! On Fri, Apr 2, 2010 at 10:07 AM, Chris Sosa <sosa@chromium.org> wrote: > For this CL, I would prefer if you didn't delete build_autotest.sh. > During the test fix-it we taught / devs taught themselves to use > build_autotest if things didn't work. I want to keep the support of > the entire old workflow until we are ready to move to a whole new one > (vs. piece-wise) > > On Thu, Apr 1, 2010 at 9:43 PM, <ericli@chromium.org> wrote: > > I am also fixing an autotest issue, when no command line argument > provided, > > should print out help message. > > > > > > http://codereview.chromium.org/1603004/diff/2001/3003 > > File src/scripts/run_remote_tests.sh (right): > > > > http://codereview.chromium.org/1603004/diff/2001/3003#newcode27 > > src/scripts/run_remote_tests.sh:27: assert_inside_chroot > > discussed offline. > > > > On 2010/04/01 23:48:28, kmixter1 wrote: > >> > >> why? > > > > http://codereview.chromium.org/1603004/diff/2001/3003#newcode139 > > src/scripts/run_remote_tests.sh:139: > > '^[[:space:]]*TEST_TYPE[[:space:]]*=' "${autotest_dir}/${control_file}") > > On 2010/04/01 23:48:28, kmixter1 wrote: > >> > >> 80 chars - though I'm not sure why you don't just pass this in as part > > > > of the > >> > >> first param. > > > > Done. > > > > http://codereview.chromium.org/1603004/diff/2001/3003#newcode254 > > src/scripts/run_remote_tests.sh:254: ${autotest} --board > > "${FLAGS_board}" -m "${FLAGS_remote}" "${option}" "${control_file}" \ > > On 2010/04/01 23:48:28, kmixter1 wrote: > >> > >> 80 chars > > > > Done. > > > > http://codereview.chromium.org/1603004/diff/2001/3003#newcode255 > > src/scripts/run_remote_tests.sh:255: -r "${results_dir}" "${verbose}" > > On 2010/04/01 23:48:28, kmixter1 wrote: > >> > >> If verbose isn't set this passes an extra empty param in which is why > > > > I didn't > >> > >> quote it. > > > > Done. > > > > http://codereview.chromium.org/1603004 > > > -- Eric Li 李咏竹 Google Kirkland
Seems like you could make build_autotest just call autotest - at least then the workflow is the same but you're testing the new code. Of course that assumes that you really trust it. On Fri, Apr 2, 2010 at 10:26 AM, Eric Li(李咏竹) <ericli@chromium.org> wrote: > This is reasonable request. Done! > > On Fri, Apr 2, 2010 at 10:07 AM, Chris Sosa <sosa@chromium.org> wrote: >> >> For this CL, I would prefer if you didn't delete build_autotest.sh. >> During the test fix-it we taught / devs taught themselves to use >> build_autotest if things didn't work. I want to keep the support of >> the entire old workflow until we are ready to move to a whole new one >> (vs. piece-wise) >> >> On Thu, Apr 1, 2010 at 9:43 PM, <ericli@chromium.org> wrote: >> > I am also fixing an autotest issue, when no command line argument >> > provided, >> > should print out help message. >> > >> > >> > http://codereview.chromium.org/1603004/diff/2001/3003 >> > File src/scripts/run_remote_tests.sh (right): >> > >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode27 >> > src/scripts/run_remote_tests.sh:27: assert_inside_chroot >> > discussed offline. >> > >> > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> >> >> why? >> > >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode139 >> > src/scripts/run_remote_tests.sh:139: >> > '^[[:space:]]*TEST_TYPE[[:space:]]*=' "${autotest_dir}/${control_file}") >> > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> >> >> 80 chars - though I'm not sure why you don't just pass this in as part >> > >> > of the >> >> >> >> first param. >> > >> > Done. >> > >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode254 >> > src/scripts/run_remote_tests.sh:254: ${autotest} --board >> > "${FLAGS_board}" -m "${FLAGS_remote}" "${option}" "${control_file}" \ >> > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> >> >> 80 chars >> > >> > Done. >> > >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode255 >> > src/scripts/run_remote_tests.sh:255: -r "${results_dir}" "${verbose}" >> > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> >> >> If verbose isn't set this passes an extra empty param in which is why >> > >> > I didn't >> >> >> >> quote it. >> > >> > Done. >> > >> > http://codereview.chromium.org/1603004 >> > > > > > -- > Eric Li > 李咏竹 > Google Kirkland > > >
build_autotest is already replaced by autotest inside buildbot, and with all the upsteam changes I made couple of days ago. So it should be trusted. I can make build_autotest to call autotest, if you feel that is the right thing to do. My personal motivation here is to keep less scripts under the directory, since that even confuse people more, and do a sharp surgical cut. But I think I need to be a team player. On Fri, Apr 2, 2010 at 12:53 PM, Ken Mixter <kmixter@chromium.org> wrote: > Seems like you could make build_autotest just call autotest - at least > then the workflow is the same but you're testing the new code. Of > course that assumes that you really trust it. > > On Fri, Apr 2, 2010 at 10:26 AM, Eric Li(李咏竹) <ericli@chromium.org> wrote: > > This is reasonable request. Done! > > > > On Fri, Apr 2, 2010 at 10:07 AM, Chris Sosa <sosa@chromium.org> wrote: > >> > >> For this CL, I would prefer if you didn't delete build_autotest.sh. > >> During the test fix-it we taught / devs taught themselves to use > >> build_autotest if things didn't work. I want to keep the support of > >> the entire old workflow until we are ready to move to a whole new one > >> (vs. piece-wise) > >> > >> On Thu, Apr 1, 2010 at 9:43 PM, <ericli@chromium.org> wrote: > >> > I am also fixing an autotest issue, when no command line argument > >> > provided, > >> > should print out help message. > >> > > >> > > >> > http://codereview.chromium.org/1603004/diff/2001/3003 > >> > File src/scripts/run_remote_tests.sh (right): > >> > > >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode27 > >> > src/scripts/run_remote_tests.sh:27: assert_inside_chroot > >> > discussed offline. > >> > > >> > On 2010/04/01 23:48:28, kmixter1 wrote: > >> >> > >> >> why? > >> > > >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode139 > >> > src/scripts/run_remote_tests.sh:139: > >> > '^[[:space:]]*TEST_TYPE[[:space:]]*=' > "${autotest_dir}/${control_file}") > >> > On 2010/04/01 23:48:28, kmixter1 wrote: > >> >> > >> >> 80 chars - though I'm not sure why you don't just pass this in as > part > >> > > >> > of the > >> >> > >> >> first param. > >> > > >> > Done. > >> > > >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode254 > >> > src/scripts/run_remote_tests.sh:254: ${autotest} --board > >> > "${FLAGS_board}" -m "${FLAGS_remote}" "${option}" "${control_file}" \ > >> > On 2010/04/01 23:48:28, kmixter1 wrote: > >> >> > >> >> 80 chars > >> > > >> > Done. > >> > > >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode255 > >> > src/scripts/run_remote_tests.sh:255: -r "${results_dir}" "${verbose}" > >> > On 2010/04/01 23:48:28, kmixter1 wrote: > >> >> > >> >> If verbose isn't set this passes an extra empty param in which is why > >> > > >> > I didn't > >> >> > >> >> quote it. > >> > > >> > Done. > >> > > >> > http://codereview.chromium.org/1603004 > >> > > > > > > > > > -- > > Eric Li > > 李咏竹 > > Google Kirkland > > > > > > > -- Eric Li 李咏竹 Google Kirkland
I think so - I agree - we should have fewer scripts too. I'm just proposing this as a temporary - build_autotest as the command stays for people used to that workflow - but it just runs the new script. Then when we're ready to announce the new stuff we erase build_autotest. Just that way the old flow works and you also have your new code tested. On Fri, Apr 2, 2010 at 1:08 PM, Eric Li(李咏竹) <ericli@chromium.org> wrote: > build_autotest is already replaced by autotest inside buildbot, and with all > the upsteam changes I made couple of days ago. So it should be trusted. > > I can make build_autotest to call autotest, if you feel that is the right > thing to do. My personal motivation here is to keep less scripts under the > directory, since that even confuse people more, and do a sharp surgical cut. > But I think I need to be a team player. > > > On Fri, Apr 2, 2010 at 12:53 PM, Ken Mixter <kmixter@chromium.org> wrote: >> >> Seems like you could make build_autotest just call autotest - at least >> then the workflow is the same but you're testing the new code. Of >> course that assumes that you really trust it. >> >> On Fri, Apr 2, 2010 at 10:26 AM, Eric Li(李咏竹) <ericli@chromium.org> wrote: >> > This is reasonable request. Done! >> > >> > On Fri, Apr 2, 2010 at 10:07 AM, Chris Sosa <sosa@chromium.org> wrote: >> >> >> >> For this CL, I would prefer if you didn't delete build_autotest.sh. >> >> During the test fix-it we taught / devs taught themselves to use >> >> build_autotest if things didn't work. I want to keep the support of >> >> the entire old workflow until we are ready to move to a whole new one >> >> (vs. piece-wise) >> >> >> >> On Thu, Apr 1, 2010 at 9:43 PM, <ericli@chromium.org> wrote: >> >> > I am also fixing an autotest issue, when no command line argument >> >> > provided, >> >> > should print out help message. >> >> > >> >> > >> >> > http://codereview.chromium.org/1603004/diff/2001/3003 >> >> > File src/scripts/run_remote_tests.sh (right): >> >> > >> >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode27 >> >> > src/scripts/run_remote_tests.sh:27: assert_inside_chroot >> >> > discussed offline. >> >> > >> >> > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> >> >> >> >> why? >> >> > >> >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode139 >> >> > src/scripts/run_remote_tests.sh:139: >> >> > '^[[:space:]]*TEST_TYPE[[:space:]]*=' >> >> > "${autotest_dir}/${control_file}") >> >> > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> >> >> >> >> 80 chars - though I'm not sure why you don't just pass this in as >> >> >> part >> >> > >> >> > of the >> >> >> >> >> >> first param. >> >> > >> >> > Done. >> >> > >> >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode254 >> >> > src/scripts/run_remote_tests.sh:254: ${autotest} --board >> >> > "${FLAGS_board}" -m "${FLAGS_remote}" "${option}" "${control_file}" \ >> >> > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> >> >> >> >> 80 chars >> >> > >> >> > Done. >> >> > >> >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode255 >> >> > src/scripts/run_remote_tests.sh:255: -r "${results_dir}" "${verbose}" >> >> > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> >> >> >> >> If verbose isn't set this passes an extra empty param in which is >> >> >> why >> >> > >> >> > I didn't >> >> >> >> >> >> quote it. >> >> > >> >> > Done. >> >> > >> >> > http://codereview.chromium.org/1603004 >> >> > >> > >> > >> > >> > -- >> > Eric Li >> > 李咏竹 >> > Google Kirkland >> > >> > >> > > > > > -- > Eric Li > 李咏竹 > Google Kirkland > > >
On Fri, Apr 2, 2010 at 1:08 PM, Eric Li(李咏竹) <ericli@chromium.org> wrote: > build_autotest is already replaced by autotest inside buildbot, and with > all the upsteam changes I made couple of days ago. So it should be trusted. > > I can make build_autotest to call autotest, if you feel that is the right > thing to do. My personal motivation here is to keep less scripts under the > directory, since that even confuse people more, and do a sharp surgical cut. > But I think I need to be a team player. > Speaking of fewer scripts -- is there much preventing you from merging autotest.py and autotest into a single Python script? > > > > On Fri, Apr 2, 2010 at 12:53 PM, Ken Mixter <kmixter@chromium.org> wrote: > >> Seems like you could make build_autotest just call autotest - at least >> then the workflow is the same but you're testing the new code. Of >> course that assumes that you really trust it. >> >> On Fri, Apr 2, 2010 at 10:26 AM, Eric Li(李咏竹) <ericli@chromium.org> >> wrote: >> > This is reasonable request. Done! >> > >> > On Fri, Apr 2, 2010 at 10:07 AM, Chris Sosa <sosa@chromium.org> wrote: >> >> >> >> For this CL, I would prefer if you didn't delete build_autotest.sh. >> >> During the test fix-it we taught / devs taught themselves to use >> >> build_autotest if things didn't work. I want to keep the support of >> >> the entire old workflow until we are ready to move to a whole new one >> >> (vs. piece-wise) >> >> >> >> On Thu, Apr 1, 2010 at 9:43 PM, <ericli@chromium.org> wrote: >> >> > I am also fixing an autotest issue, when no command line argument >> >> > provided, >> >> > should print out help message. >> >> > >> >> > >> >> > http://codereview.chromium.org/1603004/diff/2001/3003 >> >> > File src/scripts/run_remote_tests.sh (right): >> >> > >> >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode27 >> >> > src/scripts/run_remote_tests.sh:27: assert_inside_chroot >> >> > discussed offline. >> >> > >> >> > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> >> >> >> >> why? >> >> > >> >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode139 >> >> > src/scripts/run_remote_tests.sh:139: >> >> > '^[[:space:]]*TEST_TYPE[[:space:]]*=' >> "${autotest_dir}/${control_file}") >> >> > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> >> >> >> >> 80 chars - though I'm not sure why you don't just pass this in as >> part >> >> > >> >> > of the >> >> >> >> >> >> first param. >> >> > >> >> > Done. >> >> > >> >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode254 >> >> > src/scripts/run_remote_tests.sh:254: ${autotest} --board >> >> > "${FLAGS_board}" -m "${FLAGS_remote}" "${option}" "${control_file}" \ >> >> > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> >> >> >> >> 80 chars >> >> > >> >> > Done. >> >> > >> >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode255 >> >> > src/scripts/run_remote_tests.sh:255: -r "${results_dir}" "${verbose}" >> >> > On 2010/04/01 23:48:28, kmixter1 wrote: >> >> >> >> >> >> If verbose isn't set this passes an extra empty param in which is >> why >> >> > >> >> > I didn't >> >> >> >> >> >> quote it. >> >> > >> >> > Done. >> >> > >> >> > http://codereview.chromium.org/1603004 >> >> > >> > >> > >> > >> > -- >> > Eric Li >> > 李咏竹 >> > Google Kirkland >> > >> > >> > >> > > > > -- > Eric Li > 李咏竹 > Google Kirkland > > >
To Darin: No. I thought about it at the beginning and I do not want to reimplement assert_inside_chroot in python. To Ken/Chris: I step back. I think the whole point here is: You dont need to call build_autotest.sh any more in your test process. You just call run_remote_tests.sh and build will happen transparently. So I dont see any reason I should change build_autotest.sh call autotest. Yes, you can call it as usual, first build_autotest.sh and then run_remote_tests.sh, this is not a disruptive process, but that is just a duplication of work. On Fri, Apr 2, 2010 at 1:16 PM, Darin Petkov <petkov@chromium.org> wrote: > > > On Fri, Apr 2, 2010 at 1:08 PM, Eric Li(李咏竹) <ericli@chromium.org> wrote: > >> build_autotest is already replaced by autotest inside buildbot, and with >> all the upsteam changes I made couple of days ago. So it should be trusted. >> >> I can make build_autotest to call autotest, if you feel that is the right >> thing to do. My personal motivation here is to keep less scripts under the >> directory, since that even confuse people more, and do a sharp surgical cut. >> But I think I need to be a team player. >> > > Speaking of fewer scripts -- is there much preventing you from merging > autotest.py and autotest into a single Python script? > > >> >> >> >> On Fri, Apr 2, 2010 at 12:53 PM, Ken Mixter <kmixter@chromium.org> wrote: >> >>> Seems like you could make build_autotest just call autotest - at least >>> then the workflow is the same but you're testing the new code. Of >>> course that assumes that you really trust it. >>> >>> On Fri, Apr 2, 2010 at 10:26 AM, Eric Li(李咏竹) <ericli@chromium.org> >>> wrote: >>> > This is reasonable request. Done! >>> > >>> > On Fri, Apr 2, 2010 at 10:07 AM, Chris Sosa <sosa@chromium.org> wrote: >>> >> >>> >> For this CL, I would prefer if you didn't delete build_autotest.sh. >>> >> During the test fix-it we taught / devs taught themselves to use >>> >> build_autotest if things didn't work. I want to keep the support of >>> >> the entire old workflow until we are ready to move to a whole new one >>> >> (vs. piece-wise) >>> >> >>> >> On Thu, Apr 1, 2010 at 9:43 PM, <ericli@chromium.org> wrote: >>> >> > I am also fixing an autotest issue, when no command line argument >>> >> > provided, >>> >> > should print out help message. >>> >> > >>> >> > >>> >> > http://codereview.chromium.org/1603004/diff/2001/3003 >>> >> > File src/scripts/run_remote_tests.sh (right): >>> >> > >>> >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode27 >>> >> > src/scripts/run_remote_tests.sh:27: assert_inside_chroot >>> >> > discussed offline. >>> >> > >>> >> > On 2010/04/01 23:48:28, kmixter1 wrote: >>> >> >> >>> >> >> why? >>> >> > >>> >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode139 >>> >> > src/scripts/run_remote_tests.sh:139: >>> >> > '^[[:space:]]*TEST_TYPE[[:space:]]*=' >>> "${autotest_dir}/${control_file}") >>> >> > On 2010/04/01 23:48:28, kmixter1 wrote: >>> >> >> >>> >> >> 80 chars - though I'm not sure why you don't just pass this in as >>> part >>> >> > >>> >> > of the >>> >> >> >>> >> >> first param. >>> >> > >>> >> > Done. >>> >> > >>> >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode254 >>> >> > src/scripts/run_remote_tests.sh:254: ${autotest} --board >>> >> > "${FLAGS_board}" -m "${FLAGS_remote}" "${option}" "${control_file}" >>> \ >>> >> > On 2010/04/01 23:48:28, kmixter1 wrote: >>> >> >> >>> >> >> 80 chars >>> >> > >>> >> > Done. >>> >> > >>> >> > http://codereview.chromium.org/1603004/diff/2001/3003#newcode255 >>> >> > src/scripts/run_remote_tests.sh:255: -r "${results_dir}" >>> "${verbose}" >>> >> > On 2010/04/01 23:48:28, kmixter1 wrote: >>> >> >> >>> >> >> If verbose isn't set this passes an extra empty param in which is >>> why >>> >> > >>> >> > I didn't >>> >> >> >>> >> >> quote it. >>> >> > >>> >> > Done. >>> >> > >>> >> > http://codereview.chromium.org/1603004 >>> >> > >>> > >>> > >>> > >>> > -- >>> > Eric Li >>> > 李咏竹 >>> > Google Kirkland >>> > >>> > >>> > >>> >> >> >> >> -- >> Eric Li >> 李咏竹 >> Google Kirkland >> >> >> > -- Eric Li 李咏竹 Google Kirkland
LGTM If build_autotest's functionality is entirely obsoleted by the "autotest" script and it has the same parameters, I'd suggest even just having it run that script so there aren't two places to fix bugs/add default tests, etc even in the short term. Once it makes run_remote_tests obsolete and we announce/doc the new workflow, we can remove that small shim too.
Ken, Please take another look. for build_autotest.sh. This should be what you want. Thanks On 2010/04/05 17:34:31, kmixter1 wrote: > LGTM > > If build_autotest's functionality is entirely obsoleted by the "autotest" script > and it has the same parameters, I'd suggest even just having it run that script > so there aren't two places to fix bugs/add default tests, etc even in the short > term. Once it makes run_remote_tests obsolete and we announce/doc the new > workflow, we can remove that small shim too.
LGTM with small comments http://codereview.chromium.org/1603004/diff/28001/29002 File src/scripts/build_autotest.sh (right): http://codereview.chromium.org/1603004/diff/28001/29002#newcode15 src/scripts/build_autotest.sh:15: ./autotest --build=all $@ just checking: does this still allow the user to pass --build=specific-test and have that override this default of building all tests. http://codereview.chromium.org/1603004/diff/28001/29003 File src/scripts/run_remote_tests.sh (right): http://codereview.chromium.org/1603004/diff/28001/29003#newcode157 src/scripts/run_remote_tests.sh:157: TMP_INSIDE_CHROOT=$(echo ${TMP:(-26)}) Please avoid using hardcoded strlen's. Try instead ${TMP#${FLAGS_chroot}}.
Pushing both 1603004 and 1545014 together. On 2010/04/07 19:11:06, kmixter1 wrote: > LGTM with small comments > > http://codereview.chromium.org/1603004/diff/28001/29002 > File src/scripts/build_autotest.sh (right): > > http://codereview.chromium.org/1603004/diff/28001/29002#newcode15 > src/scripts/build_autotest.sh:15: ./autotest --build=all $@ > just checking: does this still allow the user to pass --build=specific-test and > have that override this default of building all tests. YES. > > http://codereview.chromium.org/1603004/diff/28001/29003 > File src/scripts/run_remote_tests.sh (right): > > http://codereview.chromium.org/1603004/diff/28001/29003#newcode157 > src/scripts/run_remote_tests.sh:157: TMP_INSIDE_CHROOT=$(echo ${TMP:(-26)}) > Please avoid using hardcoded strlen's. Try instead > ${TMP#${FLAGS_chroot}}. FIXED. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
