Standardize invocations of emerge in build_image

Standardize invocations of emerge in build_image.

Also includes a style nit fix:  don't quote arguments to numeric
comparison test operators.

BUG=chromium-os:13582
TEST=setup_board --force && build_packages && build_image; boot the result

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=eb5246f

[jrbarnette, 2011-04-07 02:23:09]

(added more reviewers)

I only need one reviewer to step up.  Anyone who's worried I might
break their world, put a hold in early, and you can have time to
inspect this.

[davidjames, 2011-04-07 06:32:18]

LGTM w/nits

http://codereview.chromium.org/6801027/diff/1002/build_image
File build_image (right):

http://codereview.chromium.org/6801027/diff/1002/build_image#newcode211
build_image:211: emerge-${FLAGS_board} -uNDvg --binpkg-respect-use=y
virtual/kernel
--binpkg-respect-use=y is implied by -N so we can get rid of that.

-uDNvg would be more consistent with the rest of the file than -uNDvg

http://codereview.chromium.org/6801027/diff/1002/build_image#newcode636
build_image:636: emerge_to_image --root="${ROOT_FS_DIR}" chromeos
${EXTRA_PACKAGES}
Could we add -uDNv here? That way, we won't ever install packages that don't
match the user's use flags -- they'll fail right away even if they're in
--nowithdev mode.

[davidjames, 2011-04-07 06:41:18]

http://codereview.chromium.org/6801027/diff/1002/build_image
File build_image (right):

http://codereview.chromium.org/6801027/diff/1002/build_image#newcode636
build_image:636: emerge_to_image --root="${ROOT_FS_DIR}" chromeos
${EXTRA_PACKAGES}
On 2011/04/07 06:32:18, davidjames wrote:
> Could we add -uDNv here? That way, we won't ever install packages that don't
> match the user's use flags -- they'll fail right away even if they're in
> --nowithdev mode.

On second thought, let's not do that  -- it'll probably sadly break the world as
I bet there are people and scripts that are still depending on not having to set
USE flags for build_image...

See the --nowithdebug flag for build_packages for an example. It sets a USE flag
in build_packages but that use flag will be nowhere to be found by the time
build_image comes around. Surprised that it works but I think it does...

[kliegs, 2011-04-07 14:34:24]

I'd also like to see this tested by running through the buidlbot scripts
(internal and external) just to make sure nothing along the path is missed.  It
looks like everything should work fine but I get a bit paranoid on build
infrastructure changes like this.

http://codereview.chromium.org/6801027/diff/1002/build_image
File build_image (right):

http://codereview.chromium.org/6801027/diff/1002/build_image#newcode148
build_image:148: export INSTALL_MASK=""
Is this export needed?  I thought export wasn't needed for variables declared at
file scope in scripts and prevents the risk of environment pollution if the
script is accidentally called wrong.

http://codereview.chromium.org/6801027/diff/1002/build_image#newcode198
build_image:198: export USE="${USE} initramfs"
Same as above - is export actually needed here?

http://codereview.chromium.org/6801027/diff/1002/build_image#newcode636
build_image:636: emerge_to_image --root="${ROOT_FS_DIR}" chromeos
${EXTRA_PACKAGES}
On 2011/04/07 06:41:18, davidjames wrote:
> On 2011/04/07 06:32:18, davidjames wrote:
> > Could we add -uDNv here? That way, we won't ever install packages that don't
> > match the user's use flags -- they'll fail right away even if they're in
> > --nowithdev mode.
> 
> On second thought, let's not do that  -- it'll probably sadly break the world
as
> I bet there are people and scripts that are still depending on not having to
set
> USE flags for build_image...
> 
> See the --nowithdebug flag for build_packages for an example. It sets a USE
flag
> in build_packages but that use flag will be nowhere to be found by the time
> build_image comes around. Surprised that it works but I think it does...

Agreed - I know up until this week I never passed extra USE flags to build_image
but did to build_packages so this would break that case.  Might be worth filing
a bug on for future consideration but should go in a different CL.

[jrbarnette, 2011-04-07 19:06:35]

On 2011/04/07 14:34:24, kliegs wrote:
> I'd also like to see this tested by running through the buidlbot scripts
> (internal and external) just to make sure nothing along the path is missed. 
It
> looks like everything should work fine but I get a bit paranoid on build
> infrastructure changes like this.
> 
I'll check in to how hard this is to do.  My sense is that currently,
some things can't be effectively tested except by 'git cl push'.  :-(
Note that I'm not endorsing that as a solution, I'm just summarizing
what I've seen of current reality.


> http://codereview.chromium.org/6801027/diff/1002/build_image
> File build_image (right):
> 
> http://codereview.chromium.org/6801027/diff/1002/build_image#newcode148
> build_image:148: export INSTALL_MASK=""
> Is this export needed?  I thought export wasn't needed for variables declared
at
> file scope in scripts and prevents the risk of environment pollution if the
> script is accidentally called wrong.
> 
Yes, it's required.

Variable assignments always have file scope, even inside a function,
unless the variable is declared with "local".  Basically, it works
just like dynamic scoping in Lisp.

Variables with file scope are only exported into the environment
if named in an 'export' statement, or if they were already in the
environment at shell start up.

> http://codereview.chromium.org/6801027/diff/1002/build_image#newcode198
> build_image:198: export USE="${USE} initramfs"
> Same as above - is export actually needed here?
> 
Same answer.  I note that there's no guarantee that USE was
inherited, so the export is needed to be sure.

[jrbarnette, 2011-04-07 19:13:45]

http://codereview.chromium.org/6801027/diff/1002/build_image
File build_image (right):

http://codereview.chromium.org/6801027/diff/1002/build_image#newcode211
build_image:211: emerge-${FLAGS_board} -uNDvg --binpkg-respect-use=y
virtual/kernel
On 2011/04/07 06:32:18, davidjames wrote:
> --binpkg-respect-use=y is implied by -N so we can get rid of that.
> 
> -uDNvg would be more consistent with the rest of the file than -uNDvg

I'll update the options.

Should the command use ${EMERGE_BOARD_CMD} instead of
emerge-${FLAGS_board}?

[kliegs, 2011-04-07 19:14:23]

Ahh right. Sorry - I'd assumed both were coming in but USE can be null and
set to initramfs.

For testing you can run cbuidlbot directly to test the buildbot flow without
needing to get actually push it.

I think just running 'chromite/buildbot/cbuildbot --noarchive --nouprev
x86-generic-full' although that would clobber your chroot - might be better
to experiment in a clean setting.

Something worth setting overnight and seeing what happens in the morning
most likely unless you're in a rush to get this CL in.



On Thu, Apr 7, 2011 at 3:06 PM, <jrbarnette@chromium.org> wrote:

> On 2011/04/07 14:34:24, kliegs wrote:
>
>> I'd also like to see this tested by running through the buidlbot scripts
>> (internal and external) just to make sure nothing along the path is
>> missed.
>>
> It
>
>> looks like everything should work fine but I get a bit paranoid on build
>> infrastructure changes like this.
>>
>
> I'll check in to how hard this is to do.  My sense is that currently,
> some things can't be effectively tested except by 'git cl push'.  :-(
> Note that I'm not endorsing that as a solution, I'm just summarizing
> what I've seen of current reality.
>
>
>
>  http://codereview.chromium.org/6801027/diff/1002/build_image
>> File build_image (right):
>>
>
>  http://codereview.chromium.org/6801027/diff/1002/build_image#newcode148
>> build_image:148: export INSTALL_MASK=""
>> Is this export needed?  I thought export wasn't needed for variables
>> declared
>>
> at
>
>> file scope in scripts and prevents the risk of environment pollution if
>> the
>> script is accidentally called wrong.
>>
>
> Yes, it's required.
>
> Variable assignments always have file scope, even inside a function,
> unless the variable is declared with "local".  Basically, it works
> just like dynamic scoping in Lisp.
>
> Variables with file scope are only exported into the environment
> if named in an 'export' statement, or if they were already in the
> environment at shell start up.
>
>
>  http://codereview.chromium.org/6801027/diff/1002/build_image#newcode198
>> build_image:198: export USE="${USE} initramfs"
>> Same as above - is export actually needed here?
>>
>
> Same answer.  I note that there's no guarantee that USE was
> inherited, so the export is needed to be sure.
>
>
>
> http://codereview.chromium.org/6801027/
>

[davidjames, 2011-04-07 19:26:16]

http://codereview.chromium.org/6801027/diff/1002/build_image
File build_image (right):

http://codereview.chromium.org/6801027/diff/1002/build_image#newcode211
build_image:211: emerge-${FLAGS_board} -uNDvg --binpkg-respect-use=y
virtual/kernel
On 2011/04/07 19:13:45, jrbarnette wrote:
> On 2011/04/07 06:32:18, davidjames wrote:
> > --binpkg-respect-use=y is implied by -N so we can get rid of that.
> > 
> > -uDNvg would be more consistent with the rest of the file than -uNDvg
> 
> I'll update the options.
> 
> Should the command use ${EMERGE_BOARD_CMD} instead of
> emerge-${FLAGS_board}?

Yes -- parallel_emerge used to have a bug where it didn't support -N correctly,
but that's fixed now, so we can use ${EMERGE_BOARD_CMD} again.

[zbehan, 2011-04-07 19:49:34]

The only thing I'd be really careful about and has been mentioned here is
that the USE flags of built packages may be inconsitent and added by various
ifdefs, even outside of this script. Only case I'm certain of is the factory
kernel, which should be ok with this CL.

I feel the issue in this CL is that we don't really have all the possible
use-cases defined/summarized anywhere, therefore no amount of testing can be
absolutely sure it doesn't break anything, and that cannot really be
addressed, so you have to go with some sort of best-effort test and keep an
eye out for angry emails.

Also can you please change the description to correctly reflect both things
that the CL changes (quotes and emerge invocations)? The reference to
targetting USB seems a little weird, all of these changes are fairly generic
invocations unrelated to USB images.

On Thu, Apr 7, 2011 at 9:26 PM, <davidjames@chromium.org> wrote:

>
> http://codereview.chromium.org/6801027/diff/1002/build_image
> File build_image (right):
>
> http://codereview.chromium.org/6801027/diff/1002/build_image#newcode211
> build_image:211: emerge-${FLAGS_board} -uNDvg --binpkg-respect-use=y
> virtual/kernel
> On 2011/04/07 19:13:45, jrbarnette wrote:
>
>> On 2011/04/07 06:32:18, davidjames wrote:
>> > --binpkg-respect-use=y is implied by -N so we can get rid of that.
>> >
>> > -uDNvg would be more consistent with the rest of the file than
>>
> -uNDvg
>
>  I'll update the options.
>>
>
>  Should the command use ${EMERGE_BOARD_CMD} instead of
>> emerge-${FLAGS_board}?
>>
>
> Yes -- parallel_emerge used to have a bug where it didn't support -N
> correctly, but that's fixed now, so we can use ${EMERGE_BOARD_CMD}
> again.
>
>
> http://codereview.chromium.org/6801027/
>

[jrbarnette, 2011-04-07 22:29:52]

I've updated the emerge command davidjames noted.

I've asked sosa about the level of testing he thinks could
realistically find a problem before 'git cl push'.  Here's
the upshot:
  * For this change, the only difference is that certain
    emerge commands will inherit a variety of environment
    variable settings that previously they couldn't inherit.
  * For this to cause a failure, there would have to be build
    code that sets an environment variable known to our build
    system.
Chris is of the opinion that the cbuildbot merely passes on
whatever environment it inherits.  We already know my
environment won't break the new build_image ('cause I
tested it), so using cbuildbot won't uncover any new failure.
It's possible that the buildbots have settings we don't know
about, but neither Chris nor I have ready access to that
environment for testing.

More testing will likely delay this change by at least a
day, and the delay will generally complicate my life.  If
Chris and I have analyzed it right, then that effort (and
risk) don't buy us anything.

So, have we missed a trick?  Is there any other test that's
really worth the effort?  I believe this code is ready except
for any missing tests.

[kliegs, 2011-04-07 22:42:08]

If you've gone through it with sosa I'm not going to push for this to be
delayed longer.  I still personally feel it would be better to run it
through once to be safe as most of the time running a final overnight test
isn't that intrusive.  Plus I personally prefer pushing in the morning when
I know I'll be around if something breaks than at night when I'm looking to
leave.

My main concern on this was its introducing a new helper function that
everyone may assume works perfectly once the CL is in.  A little extra
testing now would prevent a lot of later debugging if something is wrong.

Anyways, sosa is more familiar with the build infrastructure and what
specifically would be gained by testing with cbuildbot so I'll withdraw my
request for more testing.

LGTM

On Thu, Apr 7, 2011 at 6:29 PM, <jrbarnette@chromium.org> wrote:

> I've updated the emerge command davidjames noted.
>
> I've asked sosa about the level of testing he thinks could
> realistically find a problem before 'git cl push'.  Here's
> the upshot:
>  * For this change, the only difference is that certain
>    emerge commands will inherit a variety of environment
>    variable settings that previously they couldn't inherit.
>  * For this to cause a failure, there would have to be build
>    code that sets an environment variable known to our build
>    system.
> Chris is of the opinion that the cbuildbot merely passes on
> whatever environment it inherits.  We already know my
> environment won't break the new build_image ('cause I
> tested it), so using cbuildbot won't uncover any new failure.
> It's possible that the buildbots have settings we don't know
> about, but neither Chris nor I have ready access to that
> environment for testing.
>
> More testing will likely delay this change by at least a
> day, and the delay will generally complicate my life.  If
> Chris and I have analyzed it right, then that effort (and
> risk) don't buy us anything.
>
> So, have we missed a trick?  Is there any other test that's
> really worth the effort?  I believe this code is ready except
> for any missing tests.
>
>
>
> http://codereview.chromium.org/6801027/
>

[sosa, 2011-04-07 22:43:23]

LGTM as well.

[zbehan, 2011-04-07 22:50:01]

LGTM from me too.

On Fri, Apr 8, 2011 at 12:43 AM, <sosa@chromium.org> wrote:

> LGTM as well.
>
>
> http://codereview.chromium.org/6801027/
>

[Vince Laviano, 2011-04-07 23:03:00]

On 2011/04/07 22:50:01, zbehan wrote:
> LGTM from me too.
> 
> On Fri, Apr 8, 2011 at 12:43 AM, <mailto:sosa@chromium.org> wrote:
> 
> > LGTM as well.
> >
> >
> > http://codereview.chromium.org/6801027/
> >

LGTM. (I think my world will remain unbroken.)