Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6881112:
Archive build to pass USE flags to subcommands. (Closed)
|
Created:
9 years, 8 months ago by Peter Mayo Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa, dgarrett Visibility:
Public. |
DescriptionArchive build uses commands that may need to reassemble and re-emerge components. This gets it to pass USE flags in case some of the components are desired in a non-default configuration.
Change-Id: I3d096ec63510e470aad25de8da1112be28a5bfca
BUG=78345
TEST=Manual runs.
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=32125c5
Patch Set 1 #Patch Set 2 : Fix missing initialization. #Patch Set 3 : Variable rename. #
Total comments: 3
Patch Set 4 : Change usage string for useflags #Messages
Total messages: 21 (0 generated)
LGTM w/nit Change the name from ENV_SPEC to CHROOT_ENV and add a comment as to what it does. I'm also going to be on vacation Friday through Monday so if you make major changes you don't need to wait on my review if the other reviewers sign off.
I am planning to babysit this through the first 8-12 hours, and would beg anyone suspecting problems after that to revert. I would like an LGTM conditional on my test passing.
Ping?
+anush http://codereview.chromium.org/6881112/diff/2002/archive_build.sh File archive_build.sh (right): http://codereview.chromium.org/6881112/diff/2002/archive_build.sh#newcode78 archive_build.sh:78: "Space separated flags specifying types of pieces to assemble of modify." Lots of other scripts accept USE flags, and they do so via environment variable instead of command-line options. Why is this script an exception? Understood that environment variables are maybe a pain. But Emerge uses environment variables for its interface, and sosa@ recently converted gmerge from using command-line options to environment variables. So I'm thinking we should try to stick to one way of doing this for consistency...
The scripts accepting USE flags from their environment ... are they inside or outside chroot scripts? My understanding is that they are safe inside chroot, and potentially in conflict outside. This script is called from outside during the build process. On Mon, Apr 25, 2011 at 4:58 PM, <davidjames@chromium.org> wrote: > +anush > > > http://codereview.chromium.org/6881112/diff/2002/archive_build.sh > File archive_build.sh (right): > > http://codereview.chromium.org/6881112/diff/2002/archive_build.sh#newcode78 > archive_build.sh:78: "Space separated flags specifying types of pieces > to assemble of modify." > Lots of other scripts accept USE flags, and they do so via environment > variable instead of command-line options. Why is this script an > exception? > > Understood that environment variables are maybe a pain. But Emerge uses > environment variables for its interface, and sosa@ recently converted > gmerge from using command-line options to environment variables. So I'm > thinking we should try to stick to one way of doing this for > consistency... > > > http://codereview.chromium.org/6881112/ >
As Peter says archive_build runs outside the chroot so can't blindly accept USE flags from the environment. And I'm not aware of any scripts from outside the chroot that support the USE environment variable. I can see an alternative where a flag would tell it to pass the USE environment through, but it shouldn't use the USE environment blindly. Hmm. Maybe longterm a CROS_USE environment variable the enter_chroot would translate into USE inside the chroot? That might be needed with some of the plans for Chromite that I read about too if the notion of running fully outside the chroot is supported. Actually that might be an easy enough change for here, I expect the debate will be tougher than the implementation. On Mon, Apr 25, 2011 at 5:03 PM, Peter Mayo <petermayo@google.com> wrote: > The scripts accepting USE flags from their environment ... are they inside > or outside chroot scripts? > > My understanding is that they are safe inside chroot, and potentially in > conflict outside. This script is called from outside during the build > process. > > > On Mon, Apr 25, 2011 at 4:58 PM, <davidjames@chromium.org> wrote: > >> +anush >> >> >> http://codereview.chromium.org/6881112/diff/2002/archive_build.sh >> File archive_build.sh (right): >> >> >> http://codereview.chromium.org/6881112/diff/2002/archive_build.sh#newcode78 >> archive_build.sh:78: "Space separated flags specifying types of pieces >> to assemble of modify." >> Lots of other scripts accept USE flags, and they do so via environment >> variable instead of command-line options. Why is this script an >> exception? >> >> Understood that environment variables are maybe a pain. But Emerge uses >> environment variables for its interface, and sosa@ recently converted >> gmerge from using command-line options to environment variables. So I'm >> thinking we should try to stick to one way of doing this for >> consistency... >> >> >> http://codereview.chromium.org/6881112/ >> > >
I can do CROS_USE for this change very quickly. I changed from passing USE flags in the env when I found it didn't work and dug into why. Peter. On Mon, Apr 25, 2011 at 5:07 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: > As Peter says archive_build runs outside the chroot so can't blindly accept > USE flags from the environment. And I'm not aware of any scripts from > outside the chroot that support the USE environment variable. > > I can see an alternative where a flag would tell it to pass the USE > environment through, but it shouldn't use the USE environment blindly. > > Hmm. Maybe longterm a CROS_USE environment variable the enter_chroot would > translate into USE inside the chroot? That might be needed with some of the > plans for Chromite that I read about too if the notion of running fully > outside the chroot is supported. Actually that might be an easy enough > change for here, I expect the debate will be tougher than the > implementation. > > > On Mon, Apr 25, 2011 at 5:03 PM, Peter Mayo <petermayo@google.com> wrote: > >> The scripts accepting USE flags from their environment ... are they inside >> or outside chroot scripts? >> >> My understanding is that they are safe inside chroot, and potentially in >> conflict outside. This script is called from outside during the build >> process. >> >> >> On Mon, Apr 25, 2011 at 4:58 PM, <davidjames@chromium.org> wrote: >> >>> +anush >>> >>> >>> http://codereview.chromium.org/6881112/diff/2002/archive_build.sh >>> File archive_build.sh (right): >>> >>> >>> http://codereview.chromium.org/6881112/diff/2002/archive_build.sh#newcode78 >>> archive_build.sh:78: "Space separated flags specifying types of pieces >>> to assemble of modify." >>> Lots of other scripts accept USE flags, and they do so via environment >>> variable instead of command-line options. Why is this script an >>> exception? >>> >>> Understood that environment variables are maybe a pain. But Emerge uses >>> environment variables for its interface, and sosa@ recently converted >>> gmerge from using command-line options to environment variables. So I'm >>> thinking we should try to stick to one way of doing this for >>> consistency... >>> >>> >>> http://codereview.chromium.org/6881112/ >>> >> >> >
Anush, Sosa, :Ping: David, Jon, Did you prefer the different env variable to the explicit argument? Alternately would you prefer a generic pway of passing env to the chroot? (--chroot_add_env USE="a b") with all of the quoting headaches?
Environment variables are better here I think but I agree with something CROS specific for outside the chroot (i.e. CROS_USE), but to be honest, archive_build is going away so I don't feel strongly about it either way for this CL.
My personal preference right now is to add a flag to enter_chroot which will always pass an existing environment variable through as the USE flags. I can see it like this: enter_chroot --use_var CROS_USE or enter_chroot --use_var USE which would take that environment variable from the host environment and copy it to the USE environment variable inside the chroot. And then modify archive_build to set and use that flag. This should be pretty simple, gives us a lot of flexibility, and won't interfere with any existing scripts or settings. On Thu, Apr 28, 2011 at 1:20 PM, <petermayo@chromium.org> wrote: > Anush, Sosa, :Ping: > > David, Jon, Did you prefer the different env variable to the explicit > argument? > Alternately would you prefer a generic pway of passing env to the chroot? > (--chroot_add_env USE="a b") with all of the quoting headaches? > > > http://codereview.chromium.org/6881112/ >
And to echo sosa - I don't have strong feelings on this. This is my view on the best approach but can easily be convinced of others if people feel strongly. On Thu, Apr 28, 2011 at 1:50 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: > My personal preference right now is to add a flag to enter_chroot which > will always pass an existing environment variable through as the USE flags. > > I can see it like this: > > enter_chroot --use_var CROS_USE > or > enter_chroot --use_var USE > > which would take that environment variable from the host environment and > copy it to the USE environment variable inside the chroot. > > And then modify archive_build to set and use that flag. This should be > pretty simple, gives us a lot of flexibility, and won't interfere with any > existing scripts or settings. > > > On Thu, Apr 28, 2011 at 1:20 PM, <petermayo@chromium.org> wrote: > >> Anush, Sosa, :Ping: >> >> David, Jon, Did you prefer the different env variable to the explicit >> argument? >> Alternately would you prefer a generic pway of passing env to the chroot? >> (--chroot_add_env USE="a b") with all of the quoting headaches? >> >> >> http://codereview.chromium.org/6881112/ >> > >
That seems to me like a fair bit of rework through many levels to end up with a more cryptic system. Rather than rely on environment variables, could we have enter_chroot read a config file for what it is supposed to set up? That way it would be locally 'right' no matter which way it got there? On Thu, Apr 28, 2011 at 1:51 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: > And to echo sosa - I don't have strong feelings on this. This is my view > on the best approach but can easily be convinced of others if people feel > strongly. > > > On Thu, Apr 28, 2011 at 1:50 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: > >> My personal preference right now is to add a flag to enter_chroot which >> will always pass an existing environment variable through as the USE flags. >> >> I can see it like this: >> >> enter_chroot --use_var CROS_USE >> or >> enter_chroot --use_var USE >> >> which would take that environment variable from the host environment and >> copy it to the USE environment variable inside the chroot. >> >> And then modify archive_build to set and use that flag. This should be >> pretty simple, gives us a lot of flexibility, and won't interfere with any >> existing scripts or settings. >> >> >> On Thu, Apr 28, 2011 at 1:20 PM, <petermayo@chromium.org> wrote: >> >>> Anush, Sosa, :Ping: >>> >>> David, Jon, Did you prefer the different env variable to the explicit >>> argument? >>> Alternately would you prefer a generic pway of passing env to the >>> chroot? >>> (--chroot_add_env USE="a b") with all of the quoting headaches? >>> >>> >>> http://codereview.chromium.org/6881112/ >>> >> >> >
LGTM (meta discussion): Sorry - I realized I accidentally went off into design mode rather than "review CL" mode. I'm fine with this CL as is. My thoughts on enter_chroot changes aren't really needed unless there are problems and this CL needs to be changed. On Thu, Apr 28, 2011 at 2:11 PM, Peter Mayo <petermayo@google.com> wrote: > That seems to me like a fair bit of rework through many levels to end up > with a more cryptic system. > Rather than rely on environment variables, could we have enter_chroot read > a config file for what it is supposed to set up? That way it would be > locally 'right' no matter which way it got there? > > > On Thu, Apr 28, 2011 at 1:51 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: > >> And to echo sosa - I don't have strong feelings on this. This is my view >> on the best approach but can easily be convinced of others if people feel >> strongly. >> >> >> On Thu, Apr 28, 2011 at 1:50 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: >> >>> My personal preference right now is to add a flag to enter_chroot which >>> will always pass an existing environment variable through as the USE flags. >>> >>> I can see it like this: >>> >>> enter_chroot --use_var CROS_USE >>> or >>> enter_chroot --use_var USE >>> >>> which would take that environment variable from the host environment and >>> copy it to the USE environment variable inside the chroot. >>> >>> And then modify archive_build to set and use that flag. This should be >>> pretty simple, gives us a lot of flexibility, and won't interfere with any >>> existing scripts or settings. >>> >>> >>> On Thu, Apr 28, 2011 at 1:20 PM, <petermayo@chromium.org> wrote: >>> >>>> Anush, Sosa, :Ping: >>>> >>>> David, Jon, Did you prefer the different env variable to the explicit >>>> argument? >>>> Alternately would you prefer a generic pway of passing env to the >>>> chroot? >>>> (--chroot_add_env USE="a b") with all of the quoting headaches? >>>> >>>> >>>> http://codereview.chromium.org/6881112/ >>>> >>> >>> >> >
+ dianders Peter Mayo (petermayo@google.com) wrote: > That seems to me like a fair bit of rework through many levels to end up > with a more cryptic system. > Rather than rely on environment variables, could we have enter_chroot read a > config file for what it is supposed to set up? That way it would be locally > 'right' no matter which way it got there? > > On Thu, Apr 28, 2011 at 1:51 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: > > > And to echo sosa - I don't have strong feelings on this. This is my view > > on the best approach but can easily be convinced of others if people feel > > strongly. > > > > > > On Thu, Apr 28, 2011 at 1:50 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: > > > >> My personal preference right now is to add a flag to enter_chroot which > >> will always pass an existing environment variable through as the USE flags. > >> > >> I can see it like this: > >> > >> enter_chroot --use_var CROS_USE > >> or > >> enter_chroot --use_var USE > >> Not totally following the discussion but I thought enter_chroot.sh supported passing environment variables via the command-line. Is that not sufficient for your purposes? commit 9362fa85c74eadbe6abb1d463000c3735a3a7caf Author: Doug Anderson <dianders@google.com> Date: Thu Dec 16 14:44:12 2010 -0800 Allow specifying environment variables w/ enter_chroot again. Examples of how people might be using enter_chroot: 1. ./enter_chroot [chroot_flags] VAR1=val1 VAR2=val2 -- cmd arg1 arg2 Set env vars and run cmd w/ args 2. ./enter_chroot [chroot_flags] VAR1=val1 VAR2=val2 Set env vars and run shell 3. ./enter_chroot [chroot_flags] -- cmd arg1 arg2 Run cmd w/ args 4. ./enter_chroot [chroot_flags] VAR1=val1 VAR2=val2 cmd arg1 arg2 Like #1 _if_ args aren't flags (if they are, enter_chroot will claim them) 5. ./enter_chroot [chroot_flags] cmd arg1 arg2 Like #3 _if_ args aren't flags (if they are, enter_chroot will claim them) I also updated the help to indicate that whole-command quoting is no longer supported. If you really need whole-command quoting (maybe you want in- chroot redirection), you'll need to use sh -c like: ./enter_chroot.sh -- sh -c "echo \"Save me\" > /tmp/save.txt" You should avoid single quotes in the command and arguments. This isn't a new limitation: it's shflags related. Change-Id: I0452a8730ac9b8197834edc753b9eece69896135 BUG=chromium-os:7072 TEST=Tried a whole bunch of these commands. Review URL: http://codereview.chromium.org/5840003 > >> which would take that environment variable from the host environment and > >> copy it to the USE environment variable inside the chroot. > >> > >> And then modify archive_build to set and use that flag. This should be > >> pretty simple, gives us a lot of flexibility, and won't interfere with any > >> existing scripts or settings. > >> > >> > >> On Thu, Apr 28, 2011 at 1:20 PM, <petermayo@chromium.org> wrote: > >> > >>> Anush, Sosa, :Ping: > >>> > >>> David, Jon, Did you prefer the different env variable to the explicit > >>> argument? > >>> Alternately would you prefer a generic pway of passing env to the > >>> chroot? > >>> (--chroot_add_env USE="a b") with all of the quoting headaches? > >>> > >>> > >>> http://codereview.chromium.org/6881112/ > >>> > >> > >> > >
It does and is. Some of the meta-debate that spawned was about how to automate passing USE flags through enter_chroot without needing to specify them on the command line. The question that brought this up for this CL was a script wrapped around enter_chroot and how it should handle USE flags being passed to it as command line arguments rather than as environment variables. Most (if not all) other scripts specify USE flags via the environment so this one would be different. On Thu, Apr 28, 2011 at 2:38 PM, <msb@google.com> wrote: > + dianders > > Peter Mayo (petermayo@google.com) wrote: > > That seems to me like a fair bit of rework through many levels to end up > > with a more cryptic system. > > Rather than rely on environment variables, could we have enter_chroot > read a > > config file for what it is supposed to set up? That way it would be > locally > > 'right' no matter which way it got there? > > > > On Thu, Apr 28, 2011 at 1:51 PM, Jonathan Kliegman <kliegs@chromium.org > >wrote: > > > > > And to echo sosa - I don't have strong feelings on this. This is my > view > > > on the best approach but can easily be convinced of others if people > feel > > > strongly. > > > > > > > > > On Thu, Apr 28, 2011 at 1:50 PM, Jonathan Kliegman < > kliegs@chromium.org>wrote: > > > > > >> My personal preference right now is to add a flag to enter_chroot > which > > >> will always pass an existing environment variable through as the USE > flags. > > >> > > >> I can see it like this: > > >> > > >> enter_chroot --use_var CROS_USE > > >> or > > >> enter_chroot --use_var USE > > >> > > Not totally following the discussion but I thought enter_chroot.sh > supported passing environment variables via the command-line. Is that > not sufficient for your purposes? > > commit 9362fa85c74eadbe6abb1d463000c3735a3a7caf > Author: Doug Anderson <dianders@google.com> > Date: Thu Dec 16 14:44:12 2010 -0800 > > Allow specifying environment variables w/ enter_chroot again. > > Examples of how people might be using enter_chroot: > > 1. ./enter_chroot [chroot_flags] VAR1=val1 VAR2=val2 -- cmd arg1 arg2 > Set env vars and run cmd w/ args > 2. ./enter_chroot [chroot_flags] VAR1=val1 VAR2=val2 > Set env vars and run shell > 3. ./enter_chroot [chroot_flags] -- cmd arg1 arg2 > Run cmd w/ args > 4. ./enter_chroot [chroot_flags] VAR1=val1 VAR2=val2 cmd arg1 arg2 > Like #1 _if_ args aren't flags (if they are, enter_chroot will claim > them) > 5. ./enter_chroot [chroot_flags] cmd arg1 arg2 > Like #3 _if_ args aren't flags (if they are, enter_chroot will claim > them) > > I also updated the help to indicate that whole-command quoting is no > longer > supported. If you really need whole-command quoting (maybe you want in- > chroot redirection), you'll need to use sh -c like: > ./enter_chroot.sh -- sh -c "echo \"Save me\" > /tmp/save.txt" > > You should avoid single quotes in the command and arguments. This isn't > a new limitation: it's shflags related. > > Change-Id: I0452a8730ac9b8197834edc753b9eece69896135 > > BUG=chromium-os:7072 > TEST=Tried a whole bunch of these commands. > > Review URL: http://codereview.chromium.org/5840003 > > > >> which would take that environment variable from the host environment > and > > >> copy it to the USE environment variable inside the chroot. > > >> > > >> And then modify archive_build to set and use that flag. This should > be > > >> pretty simple, gives us a lot of flexibility, and won't interfere with > any > > >> existing scripts or settings. > > >> > > >> > > >> On Thu, Apr 28, 2011 at 1:20 PM, <petermayo@chromium.org> wrote: > > >> > > >>> Anush, Sosa, :Ping: > > >>> > > >>> David, Jon, Did you prefer the different env variable to the > explicit > > >>> argument? > > >>> Alternately would you prefer a generic pway of passing env to the > > >>> chroot? > > >>> (--chroot_add_env USE="a b") with all of the quoting headaches? > > >>> > > >>> > > >>> http://codereview.chromium.org/6881112/ > > >>> > > >> > > >> > > > >
Based on discussion, this patch looks fine. Please just clean up the comment. I also think we shouldn't make a habit of adding --use='foo' to all scripts -- this is just a special exception for archive_build.sh because of its special situation http://codereview.chromium.org/6881112/diff/2002/archive_build.sh File archive_build.sh (right): http://codereview.chromium.org/6881112/diff/2002/archive_build.sh#newcode78 archive_build.sh:78: "Space separated flags specifying types of pieces to assemble of modify." Could you clarify this comment? "Space separated flags specifying types of pieces to assemble of modify." Better would just to say "USE flags to pass in to emerge."
Joining late, but things look OK to me. I will let others debate the right syntax (don't want too many cooks), but I don't see any problems with what's there. One question, though: do we ever anticipate wanting to pass more environment variables into archive_build, like "FEATURES"? If so, we might think about using syntax like enter_chroot does? If not, then I think the syntax proposed here is perfect! :)
Just as a general note, enter_chroot also has the ability to pass a
controlled set of variables straight through, just because they are set
externally. I'm planning to start taking advantage of this for
CHROMEOS_OFFICIAL shortly, though I hope the variable can totally go away
soon after that.
CHROOT_PASSTHRU="BUILDBOT_BUILD=$FLAGS_build_number
CHROMEOS_OFFICIAL=$CHROMEOS_OFFICIAL"
CHROOT_PASSTHRU="${CHROOT_PASSTHRU} \
CHROMEOS_RELEASE_APPID=${CHROMEOS_RELEASE_APPID:-"{DEV-BUILD}"}"
# Set CHROMEOS_VERSION_TRACK, CHROMEOS_VERSION_AUSERVER,
# CHROMEOS_VERSION_DEVSERVER as environment variables to override the
default
# assumptions (local AU server). These are used in cros_set_lsb_release, and
# are used by external Chromium OS builders.
CHROOT_PASSTHRU="${CHROOT_PASSTHRU} \
CHROMEOS_VERSION_TRACK=${CHROMEOS_VERSION_TRACK} \
CHROMEOS_VERSION_AUSERVER=${CHROMEOS_VERSION_AUSERVER} \
CHROMEOS_VERSION_DEVSERVER=${CHROMEOS_VERSION_DEVSERVER}"
On Thu, Apr 28, 2011 at 12:27 PM, <dianders@chromium.org> wrote:
> Joining late, but things look OK to me. I will let others debate the right
> syntax (don't want too many cooks), but I don't see any problems with
> what's
> there.
>
> One question, though: do we ever anticipate wanting to pass more
> environment
> variables into archive_build, like "FEATURES"? If so, we might think about
> using syntax like enter_chroot does? If not, then I think the syntax
> proposed
> here is perfect! :)
>
>
> http://codereview.chromium.org/6881112/
>
--
Don
Oh thanks Don - I didn't realize that variable existed. Could definitely be useful in the future. On Thu, Apr 28, 2011 at 3:52 PM, Don Garrett <dgarrett@chromium.org> wrote: > Just as a general note, enter_chroot also has the ability to pass a > controlled set of variables straight through, just because they are set > externally. I'm planning to start taking advantage of this for > CHROMEOS_OFFICIAL shortly, though I hope the variable can totally go away > soon after that. > > > CHROOT_PASSTHRU="BUILDBOT_BUILD=$FLAGS_build_number > CHROMEOS_OFFICIAL=$CHROMEOS_OFFICIAL" > CHROOT_PASSTHRU="${CHROOT_PASSTHRU} \ > CHROMEOS_RELEASE_APPID=${CHROMEOS_RELEASE_APPID:-"{DEV-BUILD}"}" > > # Set CHROMEOS_VERSION_TRACK, CHROMEOS_VERSION_AUSERVER, > # CHROMEOS_VERSION_DEVSERVER as environment variables to override the > default > # assumptions (local AU server). These are used in cros_set_lsb_release, > and > # are used by external Chromium OS builders. > CHROOT_PASSTHRU="${CHROOT_PASSTHRU} \ > CHROMEOS_VERSION_TRACK=${CHROMEOS_VERSION_TRACK} \ > CHROMEOS_VERSION_AUSERVER=${CHROMEOS_VERSION_AUSERVER} \ > CHROMEOS_VERSION_DEVSERVER=${CHROMEOS_VERSION_DEVSERVER}" > > On Thu, Apr 28, 2011 at 12:27 PM, <dianders@chromium.org> wrote: > >> Joining late, but things look OK to me. I will let others debate the >> right >> syntax (don't want too many cooks), but I don't see any problems with >> what's >> there. >> >> One question, though: do we ever anticipate wanting to pass more >> environment >> variables into archive_build, like "FEATURES"? If so, we might think >> about >> using syntax like enter_chroot does? If not, then I think the syntax >> proposed >> here is perfect! :) >> >> >> http://codereview.chromium.org/6881112/ >> > > > > -- > Don >
On 2011/04/28 19:52:11, dgarrett wrote:
> Just as a general note, enter_chroot also has the ability to pass a
> controlled set of variables straight through, just because they are set
> externally. I'm planning to start taking advantage of this for
> CHROMEOS_OFFICIAL shortly, though I hope the variable can totally go away
> soon after that.
>
>
Hmm - if I had used this then this line would probably have busted the
specialization if we were to ever have tried and official release of another
variant.
If it is incremental, then we should be careful not to reset it like this. This
is where the general ends up with larger conflict domains than specific flags.
> CHROOT_PASSTHRU="BUILDBOT_BUILD=$FLAGS_build_number
> CHROMEOS_OFFICIAL=$CHROMEOS_OFFICIAL"
> CHROOT_PASSTHRU="${CHROOT_PASSTHRU} \
> CHROMEOS_RELEASE_APPID=${CHROMEOS_RELEASE_APPID:-"{DEV-BUILD}"}"
>
> # Set CHROMEOS_VERSION_TRACK, CHROMEOS_VERSION_AUSERVER,
> # CHROMEOS_VERSION_DEVSERVER as environment variables to override the
> default
> # assumptions (local AU server). These are used in cros_set_lsb_release, and
> # are used by external Chromium OS builders.
> CHROOT_PASSTHRU="${CHROOT_PASSTHRU} \
> CHROMEOS_VERSION_TRACK=${CHROMEOS_VERSION_TRACK} \
> CHROMEOS_VERSION_AUSERVER=${CHROMEOS_VERSION_AUSERVER} \
> CHROMEOS_VERSION_DEVSERVER=${CHROMEOS_VERSION_DEVSERVER}"
>
> On Thu, Apr 28, 2011 at 12:27 PM, <mailto:dianders@chromium.org> wrote:
>
> > Joining late, but things look OK to me. I will let others debate the right
> > syntax (don't want too many cooks), but I don't see any problems with
> > what's
> > there.
> >
> > One question, though: do we ever anticipate wanting to pass more
> > environment
> > variables into archive_build, like "FEATURES"? If so, we might think about
> > using syntax like enter_chroot does? If not, then I think the syntax
> > proposed
> > here is perfect! :)
> >
> >
> > http://codereview.chromium.org/6881112/
> >
>
>
>
> --
> Don
http://codereview.chromium.org/6881112/diff/2002/archive_build.sh File archive_build.sh (right): http://codereview.chromium.org/6881112/diff/2002/archive_build.sh#newcode78 archive_build.sh:78: "Space separated flags specifying types of pieces to assemble of modify." On 2011/04/28 19:02:14, davidjames wrote: > Could you clarify this comment? "Space separated flags specifying types of > pieces to assemble of modify." Better would just to say "USE flags to pass in to > emerge." I'm OK with dropping space separated, and referencing commands we call more explicitly, but I don't like specifying one we don't actually call. We call mod_image_for_recovery, mod_image_for_test and build_image. They are the ones that do the wrong things without the variable set. They are called through enter_chroot, so either them or enter_chroot makes sense to references, IMHO. |
