[dpdk-dev,v2] mk: filter duplicate configuration entries

Message ID 1467288021-23950-1-git-send-email-christian.ehrhardt@canonical.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Commit Message

Christian Ehrhardt June 30, 2016, noon UTC
  *updates in v2*
- move to .config target
- fix usage order of tac
- simplify inner section by only using awk (instead of awk+loop+bash+sed)

Due to the hierarchy and the demand to keep the base config showing all
options, some config keys end up multiple times in the .config file.

Due to the way the actual config is sourced only the last entry is
important. That can confuse people changing values in .config which
are then ignored.

A suggested solution was to filter for duplicates at the end of the
actual config step which is implemented here.

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 mk/rte.sdkconfig.mk | 6 ++++++
 1 file changed, 6 insertions(+)
  

Comments

Ferruh Yigit July 5, 2016, 4:47 p.m. UTC | #1
On 6/30/2016 1:00 PM, Christian Ehrhardt wrote:
> *updates in v2*
> - move to .config target
> - fix usage order of tac
> - simplify inner section by only using awk (instead of awk+loop+bash+sed)
> 
> Due to the hierarchy and the demand to keep the base config showing all
> options, some config keys end up multiple times in the .config file.
> 
> Due to the way the actual config is sourced only the last entry is
> important. That can confuse people changing values in .config which
> are then ignored.
> 
> A suggested solution was to filter for duplicates at the end of the
> actual config step which is implemented here.
> 
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  mk/rte.sdkconfig.mk | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/mk/rte.sdkconfig.mk b/mk/rte.sdkconfig.mk
> index a3acfe6..6c46122 100644
> --- a/mk/rte.sdkconfig.mk
> +++ b/mk/rte.sdkconfig.mk
> @@ -79,11 +79,17 @@ $(RTE_OUTPUT):
>  ifdef NODOTCONF
>  $(RTE_OUTPUT)/.config: ;
>  else
> +# Generate config from template, if there are duplicates keep only the last
>  $(RTE_OUTPUT)/.config: $(RTE_CONFIG_TEMPLATE) FORCE | $(RTE_OUTPUT)
>  	$(Q)if [ "$(RTE_CONFIG_TEMPLATE)" != "" -a -f "$(RTE_CONFIG_TEMPLATE)" ]; then \
>  		$(CPP) -undef -P -x assembler-with-cpp \
>  		-ffreestanding \
>  		-o $(RTE_OUTPUT)/.config_tmp $(RTE_CONFIG_TEMPLATE) ; \
> +		tac $(RTE_OUTPUT)/.config_tmp > $(RTE_OUTPUT)/.config_tmp_reverse ; \
Now we are adding new binary dependency (tac) to build system and we now
create a interim file, as far as I understand which is required to use awk.
Although this is a nice piece of awk command, I am not sure if sed
alternative or this is better because of above two issues. This is a
question that I have more than a comment against one to another.

If you prefer to use this one, I confirmed that this works fine.

> +		awk --field-separator '=' '!/^#/ {if (!seen[$$1]) {print ($$0)}; seen[$$1]=1;} \
> +			/^#/ {print($$0)}' $(RTE_OUTPUT)/.config_tmp_reverse \
> +			| tac > $(RTE_OUTPUT)/.config_tmp ; \
> +		rm $(RTE_OUTPUT)/.config_tmp_reverse ; \
>  		if ! cmp -s $(RTE_OUTPUT)/.config_tmp $(RTE_OUTPUT)/.config; then \
>  			cp $(RTE_OUTPUT)/.config_tmp $(RTE_OUTPUT)/.config ; \
>  			cp $(RTE_OUTPUT)/.config_tmp $(RTE_OUTPUT)/.config.orig ; \
>
  
Thomas Monjalon July 5, 2016, 7:47 p.m. UTC | #2
2016-07-05 17:47, Ferruh Yigit:
> On 6/30/2016 1:00 PM, Christian Ehrhardt wrote:
> > +		tac $(RTE_OUTPUT)/.config_tmp > $(RTE_OUTPUT)/.config_tmp_reverse ; \
> Now we are adding new binary dependency (tac) to build system

tac can be replaced by sed '1!G;h;$!d'
  
Christian Ehrhardt July 6, 2016, 5:37 a.m. UTC | #3
Hi,
I came up with something very similar when looking for tac replacements
yesterday, but had no time to finish things.
But your suggestion is even shorter - I had found "sed -n '1{h;T;};G;h;$p;'
file" or "sed -n '1!G;h;$p'".
That removes the tac dependency, which I agree is a good thing.

To chain things up without a temp file one would need the "in-place"
features of sed&awk which I'm not sure they are available (awk >=4.1 and
only GNU awk).
sed -i is only used in validate-abi.sh which might not be used on all
platforms to count as "-i is there already so I can use it".
And I really don't want to break anyone due to that change, just naively
clean up the resulting config a bit.
Also we already have a temp file .config_tmp in the same scope and remove
it on our own.
So it is not that much different to create and remove a second one for that
section.

Thanks for both of your feedback, submitting v3 now ...


Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Tue, Jul 5, 2016 at 9:47 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> 2016-07-05 17:47, Ferruh Yigit:
> > On 6/30/2016 1:00 PM, Christian Ehrhardt wrote:
> > > +           tac $(RTE_OUTPUT)/.config_tmp >
> $(RTE_OUTPUT)/.config_tmp_reverse ; \
> > Now we are adding new binary dependency (tac) to build system
>
> tac can be replaced by sed '1!G;h;$!d'
>
>
  

Patch

diff --git a/mk/rte.sdkconfig.mk b/mk/rte.sdkconfig.mk
index a3acfe6..6c46122 100644
--- a/mk/rte.sdkconfig.mk
+++ b/mk/rte.sdkconfig.mk
@@ -79,11 +79,17 @@  $(RTE_OUTPUT):
 ifdef NODOTCONF
 $(RTE_OUTPUT)/.config: ;
 else
+# Generate config from template, if there are duplicates keep only the last
 $(RTE_OUTPUT)/.config: $(RTE_CONFIG_TEMPLATE) FORCE | $(RTE_OUTPUT)
 	$(Q)if [ "$(RTE_CONFIG_TEMPLATE)" != "" -a -f "$(RTE_CONFIG_TEMPLATE)" ]; then \
 		$(CPP) -undef -P -x assembler-with-cpp \
 		-ffreestanding \
 		-o $(RTE_OUTPUT)/.config_tmp $(RTE_CONFIG_TEMPLATE) ; \
+		tac $(RTE_OUTPUT)/.config_tmp > $(RTE_OUTPUT)/.config_tmp_reverse ; \
+		awk --field-separator '=' '!/^#/ {if (!seen[$$1]) {print ($$0)}; seen[$$1]=1;} \
+			/^#/ {print($$0)}' $(RTE_OUTPUT)/.config_tmp_reverse \
+			| tac > $(RTE_OUTPUT)/.config_tmp ; \
+		rm $(RTE_OUTPUT)/.config_tmp_reverse ; \
 		if ! cmp -s $(RTE_OUTPUT)/.config_tmp $(RTE_OUTPUT)/.config; then \
 			cp $(RTE_OUTPUT)/.config_tmp $(RTE_OUTPUT)/.config ; \
 			cp $(RTE_OUTPUT)/.config_tmp $(RTE_OUTPUT)/.config.orig ; \