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

[George Dunlap <george.dunlap@eu.citrix.com>]

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@eu.citrix.com>
> 
> Good idea, I wanted to fix the way we disable components for a while now
> 
> 
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>> ---
>>  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.

>>  # 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