Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10166023:
Add a --no-prompt flag to install quietly. (Closed)
|
Created:
8 years, 8 months ago by Peter Mayo Modified:
8 years, 8 months ago CC:
chromium-reviews Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdd a --no-prompt flag to install quietly.
R=cmp@chromium.org,nsylvain@chromium.org
BUG=124546
TEST=on a victim that didn't need to change, and on one that did.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133404
Patch Set 1 #
Total comments: 9
Patch Set 2 : #Patch Set 3 : Change variable name to make more english sense #Patch Set 4 : Change variable name to make more english sense #Patch Set 5 : Change option name to make it means less and 'do' more. #
Total comments: 1
Patch Set 6 : remove double defaulting #Patch Set 7 : new list generation can't be done quietly #Patch Set 8 : rebase #Messages
Total messages: 15 (0 generated)
lgtm
http://codereview.chromium.org/10166023/diff/1/build/install-build-deps.sh File build/install-build-deps.sh (right): http://codereview.chromium.org/10166023/diff/1/build/install-build-deps.sh#ne... build/install-build-deps.sh:15: echo "--[no-]lib32: enable or disable installation of 32 bit libraries" you should add the new flag to the usage here http://codereview.chromium.org/10166023/diff/1/build/install-build-deps.sh#ne... build/install-build-deps.sh:29: do_inst_lib32=0 can you remove these previous two lines? if you leave them, the behavior of whether --lib32 will be observed changes based on whether it comes before or after --bot. when you remove them, it means that this arg is no longer mainly about doing the right thing 'for a bot', so i would rename this '--no-prompt' http://codereview.chromium.org/10166023/diff/1/build/install-build-deps.sh#ne... build/install-build-deps.sh:191: new_list_cmd="sudo apt-get install --reinstall ${do_quiet-} $(echo $packages)" does --quiet imply -y? assuming no, i think you also want to signal here -y in the --no-prompt case. http://codereview.chromium.org/10166023/diff/1/build/install-build-deps.sh#ne... build/install-build-deps.sh:205: sudo apt-get install ${do_quiet-} ${new_list} does --quiet imply -y? assuming no, i think you also want to signal here -y in the --no-prompt case.
http://codereview.chromium.org/10166023/diff/1/build/install-build-deps.sh File build/install-build-deps.sh (right): http://codereview.chromium.org/10166023/diff/1/build/install-build-deps.sh#ne... build/install-build-deps.sh:29: do_inst_lib32=0 On 2012/04/21 19:39:24, cmp wrote: > can you remove these previous two lines? if you leave them, the behavior of > whether --lib32 will be observed changes based on whether it comes before or > after --bot. > > when you remove them, it means that this arg is no longer mainly about doing the > right thing 'for a bot', so i would rename this '--no-prompt' I had not considered passing both "use the right defaults" and "do this" ... seems incongruous. Prefer changing it to apply the correct things, except where the commandline says otherwise. http://codereview.chromium.org/10166023/diff/1/build/install-build-deps.sh#ne... build/install-build-deps.sh:191: new_list_cmd="sudo apt-get install --reinstall ${do_quiet-} $(echo $packages)" On 2012/04/21 19:39:24, cmp wrote: > does --quiet imply -y? assuming no, i think you also want to signal here -y in > the --no-prompt case. --quiet does not, but $do_quiet was supposed to - fixed there. -qq does, which was my intent. Changing http://codereview.chromium.org/10166023/diff/1/build/install-build-deps.sh#ne... build/install-build-deps.sh:205: sudo apt-get install ${do_quiet-} ${new_list} On 2012/04/21 19:39:24, cmp wrote: > does --quiet imply -y? assuming no, i think you also want to signal here -y in > the --no-prompt case. as above.
http://codereview.chromium.org/10166023/diff/1/build/install-build-deps.sh File build/install-build-deps.sh (right): http://codereview.chromium.org/10166023/diff/1/build/install-build-deps.sh#ne... build/install-build-deps.sh:15: echo "--[no-]lib32: enable or disable installation of 32 bit libraries" On 2012/04/21 19:39:24, cmp wrote: > you should add the new flag to the usage here Done.
http://codereview.chromium.org/10166023/diff/1/build/install-build-deps.sh File build/install-build-deps.sh (right): http://codereview.chromium.org/10166023/diff/1/build/install-build-deps.sh#ne... build/install-build-deps.sh:29: do_inst_lib32=0 On 2012/04/21 21:02:55, Peter Mayo wrote: > On 2012/04/21 19:39:24, cmp wrote: > > can you remove these previous two lines? if you leave them, the behavior of > > whether --lib32 will be observed changes based on whether it comes before or > > after --bot. > > > > when you remove them, it means that this arg is no longer mainly about doing > the > > right thing 'for a bot', so i would rename this '--no-prompt' > > > I had not considered passing both "use the right defaults" and "do this" ... > seems incongruous. > > Prefer changing it to apply the correct things, except where the commandline > says otherwise. Is there a "right defaults" wrt the lib32+syms flags? My understanding is that on some of the bots, we need one set of flags, and on others, we need a different set/the default. And on all of them, we need the "no prompt" feature. Hmm.
On 2012/04/21 21:17:46, cmp wrote: > Is there a "right defaults" wrt the lib32+syms flags? My understanding is that > on some of the bots, we need one set of flags, and on others, we need a > different set/the default. And on all of them, we need the "no prompt" feature. > Hmm. Btw, --bot means less to the dev user of install-build-deps.sh, which is a big user group of this script today. I assume they would benefit from the promptless feature just as well.
On 2012/04/21 21:17:46, cmp wrote: > Is there a "right defaults" wrt the lib32+syms flags? My understanding is that > on some of the bots, we need one set of flags, and on others, we need a > different set/the default. And on all of them, we need the "no prompt" feature. > Hmm. Btw, --bot means less to the dev user of install-build-deps.sh, which is a big user group of this script today. I assume they would benefit from the promptless feature just as well.
http://codereview.chromium.org/10166023/diff/2003/build/install-build-deps.sh File build/install-build-deps.sh (right): http://codereview.chromium.org/10166023/diff/2003/build/install-build-deps.sh... build/install-build-deps.sh:30: do_inst_lib32=${do_inst_lib32-0} Hi Peter, I still think lines 29-30 should be removed. :) I thought we agreed --no-prompt would mean less *and* do less (rather than mean less and do more). Like I mentioned over IM, setting the lib32/syms defaults here doesn't lessen the work someone maintaining the bots has to do, the work stays the same since they'll need to juggle where to add --lib32, etc. While leaving lines 29-30 has the unfortunate side effect of conflating the --no-prompt flag with a prescription that's not relevant to other users of --no-prompt. I noticed the title+description still mentions --bot. That should be updated to mirror the updated purpose. You're probably used to working in gerrit which auto-emails when a patch is updated. But rietveld doesn't do that, and so I didn't see in my email that you'd updated this patch. (I just happened to reload it to check.) Just a reminder to remember to press 'publish+mail' to ensure that I see your updates. Thanks.
On 2012/04/21 23:49:26, cmp wrote: > http://codereview.chromium.org/10166023/diff/2003/build/install-build-deps.sh > File build/install-build-deps.sh (right): > > http://codereview.chromium.org/10166023/diff/2003/build/install-build-deps.sh... > build/install-build-deps.sh:30: do_inst_lib32=${do_inst_lib32-0} > Hi Peter, > > I still think lines 29-30 should be removed. :) I thought we agreed --no-prompt > would mean less *and* do less (rather than mean less and do more). Yes. The do more was supposed to capture the more quietly. > > I noticed the title+description still mentions --bot. That should be updated to > mirror the updated purpose. Done - I had meant to do that before mailing. > > You're probably used to working in gerrit ... Actually actively bouncing back and forth at the moment. More distracted. Still haven't got back to testing this. > Thanks. You're welcome. Thanks for the patience.
Updated from test.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petermayo@chromium.org/10166023/14001
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petermayo@chromium.org/10166023/13004
Change committed as 133404 |
