Re: [Xen-devel] [PATCH 7/8] raisin: Rework component specification

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 20 Apr 2015, George Dunlap wrote:
> On 04/17/2015 11:37 AM, Stefano Stabellini wrote:
> > On Thu, 16 Apr 2015, George Dunlap wrote:
> >> Allow COMPONENTS to be specified in the config (or on the command-line)
> >>
> >> Now you can keep all components enabled in your config but build only
> >> one like so:
> >>
> >> COMPONENTS="xen" ./raise build
> >>
> >> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> > 
> > Good idea, I wanted to fix the way we disable components for a while now
> > 
> > 
> >> CC: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> >> ---
> >>  defconfig               |  5 +++++
> >>  lib/common-functions.sh | 49 
> >> +++++++++++++++++++++++++++++++++++--------------
> >>  2 files changed, 40 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/defconfig b/defconfig
> >> index 23c76eb..4ec8a0b 100644
> >> --- a/defconfig
> >> +++ b/defconfig
> >> @@ -1,5 +1,10 @@
> >>  # Config variables for raisin
> >>  
> >> +# Components
> >> +# All components: xen grub libvirt
> >> +# Core xen functionality: xen
> >> +DEFAULT_COMPONENTS="xen grub libvirt"
> > 
> > I would just call this COMPONENTS, also see below.
> > Also please update the other comment in this file on how to disable 
> > components.
> 
> I think the problem here is that if you just say "COMPONENTS=" in the
> config file, then it will override the COMPONENTS= on the command-line
> (i.e., 'COMPONENTS="xen" ./raise build' no longer works as expected).
> 
> We could use one of the "set if not set already" bashisms, but I think
> those are all pretty ugly, and probably easy to accidentally forget when
> editing.  Since the config file is part of the public interface, I
> thought it important to keep it simple and clean.

You are right, it is important to keep the config file simple and clean,
that is why I was suggesting to use a simpler name for the config
variable. I certainly wouldn't want any of the "set if not set already"
constructs in the config file.

I guess that having COMPONENTS="xen" ./raise build would be nice and it
is simpler to achieve it by having a different name for the variable in
the config file.

OK, you convinced me :-)




> >>  # Build config
> >>  ## Make command to run
> >>  MAKE="make -j2"
> >> diff --git a/lib/common-functions.sh b/lib/common-functions.sh
> >> index e66c6f4..42406e9 100644
> >> --- a/lib/common-functions.sh
> >> +++ b/lib/common-functions.sh
> >> @@ -31,13 +31,41 @@ function common_init() {
> >>  
> >>      get_distro
> >>      get_arch
> >> +    get_components
> >>  
> >> -    for f in `cat "$BASEDIR"/components/series`
> >> +    echo "Distro: $DISTRO"
> >> +    echo "Arch: $ARCH"
> >> +    echo "Components: $COMPONENTS"
> > 
> > if [[ $VERBOSE -eq 1 ]]
> > then
> >     echo stuff
> > fi
> > 
> > To make the code nicer to read, we could have a function called
> > verbose_echo:
> > 
> > function verbose_echo() {
> >     if [[ $VERBOSE -eq 1 ]]
> >     then
> >         echo $*
> >     fi
> > }
> 
> I'll do the 'if' first, and maybe look at making a function later.
> 
> >> +function get_components() {
> >> +    if [[ -z "$COMPONENTS" ]]
> >> +    then
> >> +  COMPONENTS="$DEFAULT_COMPONENTS"
> > 
> > Stray tab.
> 
> Weird... I thought I had emacs set to use no tabs.  I'll take a look...
> 
> > I would call our internal components variable RAISIN_COMPONENTS and the
> > one in the config and exposed to users COMPONENTS.
> 
> Any revision to this given my answer above?  Off the top of my head I
> don't see a reason to have three variables.
> 
>  -George
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel