On Wed, Nov 16, 2022 at 3:43 PM Roi Dayan via dev <[email protected]>
wrote:
>
>
> 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.
>
I don't quite agree with this. Speaking as one of the Open vSwitch and OVN
package maintainers in Ubuntu, we seek to keep the packaging source in
upstream repositories as up to date and as close to a match as possible to
what we'd see trickling into our distribution. The current state of the
Open vSwitch upstream packaging source is that it's nearly identical to
what is currently in tip of Debian and Ubuntu. Building the packaging
source is also part of the upstream Open vSwitch gate.
> So what's wrong with upstream repository having it's own default
> with -Werror ?
>
Furthering what Ilya has already pointed out, the distributions update
their toolchain to stay on the edge of development, and there will always
be a gap between when when the Open vSwitch community gets around to
squelching the next wave of warnings introduced by compiler updates and the
general availability of said compiler updates in the distribution.
The upstream gate currently builds and gates with -Werror, which is of
utmost importance, and also OK because we control when we switch the base
image to the next generation.
The packaging sources on the other hand will be put into new toolchain
environments at a cadence that is independent of the upstream project, and
I don't think it's fair to impose the entire burden of maintenance of
toolchain related updates to the Open vSwitch source on the distributions.
> Changing the spec/debian here should not and probably is not affecting
> a distribution release version.
>
See above.
--
Frode Nordahl
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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
