On 16/11/2022 16:20, Ilya Maximets wrote:
> 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 <[email protected]>
>> ---
>> 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.
>
Hi Ilya,
I have something to add here. without getting into the questionable fixes
as you mentioned. this is the upstream repository. Aren't the spec file
here and debian files are for a working example?
Each distribution usually maintain their own repository where they take
some stable ovs version, add more recent fixes, revert some stuff maybe,
and probably have their own spec file or debian packaging files.
So what's wrong with upstream repository having it's own default
with -Werror ?
Changing the spec/debian here should not and probably is not affecting
a distribution release version.
Thanks,
Roi
>>
>> 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
> [email protected]
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=05%7C01%7Croid%40nvidia.com%7C31016034e21d45b8b36208dac7ddb85f%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638042052352712190%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=%2B8uJgVXgHPzfANmZburHoZ2G9fyfIfxEF4qvwM0r3gw%3D&reserved=0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
