diff mbox

[dpdk-dev] scripts: fix symbol overriding in configuration files

Message ID CALwxeUvRWYPmwSR4tuH9x8QYwR1AA6qnUarMu=wM-NrK+Aq-6w@mail.gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

David Marchand Nov. 28, 2014, 2:06 p.m. UTC
Hello Bruce,

On Fri, Nov 28, 2014 at 2:56 PM, Bruce Richardson <
bruce.richardson@intel.com> wrote:

> On Thu, Nov 27, 2014 at 10:17:22AM -0800, Thomas Monjalon wrote:
> > > When redefining the same symbol in configuration (basically after an
> inclusion),
> > > we need to undefine the previous symbol to avoid "redefined" errors.
> > >
> > > Signed-off-by: David Marchand <david.marchand@6wind.com>
> >
> > Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> >
> > Applied
> >
> This patch doesn't seem to work on FreeBSD. I'm still investigating how to
> fix
> it but right now compilation with gcc/clang on FreeBSD is broken due to the
> config.h file having lines like the below in it :-(
>
> /usr/home/bruce/
> dpdk.org/x86_64-native-bsdapp-clang/include/rte_config.h:3:21: fatal
> error: extra tokens at end of #undef directive [-Wextra-tokens]
> #undef RTE_EXEC_ENVn#define RTE_EXEC_ENV "bsdapp"
>

This is probably because of the \n in the sed.

Can you try something like this (I did not like this version of my patch at
first because it is not that readable) ?

Comments

Bruce Richardson Nov. 28, 2014, 2:37 p.m. UTC | #1
On Fri, Nov 28, 2014 at 03:06:11PM +0100, David Marchand wrote:
> Hello Bruce,
> 
> On Fri, Nov 28, 2014 at 2:56 PM, Bruce Richardson <
> bruce.richardson@intel.com> wrote:
> 
> > On Thu, Nov 27, 2014 at 10:17:22AM -0800, Thomas Monjalon wrote:
> > > > When redefining the same symbol in configuration (basically after an
> > inclusion),
> > > > we need to undefine the previous symbol to avoid "redefined" errors.
> > > >
> > > > Signed-off-by: David Marchand <david.marchand@6wind.com>
> > >
> > > Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> > >
> > > Applied
> > >
> > This patch doesn't seem to work on FreeBSD. I'm still investigating how to
> > fix
> > it but right now compilation with gcc/clang on FreeBSD is broken due to the
> > config.h file having lines like the below in it :-(
> >
> > /usr/home/bruce/
> > dpdk.org/x86_64-native-bsdapp-clang/include/rte_config.h:3:21: fatal
> > error: extra tokens at end of #undef directive [-Wextra-tokens]
> > #undef RTE_EXEC_ENVn#define RTE_EXEC_ENV "bsdapp"
> >
> 
> This is probably because of the \n in the sed.
> 
> Can you try something like this (I did not like this version of my patch at
> first because it is not that readable) ?
> 
> 
> diff --git a/scripts/gen-config-h.sh b/scripts/gen-config-h.sh
> index 2fac08c..d36efd6 100755
> --- a/scripts/gen-config-h.sh
> +++ b/scripts/gen-config-h.sh
> @@ -35,9 +35,11 @@ echo "#ifndef __RTE_CONFIG_H"
>  echo "#define __RTE_CONFIG_H"
>  grep CONFIG_ $1 |
>  grep -v '^[ \t]*#' |
> -sed 's,CONFIG_\(.*\)=y.*$,#undef \1\n#define \1 1,' |
> +sed 's,CONFIG_\(.*\)=y.*$,#undef \1\
> +#define \1 1,' |
>  sed 's,CONFIG_\(.*\)=n.*$,#undef \1,' |
> -sed 's,CONFIG_\(.*\)=\(.*\)$,#undef \1\n#define \1 \2,' |
> +sed 's,CONFIG_\(.*\)=\(.*\)$,#undef \1\
> +#define \1 \2,' |
>  sed 's,\# CONFIG_\(.*\) is not set$,#undef \1,'
>  echo "#endif /* __RTE_CONFIG_H */"
>
I tried a number of different things to get sed to introduce "\n" characters,
but I missed trying that one. I've sent on an alternative fix now, which uses
tr instead of sed to insert the "\n"'s. It's not a fix I'm terribly happy with,
but having seen the above (but not tried it yet), it actually doesn't seem that
bad in comparison :-)

/Bruce
Bruce Richardson Nov. 28, 2014, 2:43 p.m. UTC | #2
On Fri, Nov 28, 2014 at 03:06:11PM +0100, David Marchand wrote:
> Hello Bruce,
> 
> On Fri, Nov 28, 2014 at 2:56 PM, Bruce Richardson <
> bruce.richardson@intel.com> wrote:
> 
> > On Thu, Nov 27, 2014 at 10:17:22AM -0800, Thomas Monjalon wrote:
> > > > When redefining the same symbol in configuration (basically after an
> > inclusion),
> > > > we need to undefine the previous symbol to avoid "redefined" errors.
> > > >
> > > > Signed-off-by: David Marchand <david.marchand@6wind.com>
> > >
> > > Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> > >
> > > Applied
> > >
> > This patch doesn't seem to work on FreeBSD. I'm still investigating how to
> > fix
> > it but right now compilation with gcc/clang on FreeBSD is broken due to the
> > config.h file having lines like the below in it :-(
> >
> > /usr/home/bruce/
> > dpdk.org/x86_64-native-bsdapp-clang/include/rte_config.h:3:21: fatal
> > error: extra tokens at end of #undef directive [-Wextra-tokens]
> > #undef RTE_EXEC_ENVn#define RTE_EXEC_ENV "bsdapp"
> >
> 
> This is probably because of the \n in the sed.
> 
> Can you try something like this (I did not like this version of my patch at
> first because it is not that readable) ?
> 
> 
> diff --git a/scripts/gen-config-h.sh b/scripts/gen-config-h.sh
> index 2fac08c..d36efd6 100755
> --- a/scripts/gen-config-h.sh
> +++ b/scripts/gen-config-h.sh
> @@ -35,9 +35,11 @@ echo "#ifndef __RTE_CONFIG_H"
>  echo "#define __RTE_CONFIG_H"
>  grep CONFIG_ $1 |
>  grep -v '^[ \t]*#' |
> -sed 's,CONFIG_\(.*\)=y.*$,#undef \1\n#define \1 1,' |
> +sed 's,CONFIG_\(.*\)=y.*$,#undef \1\
> +#define \1 1,' |
>  sed 's,CONFIG_\(.*\)=n.*$,#undef \1,' |
> -sed 's,CONFIG_\(.*\)=\(.*\)$,#undef \1\n#define \1 \2,' |
> +sed 's,CONFIG_\(.*\)=\(.*\)$,#undef \1\
> +#define \1 \2,' |
>  sed 's,\# CONFIG_\(.*\) is not set$,#undef \1,'
>  echo "#endif /* __RTE_CONFIG_H */"
> 
>
This proposed fix also works. I don't mind whether my patch proposal or this
fix gets applied - gen-config-h.sh is not a commonly modified script anyway.

/Bruce
David Marchand Nov. 28, 2014, 2:49 p.m. UTC | #3
On Fri, Nov 28, 2014 at 3:43 PM, Bruce Richardson <
bruce.richardson@intel.com> wrote:

> This proposed fix also works. I don't mind whether my patch proposal or
> this
> fix gets applied - gen-config-h.sh is not a commonly modified script
> anyway.
>

I prefer my ugliness ;-)
But Thomas just proposed me something that could work.
Trying ...
Bruce Richardson Nov. 28, 2014, 2:59 p.m. UTC | #4
On Fri, Nov 28, 2014 at 03:49:29PM +0100, David Marchand wrote:
> On Fri, Nov 28, 2014 at 3:43 PM, Bruce Richardson <
> bruce.richardson@intel.com> wrote:
> 
> > This proposed fix also works. I don't mind whether my patch proposal or
> > this
> > fix gets applied - gen-config-h.sh is not a commonly modified script
> > anyway.
> >
> 
> I prefer my ugliness ;-)
> But Thomas just proposed me something that could work.
> Trying ...
>
Yes, it's ugly, but it's probably more resilient. I'm looking forward to getting
an option C.

/Bruce
David Marchand Nov. 28, 2014, 3:41 p.m. UTC | #5
On Fri, Nov 28, 2014 at 3:59 PM, Bruce Richardson <
bruce.richardson@intel.com> wrote:

> Yes, it's ugly, but it's probably more resilient. I'm looking forward to
> getting
> an option C.
>

Option C and D are ugly as well (using some bashism like $' ' or using an
intermediate variable with a newline in it).
Our "posix" guy told me that he prefers the initial version of the patch.
I will send a patch with this.

Thomas ?
diff mbox

Patch

diff --git a/scripts/gen-config-h.sh b/scripts/gen-config-h.sh
index 2fac08c..d36efd6 100755
--- a/scripts/gen-config-h.sh
+++ b/scripts/gen-config-h.sh
@@ -35,9 +35,11 @@  echo "#ifndef __RTE_CONFIG_H"
 echo "#define __RTE_CONFIG_H"
 grep CONFIG_ $1 |
 grep -v '^[ \t]*#' |
-sed 's,CONFIG_\(.*\)=y.*$,#undef \1\n#define \1 1,' |
+sed 's,CONFIG_\(.*\)=y.*$,#undef \1\
+#define \1 1,' |
 sed 's,CONFIG_\(.*\)=n.*$,#undef \1,' |
-sed 's,CONFIG_\(.*\)=\(.*\)$,#undef \1\n#define \1 \2,' |
+sed 's,CONFIG_\(.*\)=\(.*\)$,#undef \1\
+#define \1 \2,' |
 sed 's,\# CONFIG_\(.*\) is not set$,#undef \1,'
 echo "#endif /* __RTE_CONFIG_H */"