On 11/16/22 11:47, Eli Britstein wrote:
> After resolving DPDK cast align warnings as stated in [1], and
> resolving some more warnings in OVS side, enforce -Werror for debian and
> rhel builds too.
>
> [1] 0b6d2faace76 ("ci: Remove -Wno-cast-align from CI.")
>
> Signed-off-by: Eli Britstein <el...@nvidia.com>
> ---
> debian/rules | 4 ++--
> rhel/openvswitch.spec.in | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
Hi, Eli. Thanks for fixing some warnings.
But I don't think we can enable Werror for the distribution builds
that easily. The main issue here is that we do not test on all the
versions of different distributions. We only cover Ubuntu 20.04,
for example, in our CI. Not 22.04 and not any version of Debian.
And it's not really about cast-align, it's more about compiler flags
imposed by the distribution. You may notice that on Ubuntu -flto
is enabled by default and lots of other flags. We can't control
that and we can't protect ourselves from future failures, since these
flags are different from one version of the distribution to another.
Also, having -Werror will likely mean issues for users who may try
building OVS packages from slightly outdated branches or for slightly
outdated or instead newer versions of distributions. Such users
may not know enough about build process to fix unforeseen issues
themselves. Especially because these are mostly false-positive
warnings triggered due to obscure link time optimizations or even
compiler bugs as demonstrated in this patch set.
So, I'm not sure if enabling -Werror is a good thing. At least until
we test on all supported versions of Ubuntu/Debian, which is not
really something we should do, IMO.
Also, some of the fixes in this patch set are fairly questionable.
I'd argue that we should not replace the memset() in the netlink code.
At least not without a big comment in the code on what is happening.
And moving around reset of the ol_flags is also questionable as
this is not an implementation-specific field.
CC: Frode, as he may also have some opinion on that topic.
Best regards, Ilya Maximets.
P.S. I'm traveling this week, so there could be big delay between
my replies.
>
> diff --git a/debian/rules b/debian/rules
> index 971bc1775..ffc218e9d 100755
> --- a/debian/rules
> +++ b/debian/rules
> @@ -24,7 +24,7 @@ override_dh_auto_configure:
> cd _debian && ( \
> test -e Makefile || \
> ../configure --prefix=/usr --localstatedir=/var --enable-ssl \
> - --sysconfdir=/etc \
> + --sysconfdir=/etc --enable-Werror \
> $(DATAPATH_CONFIGURE_OPTS) \
> $(EXTRA_CONFIGURE_OPTS) \
> )
> @@ -34,7 +34,7 @@ ifeq (,$(filter nodpdk, $(DEB_BUILD_OPTIONS)))
> cd _dpdk && ( \
> test -e Makefile || \
> ../configure --prefix=/usr --localstatedir=/var --enable-ssl \
> - --with-dpdk=shared --sysconfdir=/etc \
> + --with-dpdk=shared --sysconfdir=/etc --enable-Werror \
> $(DATAPATH_CONFIGURE_OPTS) \
> $(EXTRA_CONFIGURE_OPTS) \
> )
> diff --git a/rhel/openvswitch.spec.in b/rhel/openvswitch.spec.in
> index 9903dd10a..35ae42356 100644
> --- a/rhel/openvswitch.spec.in
> +++ b/rhel/openvswitch.spec.in
> @@ -70,7 +70,7 @@ Tailored Open vSwitch SELinux policy
>
> %build
> ./configure --prefix=/usr --sysconfdir=/etc
> --localstatedir=%{_localstatedir} \
> - --libdir=%{_libdir} --enable-ssl --enable-shared
> + --libdir=%{_libdir} --enable-ssl --enable-shared --enable-Werror
> make %{_smp_mflags}
> make selinux-policy
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
