[Buildroot] [PATCH 1/1] Support: Config generation script

Arnout Vandecappelle arnout at mind.be
Thu Aug 24 22:46:04 UTC 2017


 Hi Sam,

 The summary line should be something like:

utils/diffconfig: new kconfig fragment generation script

On 23-08-17 16:21, Sam Voss wrote:
> Add script to create kconfig fragments that are directly usable.
> 
> - Sort commented out packages
> - Output to stdout, must be redirected
> - Add linux example
> 
> Signed-off-by: Sam Voss <sam.voss at rockwellcollins.com>
> 
> ---
> 
> Relates to: https://patchwork.ozlabs.org/patch/686694/

 Good that you added a reference to that previous version!


> Missing feature to combine all fragments used to create newest
> configuration file, which can make it difficult to diff large fragment
> sets. I was hoping to leverage buildroot's fragment combiner, but haven't
> had a chance to look into it.
> ---
>  support/scripts/diffconfig.sh | 75 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100755 support/scripts/diffconfig.sh
> 
> diff --git a/support/scripts/diffconfig.sh b/support/scripts/diffconfig.sh
> new file mode 100755
> index 0000000..59227ad
> --- /dev/null
> +++ b/support/scripts/diffconfig.sh

 We recently added a top-level utils/ directory for the user-facing scripts. It
also has a readme.txt that should be updated with a brief explanation of the tool.

 Drop the .sh extension. It's not relevant that it's a shell script.

 I'm also not entirely convinced that diffconfig is the best name, since it
completely different from the one in the kernel, and it's not at all making a
diff of configs. Perhaps splitconfig? fragmentconfig? But I'm not too bothered
about the exact name.

 However, why don't you just copy the kernel's diffconfig script? What is
missing in 'diffconfig -m' that this script adds? And in fact, why do we need
this script in Buildroot? Shouldn't we just refer to it from the documentation?

> @@ -0,0 +1,75 @@
> +#!/bin/bash

 Use /usr/bin/env bash for people who put bash somewhere else.

> +
> +################################################################################
> +# DESCRIPTION:
> +#     Creates a fragment by comparing two kconfigs.
> +#
> +#     Required arguments:
> +#       $1 - config #1. This is generally the new one created
> +#       $2 - config #2: The configuration file to compare against.

 This just repeats the usage() immediately below so not very useful.

> +#
> +# Linux Example
> +# ./support/scripts/diffconfig.sh <outputdir>/build/linux-x.y.z/defconfig
> +#     board/custom-board/linux-x.y.z.config

 This would be useful to add to usage() output. And extend it a little:

Example to generate a linux config fragment:
make linux-savedefconfig
$0 output/build/linux-4.*/defconfig board/my-board/linux.config \
   > board/my-board/linux-feature.fragment

 However, I think it's more intuitive to swap the two arguments, i.e. first old,
then new; or first minimal, then complete. That's more analogous to diff: the
fragment is a patch, and you apply the patch to the first argument of diff to
get the second argument.

> +################################################################################
> +
> +usage ()
> +{
> +cat <<-_EOF_
> +	diffconfig.sh:

 Use $0 to get the actual executable name.

 Also you usually put the arguments here, like

	Usage: $0 FULLCONFIG BASECONFIG

> +	Creates a fragment by comparing two kconfigs.

 Summary line uses an imperative, so "Create a fragment ..."

> +
> +	Required arguments:
> +	$1 - config #1. This is generally the new one created

 $1 will be expanded, no? Use the names you used above, so FULLCONFIG here.

> +	$2 - config #2: The configuration file to compare against.
> +_EOF_
> +
> +    exit 1
> +}
> +
> +if [ -z "$1" ] || [ -z "$2" ]; then

 Both must exist already, so test with -e instead. Or even -r (readable).

> +        usage
> +fi
> +
> +declare -a loaded_first_config loaded_second_config
> +
> +# spaces must be removed, and then re-replaced.
> +SPACE_DELIM="_s_p_a_c_e_"

 Why is this needed? You do proper quoting everywhere, and sort and comm work
line by line...

> +
> +while read line; do
> +        if [ "${line:0:1}" != "#" ]; then
> +                loaded_first_config+=("${line// /${SPACE_DELIM}}")
> +        elif [ "${line:2:3}" = "BR2" ] || [ "${line:2:6}" = "CONFIG" ]; then

 That's not so nice, it might start with something else as well... A commented
line will always end with " is not set" so I would match with that. I think it
would be easier to write these conditions as a case:

	case "${line}" in
	("# "*" is not set")
		...
		;;
	("# "*) ;; # Ignore comments
	(*)
		loaded_first_config+=...

 You should probably also eliminate empty lines, so use (?*) instead of (*).

> +                # was a commented line, but still needs alpha sorting
> +                NOC="${line:2}"
> +                loaded_first_config+=("${NOC// /${SPACE_DELIM}}")
> +        fi
> +done < "$1" # load file 1

 Could you factor this into a function? Instead of appending to the array, you
can just echo the lines. And then...

> +
> +while read line; do
> +        if [ "${line:0:1}" != "#" ]; then
> +                loaded_second_config+=("${line// /${SPACE_DELIM}}")
> +        elif [ "${line:2:3}" = "BR2" ] || [ "${line:2:6}" = "CONFIG" ]; then
> +                # was a commented line, but still needs alpha sorting
> +                NOC="${line:2}"
> +                loaded_second_config+=("${NOC// /${SPACE_DELIM}}")
> +        fi
> +done < "$2" # load file 2
> +
> +D=($(comm -23 <(printf '%s\n' "${loaded_first_config[@]}" | sort -d) <(printf '%s\n' "${loaded_second_config[@]}" | sort -d)))

 ... this becomes

D=($(comm -23 <(load_config $1 | sort -d) <(load_config $2 | sort -d))

 Don't you also have to do a comm -13 to get the lines that were removed, and
add those commented out? And what with string or integer options that get a
different value?

> +
> +# Earlier, the '# ' were removed from the beginning of not-set packages for
> +#  sorting. Add them back.
> +declare -a NFRAG
> +
> +for line in "${D[@]}"; do
> +    # if the line does not have an '=', it was a comment
> +    if [ "${line#*=*}" = "${line}" ]; then
> +        NFRAG+=("# ${line}")

 Instead of building up yet another array, you could directly do the printf
here. Or do line="# ${line}" here and do the printf after the condition.

 Regards,
 Arnout


> +    else
> +        NFRAG+=($line)
> +    fi
> +done
> +
> +# Earlier, spaces were replaced with '_space_'. Revert this and dump to stdout.
> +printf "%s\n" "${NFRAG[@]//${SPACE_DELIM}/ }"
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF



More information about the buildroot mailing list