Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6417001:
build_image: use string comparison for comparing strings (Closed)
Descriptionbuild_image: use string comparison for comparing strings
BUG=n0ne
TEST=./build_image and see it fail and ask me a question, instead of death
Change-Id: I27f7ac71ce74ff711e8b152ee202121d518045cd
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=23bc6ca
Patch Set 1 #
Messages
Total messages: 9 (0 generated)
Comparing $USER in a build tool is evil. Maybe we can expose a --sayyes flag that chrome-bot can use. On Mon, Feb 7, 2011 at 6:10 PM, <zbehan@chromium.org> wrote: > Reviewers: sosa, > > Description: > build_image: use string comparison for comparing strings > > BUG=n0ne > TEST=./build_image and see it fail and ask me a question, instead of death > > Change-Id: I27f7ac71ce74ff711e8b152ee202121d518045cd > > Please review this at http://codereview.chromium.org/6417001/ > > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master > > Affected files: > M build_image > > > Index: build_image > diff --git a/build_image b/build_image > index > 2dc7c7887bc20feeaadd4dc89f2f4e80a059cf66..95cdcb790118e6fdf9fef865cef03037432f98ea > 100755 > --- a/build_image > +++ b/build_image > @@ -358,7 +358,7 @@ delete_prompt() { > > # Only prompt if both stdin and stdout are a tty. If either is not a tty, > # then the user may not be present, so we shouldn't bother prompting. > - if tty -s && tty -s <&1 && [ "${USER}" -ne 'chrome-bot' ]; then > + if tty -s && tty -s <&1 && [ "${USER}" != 'chrome-bot' ]; then > read -p "Would you like to delete the output directory (y/N)? " SURE > SURE="${SURE:0:1}" # Get just the first character. > else > > >
Agreed, but I'm merely driveby fixing it. I'm pretty sure the statement is currently a lot more evil (because it doesn't work) so this CL is an improvement. I have no objections to seeking a better solution after this is landed, but any other solution is probably worth at least a short discussion about it. 2011/2/8 Anush Elangovan(அனுஷ்) <anush@chromium.org> > Comparing $USER in a build tool is evil. Maybe we can expose a --sayyes > flag that chrome-bot can use. > > On Mon, Feb 7, 2011 at 6:10 PM, <zbehan@chromium.org> wrote: > > Reviewers: sosa, > > > > Description: > > build_image: use string comparison for comparing strings > > > > BUG=n0ne > > TEST=./build_image and see it fail and ask me a question, instead of > death > > > > Change-Id: I27f7ac71ce74ff711e8b152ee202121d518045cd > > > > Please review this at http://codereview.chromium.org/6417001/ > > > > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master > > > > Affected files: > > M build_image > > > > > > Index: build_image > > diff --git a/build_image b/build_image > > index > > > 2dc7c7887bc20feeaadd4dc89f2f4e80a059cf66..95cdcb790118e6fdf9fef865cef03037432f98ea > > 100755 > > --- a/build_image > > +++ b/build_image > > @@ -358,7 +358,7 @@ delete_prompt() { > > > > # Only prompt if both stdin and stdout are a tty. If either is not a > tty, > > # then the user may not be present, so we shouldn't bother prompting. > > - if tty -s && tty -s <&1 && [ "${USER}" -ne 'chrome-bot' ]; then > > + if tty -s && tty -s <&1 && [ "${USER}" != 'chrome-bot' ]; then > > read -p "Would you like to delete the output directory (y/N)? " SURE > > SURE="${SURE:0:1}" # Get just the first character. > > else > > > > > > >
Please open a tracker for it and add a todo if committed as is. On Feb 7, 2011 6:59 PM, "Zdenek Behan" <zbehan@chromium.org> wrote: > Agreed, but I'm merely driveby fixing it. I'm pretty sure the statement is > currently a lot more evil (because it doesn't work) so this CL is an > improvement. I have no objections to seeking a better solution after this is > landed, but any other solution is probably worth at least a short discussion > about it. > > 2011/2/8 Anush Elangovan(அனுஷ்) <anush@chromium.org> > >> Comparing $USER in a build tool is evil. Maybe we can expose a --sayyes >> flag that chrome-bot can use. >> >> On Mon, Feb 7, 2011 at 6:10 PM, <zbehan@chromium.org> wrote: >> > Reviewers: sosa, >> > >> > Description: >> > build_image: use string comparison for comparing strings >> > >> > BUG=n0ne >> > TEST=./build_image and see it fail and ask me a question, instead of >> death >> > >> > Change-Id: I27f7ac71ce74ff711e8b152ee202121d518045cd >> > >> > Please review this at http://codereview.chromium.org/6417001/ >> > >> > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master >> > >> > Affected files: >> > M build_image >> > >> > >> > Index: build_image >> > diff --git a/build_image b/build_image >> > index >> > >> 2dc7c7887bc20feeaadd4dc89f2f4e80a059cf66..95cdcb790118e6fdf9fef865cef03037432f98ea >> > 100755 >> > --- a/build_image >> > +++ b/build_image >> > @@ -358,7 +358,7 @@ delete_prompt() { >> > >> > # Only prompt if both stdin and stdout are a tty. If either is not a >> tty, >> > # then the user may not be present, so we shouldn't bother prompting. >> > - if tty -s && tty -s <&1 && [ "${USER}" -ne 'chrome-bot' ]; then >> > + if tty -s && tty -s <&1 && [ "${USER}" != 'chrome-bot' ]; then >> > read -p "Would you like to delete the output directory (y/N)? " SURE >> > SURE="${SURE:0:1}" # Get just the first character. >> > else >> > >> > >> > >>
Will do. On Tue, Feb 8, 2011 at 4:04 AM, Anush Elangovan(அனுஷ்) <anush@chromium.org>wrote: > Please open a tracker for it and add a todo if committed as is. > On Feb 7, 2011 6:59 PM, "Zdenek Behan" <zbehan@chromium.org> wrote: > > Agreed, but I'm merely driveby fixing it. I'm pretty sure the statement > is > > currently a lot more evil (because it doesn't work) so this CL is an > > improvement. I have no objections to seeking a better solution after this > is > > landed, but any other solution is probably worth at least a short > discussion > > about it. > > > > 2011/2/8 Anush Elangovan(அனுஷ்) <anush@chromium.org> > > > >> Comparing $USER in a build tool is evil. Maybe we can expose a --sayyes > >> flag that chrome-bot can use. > >> > >> On Mon, Feb 7, 2011 at 6:10 PM, <zbehan@chromium.org> wrote: > >> > Reviewers: sosa, > >> > > >> > Description: > >> > build_image: use string comparison for comparing strings > >> > > >> > BUG=n0ne > >> > TEST=./build_image and see it fail and ask me a question, instead of > >> death > >> > > >> > Change-Id: I27f7ac71ce74ff711e8b152ee202121d518045cd > >> > > >> > Please review this at http://codereview.chromium.org/6417001/ > >> > > >> > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master > >> > > >> > Affected files: > >> > M build_image > >> > > >> > > >> > Index: build_image > >> > diff --git a/build_image b/build_image > >> > index > >> > > >> > 2dc7c7887bc20feeaadd4dc89f2f4e80a059cf66..95cdcb790118e6fdf9fef865cef03037432f98ea > >> > 100755 > >> > --- a/build_image > >> > +++ b/build_image > >> > @@ -358,7 +358,7 @@ delete_prompt() { > >> > > >> > # Only prompt if both stdin and stdout are a tty. If either is not a > >> tty, > >> > # then the user may not be present, so we shouldn't bother prompting. > >> > - if tty -s && tty -s <&1 && [ "${USER}" -ne 'chrome-bot' ]; then > >> > + if tty -s && tty -s <&1 && [ "${USER}" != 'chrome-bot' ]; then > >> > read -p "Would you like to delete the output directory (y/N)? " SURE > >> > SURE="${SURE:0:1}" # Get just the first character. > >> > else > >> > > >> > > >> > > >> >
LGTM On Mon, Feb 7, 2011 at 7:09 PM, Zdenek Behan <zbehan@chromium.org> wrote: > Will do. > > On Tue, Feb 8, 2011 at 4:04 AM, Anush Elangovan(அனுஷ்) <anush@chromium.org> > wrote: >> >> Please open a tracker for it and add a todo if committed as is. >> >> On Feb 7, 2011 6:59 PM, "Zdenek Behan" <zbehan@chromium.org> wrote: >> > Agreed, but I'm merely driveby fixing it. I'm pretty sure the statement >> > is >> > currently a lot more evil (because it doesn't work) so this CL is an >> > improvement. I have no objections to seeking a better solution after >> > this is >> > landed, but any other solution is probably worth at least a short >> > discussion >> > about it. >> > >> > 2011/2/8 Anush Elangovan(அனுஷ்) <anush@chromium.org> >> > >> >> Comparing $USER in a build tool is evil. Maybe we can expose a --sayyes >> >> flag that chrome-bot can use. >> >> >> >> On Mon, Feb 7, 2011 at 6:10 PM, <zbehan@chromium.org> wrote: >> >> > Reviewers: sosa, >> >> > >> >> > Description: >> >> > build_image: use string comparison for comparing strings >> >> > >> >> > BUG=n0ne >> >> > TEST=./build_image and see it fail and ask me a question, instead of >> >> death >> >> > >> >> > Change-Id: I27f7ac71ce74ff711e8b152ee202121d518045cd >> >> > >> >> > Please review this at http://codereview.chromium.org/6417001/ >> >> > >> >> > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master >> >> > >> >> > Affected files: >> >> > M build_image >> >> > >> >> > >> >> > Index: build_image >> >> > diff --git a/build_image b/build_image >> >> > index >> >> > >> >> >> >> 2dc7c7887bc20feeaadd4dc89f2f4e80a059cf66..95cdcb790118e6fdf9fef865cef03037432f98ea >> >> > 100755 >> >> > --- a/build_image >> >> > +++ b/build_image >> >> > @@ -358,7 +358,7 @@ delete_prompt() { >> >> > >> >> > # Only prompt if both stdin and stdout are a tty. If either is not a >> >> tty, >> >> > # then the user may not be present, so we shouldn't bother prompting. >> >> > - if tty -s && tty -s <&1 && [ "${USER}" -ne 'chrome-bot' ]; then >> >> > + if tty -s && tty -s <&1 && [ "${USER}" != 'chrome-bot' ]; then >> >> > read -p "Would you like to delete the output directory (y/N)? " SURE >> >> > SURE="${SURE:0:1}" # Get just the first character. >> >> > else >> >> > >> >> > >> >> > >> >> > >
Just FYI:
build_image: if tty -s && tty -s <&1 && [ "${USER}" != 'chrome-bot' ]; then
common.sh: if [ "$USER" != "chrome-bot" ]; then
customize_rootfs:elif [ "$USER" = "chrome-bot" ]; then
make_chroot: if [[ "$USER" = "chrome-bot" ]]; then
Evil or not, it seems to be heavilly used.
On Tue, Feb 8, 2011 at 4:04 AM, Anush Elangovan(அனுஷ்)
<anush@chromium.org>wrote:
> Please open a tracker for it and add a todo if committed as is.
> On Feb 7, 2011 6:59 PM, "Zdenek Behan" <zbehan@chromium.org> wrote:
> > Agreed, but I'm merely driveby fixing it. I'm pretty sure the statement
> is
> > currently a lot more evil (because it doesn't work) so this CL is an
> > improvement. I have no objections to seeking a better solution after this
> is
> > landed, but any other solution is probably worth at least a short
> discussion
> > about it.
> >
> > 2011/2/8 Anush Elangovan(அனுஷ்) <anush@chromium.org>
> >
> >> Comparing $USER in a build tool is evil. Maybe we can expose a --sayyes
> >> flag that chrome-bot can use.
> >>
> >> On Mon, Feb 7, 2011 at 6:10 PM, <zbehan@chromium.org> wrote:
> >> > Reviewers: sosa,
> >> >
> >> > Description:
> >> > build_image: use string comparison for comparing strings
> >> >
> >> > BUG=n0ne
> >> > TEST=./build_image and see it fail and ask me a question, instead of
> >> death
> >> >
> >> > Change-Id: I27f7ac71ce74ff711e8b152ee202121d518045cd
> >> >
> >> > Please review this at http://codereview.chromium.org/6417001/
> >> >
> >> > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master
> >> >
> >> > Affected files:
> >> > M build_image
> >> >
> >> >
> >> > Index: build_image
> >> > diff --git a/build_image b/build_image
> >> > index
> >> >
> >>
>
2dc7c7887bc20feeaadd4dc89f2f4e80a059cf66..95cdcb790118e6fdf9fef865cef03037432f98ea
> >> > 100755
> >> > --- a/build_image
> >> > +++ b/build_image
> >> > @@ -358,7 +358,7 @@ delete_prompt() {
> >> >
> >> > # Only prompt if both stdin and stdout are a tty. If either is not a
> >> tty,
> >> > # then the user may not be present, so we shouldn't bother prompting.
> >> > - if tty -s && tty -s <&1 && [ "${USER}" -ne 'chrome-bot' ]; then
> >> > + if tty -s && tty -s <&1 && [ "${USER}" != 'chrome-bot' ]; then
> >> > read -p "Would you like to delete the output directory (y/N)? " SURE
> >> > SURE="${SURE:0:1}" # Get just the first character.
> >> > else
> >> >
> >> >
> >> >
> >>
>
It'll probably be easier to fix once we have one buildbot script rather than having all these extra flags just for buildbots that have to be maintained on each buildbot workflow On Tue, Feb 8, 2011 at 12:50 PM, Zdenek Behan <zbehan@chromium.org> wrote: > Just FYI: > build_image: if tty -s && tty -s <&1 && [ "${USER}" != 'chrome-bot' ]; then > common.sh: if [ "$USER" != "chrome-bot" ]; then > customize_rootfs:elif [ "$USER" = "chrome-bot" ]; then > make_chroot: if [[ "$USER" = "chrome-bot" ]]; then > Evil or not, it seems to be heavilly used. > On Tue, Feb 8, 2011 at 4:04 AM, Anush Elangovan(அனுஷ்) <anush@chromium.org> > wrote: >> >> Please open a tracker for it and add a todo if committed as is. >> >> On Feb 7, 2011 6:59 PM, "Zdenek Behan" <zbehan@chromium.org> wrote: >> > Agreed, but I'm merely driveby fixing it. I'm pretty sure the statement >> > is >> > currently a lot more evil (because it doesn't work) so this CL is an >> > improvement. I have no objections to seeking a better solution after >> > this is >> > landed, but any other solution is probably worth at least a short >> > discussion >> > about it. >> > >> > 2011/2/8 Anush Elangovan(அனுஷ்) <anush@chromium.org> >> > >> >> Comparing $USER in a build tool is evil. Maybe we can expose a --sayyes >> >> flag that chrome-bot can use. >> >> >> >> On Mon, Feb 7, 2011 at 6:10 PM, <zbehan@chromium.org> wrote: >> >> > Reviewers: sosa, >> >> > >> >> > Description: >> >> > build_image: use string comparison for comparing strings >> >> > >> >> > BUG=n0ne >> >> > TEST=./build_image and see it fail and ask me a question, instead of >> >> death >> >> > >> >> > Change-Id: I27f7ac71ce74ff711e8b152ee202121d518045cd >> >> > >> >> > Please review this at http://codereview.chromium.org/6417001/ >> >> > >> >> > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master >> >> > >> >> > Affected files: >> >> > M build_image >> >> > >> >> > >> >> > Index: build_image >> >> > diff --git a/build_image b/build_image >> >> > index >> >> > >> >> >> >> 2dc7c7887bc20feeaadd4dc89f2f4e80a059cf66..95cdcb790118e6fdf9fef865cef03037432f98ea >> >> > 100755 >> >> > --- a/build_image >> >> > +++ b/build_image >> >> > @@ -358,7 +358,7 @@ delete_prompt() { >> >> > >> >> > # Only prompt if both stdin and stdout are a tty. If either is not a >> >> tty, >> >> > # then the user may not be present, so we shouldn't bother prompting. >> >> > - if tty -s && tty -s <&1 && [ "${USER}" -ne 'chrome-bot' ]; then >> >> > + if tty -s && tty -s <&1 && [ "${USER}" != 'chrome-bot' ]; then >> >> > read -p "Would you like to delete the output directory (y/N)? " SURE >> >> > SURE="${SURE:0:1}" # Get just the first character. >> >> > else >> >> > >> >> > >> >> > >> >> > >
I tend to agree. I can open a P2 issue just to keep it documented, but I think this is a really small fish with a lot of work to catch it. On Tue, Feb 8, 2011 at 9:52 PM, Chris Sosa <sosa@chromium.org> wrote: > It'll probably be easier to fix once we have one buildbot script > rather than having all these extra flags just for buildbots that have > to be maintained on each buildbot workflow > > On Tue, Feb 8, 2011 at 12:50 PM, Zdenek Behan <zbehan@chromium.org> wrote: > > Just FYI: > > build_image: if tty -s && tty -s <&1 && [ "${USER}" != 'chrome-bot' ]; > then > > common.sh: if [ "$USER" != "chrome-bot" ]; then > > customize_rootfs:elif [ "$USER" = "chrome-bot" ]; then > > make_chroot: if [[ "$USER" = "chrome-bot" ]]; then > > Evil or not, it seems to be heavilly used. > > On Tue, Feb 8, 2011 at 4:04 AM, Anush Elangovan(அனுஷ்) < > anush@chromium.org> > > wrote: > >> > >> Please open a tracker for it and add a todo if committed as is. > >> > >> On Feb 7, 2011 6:59 PM, "Zdenek Behan" <zbehan@chromium.org> wrote: > >> > Agreed, but I'm merely driveby fixing it. I'm pretty sure the > statement > >> > is > >> > currently a lot more evil (because it doesn't work) so this CL is an > >> > improvement. I have no objections to seeking a better solution after > >> > this is > >> > landed, but any other solution is probably worth at least a short > >> > discussion > >> > about it. > >> > > >> > 2011/2/8 Anush Elangovan(அனுஷ்) <anush@chromium.org> > >> > > >> >> Comparing $USER in a build tool is evil. Maybe we can expose a > --sayyes > >> >> flag that chrome-bot can use. > >> >> > >> >> On Mon, Feb 7, 2011 at 6:10 PM, <zbehan@chromium.org> wrote: > >> >> > Reviewers: sosa, > >> >> > > >> >> > Description: > >> >> > build_image: use string comparison for comparing strings > >> >> > > >> >> > BUG=n0ne > >> >> > TEST=./build_image and see it fail and ask me a question, instead > of > >> >> death > >> >> > > >> >> > Change-Id: I27f7ac71ce74ff711e8b152ee202121d518045cd > >> >> > > >> >> > Please review this at http://codereview.chromium.org/6417001/ > >> >> > > >> >> > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master > >> >> > > >> >> > Affected files: > >> >> > M build_image > >> >> > > >> >> > > >> >> > Index: build_image > >> >> > diff --git a/build_image b/build_image > >> >> > index > >> >> > > >> >> > >> >> > 2dc7c7887bc20feeaadd4dc89f2f4e80a059cf66..95cdcb790118e6fdf9fef865cef03037432f98ea > >> >> > 100755 > >> >> > --- a/build_image > >> >> > +++ b/build_image > >> >> > @@ -358,7 +358,7 @@ delete_prompt() { > >> >> > > >> >> > # Only prompt if both stdin and stdout are a tty. If either is not > a > >> >> tty, > >> >> > # then the user may not be present, so we shouldn't bother > prompting. > >> >> > - if tty -s && tty -s <&1 && [ "${USER}" -ne 'chrome-bot' ]; then > >> >> > + if tty -s && tty -s <&1 && [ "${USER}" != 'chrome-bot' ]; then > >> >> > read -p "Would you like to delete the output directory (y/N)? " > SURE > >> >> > SURE="${SURE:0:1}" # Get just the first character. > >> >> > else > >> >> > > >> >> > > >> >> > > >> >> > > > > > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
