From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruce Richardson Subject: Re: [PATCH v3] mk: filter duplicate configuration entries Date: Wed, 6 Jul 2016 10:30:46 +0100 Message-ID: <20160706093045.GD17404@bricha3-MOBL3> References: <1467783465-14533-1-git-send-email-christian.ehrhardt@canonical.com> <20160706081238.GA17404@bricha3-MOBL3> <577CC7DD.1000004@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Christian Ehrhardt , thomas.monjalon@6wind.com, dev@dpdk.org To: Ferruh Yigit Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id A64C06A5B for ; Wed, 6 Jul 2016 11:30:50 +0200 (CEST) Content-Disposition: inline In-Reply-To: <577CC7DD.1000004@intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Jul 06, 2016 at 09:57:01AM +0100, Ferruh Yigit wrote: > On 7/6/2016 9:12 AM, Bruce Richardson wrote: > > On Wed, Jul 06, 2016 at 07:37:45AM +0200, Christian Ehrhardt wrote: > >> *updates in v3* > >> - replace tac with sed '1!G;h;$!d' to avoid build time dependency > >> > >> *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 > >> --- > >> mk/rte.sdkconfig.mk | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/mk/rte.sdkconfig.mk b/mk/rte.sdkconfig.mk > >> index a3acfe6..d031bf4 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) ; \ > >> + sed '1!G;h;$$!d' $(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 \ > >> + | sed '1!G;h;$$!d' > $(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 ; \ > >> -- > > > > Given the length and complexity of the work being done here, using some pretty > > fancy sed and awk features, I feel that the comment at the top should be > > expanded to actually explain what is being done and how. I would also include > > in that explanation how sed is being used to reverse a file. Personally, I > > would have preferred to keep the dependency on tac for a readability perspective. > > > > By using sed, I didn't really mean using sed instead of tac, but > something close to first version of this patch [1], these are just > different ways of doing same thing. > > I don't know how common "tac" is, but even a box breaks build because > off missing tac, that is problem, specially this change is not a must > but a good to have. > > [1] > for config in $$(grep -v "^#" $(RTE_OUTPUT)/.config_tmp | cut -d"=" -f1 > | sort | uniq -d); do \ > while [ $$(grep "^$${config}=" $(RTE_OUTPUT)/.config_tmp -c ) -gt 1 > ]; do \ > sed -i "0,/^$${config}=/{//d}" $(RTE_OUTPUT)/.config_tmp; \ > done; \ > done; \ > Well, at least on my Fedora box, tac is part of coreutils, so it's probably pretty widespread on Linux. Unfortunately, it's not available out-of-the-box on FreeBSD. /Bruce