| Message ID | 1467783465-14533-1-git-send-email-christian.ehrhardt@canonical.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 81B5D68E8; Wed, 6 Jul 2016 07:37:59 +0200 (CEST) Received: from youngberry.canonical.com (youngberry.canonical.com [91.189.89.112]) by dpdk.org (Postfix) with ESMTP id D3DAE68DD for <dev@dpdk.org>; Wed, 6 Jul 2016 07:37:57 +0200 (CEST) Received: from 1.general.paelzer.uk.vpn ([10.172.196.172] helo=localhost.localdomain) by youngberry.canonical.com with esmtpsa (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from <christian.ehrhardt@canonical.com>) id 1bKfX0-0003xg-LT; Wed, 06 Jul 2016 05:37:54 +0000 From: Christian Ehrhardt <christian.ehrhardt@canonical.com> To: christian.ehrhardt@canonical.com, ferruh.yigit@intel.com, thomas.monjalon@6wind.com, dev@dpdk.org Date: Wed, 6 Jul 2016 07:37:45 +0200 Message-Id: <1467783465-14533-1-git-send-email-christian.ehrhardt@canonical.com> X-Mailer: git-send-email 2.7.4 In-Reply-To: <CAATJJ0+1MWVE6C1MAr=etkSAErmNOg3mVm2k6b6yxL=VVB1e0w@mail.gmail.com> References: <CAATJJ0+1MWVE6C1MAr=etkSAErmNOg3mVm2k6b6yxL=VVB1e0w@mail.gmail.com> Subject: [dpdk-dev] [PATCH v3] mk: filter duplicate configuration entries X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Christian Ehrhardt
July 6, 2016, 5:37 a.m. UTC
*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 <christian.ehrhardt@canonical.com>
---
mk/rte.sdkconfig.mk | 6 ++++++
1 file changed, 6 insertions(+)
Comments
On 7/6/2016 6:37 AM, 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 <christian.ehrhardt@canonical.com> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
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 <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..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. /Bruce
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 <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..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; \
Thanks Ferruh, I'm personally more an awk guy so that was my natural choice. But I in general agree, the less tools used the better for dependencies and stability. I checked your suggestion and works like a charm. I'll still follow Bruce guidance to add more explanation. v4 should show up any minute ... Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd On Wed, Jul 6, 2016 at 10:57 AM, Ferruh Yigit <ferruh.yigit@intel.com> 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 <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..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; \ > > > >
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 <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..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
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 ; \