Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3148016:
Make sync_build_test and build_autotest support workon. (Closed)
DescriptionMake sync_build_test and build_autotest support workon.
BUG=5766
Patch Set 1 #Patch Set 2 : Autodetect workon #
Total comments: 9
Patch Set 3 : Respond to review #Messages
Total messages: 9 (0 generated)
On 2010/08/17 22:02:01, kmixter1 wrote: > Cool! So now I know where do you build all the tests. Is this (build_autotest.sh) the authoritative place used for building tests in things like factory, hwqual testing? While currently the way is to emerge autotest-tests, this might change to something slightly more sophisticated as soon as we have more than one tests ebuild, and I'll take care of changing it here. Also, LGTM to the build_autotest.sh, the other script is way beyond my scope. I slightly wonder why does the original workflow variant always build for default board and not for --board, but your change is merely preserving the original behaviour, so this question should be discussed and possibly fixed in a different CL.
Alas no, though I thought it was. It used to be called by build_packages, but it appears build_packages calls autotest --build=all itself. I think I'll switch build_packages to use build_autotest.sh again. I believe build_packages --withautotest is how factory tests are built, but it's definitely the way that hwqual and automated tests end up getting the compiled autotests. On Tue, Aug 17, 2010 at 4:11 PM, <zbehan@chromium.org> wrote: > On 2010/08/17 22:02:01, kmixter1 wrote: > > > Cool! So now I know where do you build all the tests. Is this > (build_autotest.sh) the authoritative place used for building tests in > things > like factory, hwqual testing? > > While currently the way is to emerge autotest-tests, this might change to > something slightly more sophisticated as soon as we have more than one tests > ebuild, and I'll take care of changing it here. > > Also, LGTM to the build_autotest.sh, the other script is way beyond my > scope. > I slightly wonder why does the original workflow variant always build for > default board and not for --board, but your change is merely preserving the > original behaviour, so this question should be discussed and possibly fixed > in a > different CL. > > http://codereview.chromium.org/3148016/show >
I see. Well, I'd expect there to be one place that everybody calls. Otherwise we'll just keep forgetting to edit both (all N) places at the same time. So thanks for fixing it, if you're about to do that! On Tue, Aug 17, 2010 at 5:34 PM, Ken Mixter <kmixter@chromium.org> wrote: > Alas no, though I thought it was. It used to be called by > build_packages, but it appears build_packages calls autotest > --build=all itself. I think I'll switch build_packages to use > build_autotest.sh again. I believe build_packages --withautotest is > how factory tests are built, but it's definitely the way that hwqual > and automated tests end up getting the compiled autotests. > > On Tue, Aug 17, 2010 at 4:11 PM, <zbehan@chromium.org> wrote: > > On 2010/08/17 22:02:01, kmixter1 wrote: > > > > > > Cool! So now I know where do you build all the tests. Is this > > (build_autotest.sh) the authoritative place used for building tests in > > things > > like factory, hwqual testing? > > > > While currently the way is to emerge autotest-tests, this might change to > > something slightly more sophisticated as soon as we have more than one > tests > > ebuild, and I'll take care of changing it here. > > > > Also, LGTM to the build_autotest.sh, the other script is way beyond my > > scope. > > I slightly wonder why does the original workflow variant always build for > > default board and not for --board, but your change is merely preserving > the > > original behaviour, so this question should be discussed and possibly > fixed > > in a > > different CL. > > > > http://codereview.chromium.org/3148016/show > > >
I am. The related review is http://codereview.chromium.org/3180011 which calls build_autotest directly and appropriately builds the autotests based on work flow.
http://codereview.chromium.org/3148016/diff/5001/6002 File sync_build_test.sh (right): http://codereview.chromium.org/3148016/diff/5001/6002#newcode123 sync_build_test.sh:123: if [[ -d "${FLAGS_top}" ]]; then Why do you want to auto-detect this? http://codereview.chromium.org/3148016/diff/5001/6002#newcode411 sync_build_test.sh:411: # Configures/initializes a new checkout period at end http://codereview.chromium.org/3148016/diff/5001/6002#newcode430 sync_build_test.sh:430: git cl config "file://$(pwd)/../../../codereview.settings" Why do you need git cl config here and below? Are you uploading/committing changes? http://codereview.chromium.org/3148016/diff/5001/6002#newcode549 sync_build_test.sh:549: [[ ${FLAGS_useworkon} ]] && extra_flags="--useworkon" The default is changing soon so you should explicitly use either --useworkon or --nouseworkon
PTAL. http://codereview.chromium.org/3148016/diff/5001/6002 File sync_build_test.sh (right): http://codereview.chromium.org/3148016/diff/5001/6002#newcode123 sync_build_test.sh:123: if [[ -d "${FLAGS_top}" ]]; then On 2010/08/18 18:44:18, sosa wrote: > Why do you want to auto-detect this? Just makes it be one less parameter to pass to sync_build_test, when it can be auto-detected. There's never a point to switching the workflow for a given checkout, right? So I'd either want to give an error if it mismatched, or just do the right thing. This is the latter. The parameter still is useful to expose though because sync_build_test can also check out/build/test brand new checkouts. Is that what you were asking? http://codereview.chromium.org/3148016/diff/5001/6002#newcode430 sync_build_test.sh:430: git cl config "file://$(pwd)/../../../codereview.settings" On 2010/08/18 18:44:18, sosa wrote: > Why do you need git cl config here and below? Are you uploading/committing > changes? Is it no longer necessary to run git cl config once before doing a git cl upload in a checkout? And yes, I would be - this script is generally part of my development workflow. I believe others' as well. http://codereview.chromium.org/3148016/diff/5001/6002#newcode549 sync_build_test.sh:549: [[ ${FLAGS_useworkon} ]] && extra_flags="--useworkon" On 2010/08/18 18:44:18, sosa wrote: > The default is changing soon so you should explicitly use either --useworkon or > --nouseworkon Done.
http://codereview.chromium.org/3148016/diff/5001/6002 File sync_build_test.sh (right): http://codereview.chromium.org/3148016/diff/5001/6002#newcode123 sync_build_test.sh:123: if [[ -d "${FLAGS_top}" ]]; then Oh I would think you would want to either detect this automatically or pass in a flag. It seems strange to pass in a flag and also automatically modify prev-mentioned flag. If you automatically detect this and don't want to worry about it, why not just remove it from being a flag? In the case you do remove the flag, you should also probably just set the variable to true if you're missing the dev directory (since you would always have it in the old workflow) On 2010/08/18 18:55:29, kmixter1 wrote: > On 2010/08/18 18:44:18, sosa wrote: > > Why do you want to auto-detect this? > > Just makes it be one less parameter to pass to sync_build_test, when it can be > auto-detected. There's never a point to switching the workflow for a given > checkout, right? So I'd either want to give an error if it mismatched, or just > do the right thing. This is the latter. The parameter still is useful to > expose though because sync_build_test can also check out/build/test brand new > checkouts. Is that what you were asking? http://codereview.chromium.org/3148016/diff/5001/6002#newcode430 sync_build_test.sh:430: git cl config "file://$(pwd)/../../../codereview.settings" It is necessary, I just didn't realize you were using this as your dev workflow. On 2010/08/18 18:55:29, kmixter1 wrote: > On 2010/08/18 18:44:18, sosa wrote: > > Why do you need git cl config here and below? Are you uploading/committing > > changes? > > Is it no longer necessary to run git cl config once before doing a git cl upload > in a checkout? And yes, I would be - this script is generally part of my > development workflow. I believe others' as well.
After convo LGTM On Wed, Aug 18, 2010 at 12:26 PM, <sosa@chromium.org> wrote: > > http://codereview.chromium.org/3148016/diff/5001/6002 > File sync_build_test.sh (right): > > http://codereview.chromium.org/3148016/diff/5001/6002#newcode123 > sync_build_test.sh:123: if [[ -d "${FLAGS_top}" ]]; then > Oh I would think you would want to either detect this automatically or > pass in a flag. It seems strange to pass in a flag and also > automatically modify prev-mentioned flag. If you automatically detect > this and don't want to worry about it, why not just remove it from being > a flag? > > In the case you do remove the flag, you should also probably just set > the variable to true if you're missing the dev directory (since you > would always have it in the old workflow) > > On 2010/08/18 18:55:29, kmixter1 wrote: >> >> On 2010/08/18 18:44:18, sosa wrote: >> > Why do you want to auto-detect this? > >> Just makes it be one less parameter to pass to sync_build_test, when > > it can be >> >> auto-detected. There's never a point to switching the workflow for a > > given >> >> checkout, right? So I'd either want to give an error if it > > mismatched, or just >> >> do the right thing. This is the latter. The parameter still is > > useful to >> >> expose though because sync_build_test can also check out/build/test > > brand new >> >> checkouts. Is that what you were asking? > > http://codereview.chromium.org/3148016/diff/5001/6002#newcode430 > sync_build_test.sh:430: git cl config > "file://$(pwd)/../../../codereview.settings" > It is necessary, I just didn't realize you were using this as your dev > workflow. > > On 2010/08/18 18:55:29, kmixter1 wrote: >> >> On 2010/08/18 18:44:18, sosa wrote: >> > Why do you need git cl config here and below? Are you > > uploading/committing >> >> > changes? > >> Is it no longer necessary to run git cl config once before doing a git > > cl upload >> >> in a checkout? And yes, I would be - this script is generally part of > > my >> >> development workflow. I believe others' as well. > > http://codereview.chromium.org/3148016/show > |
