| Message ID | 20201111092530.136929-3-mark.d.gray@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Some fixes for OVS IPsec on Fedora | expand |
> ovs/dirs.py should be auto-generated using the template > ovs/dirs.py.template at build time. This will set the > ovs.dirs python variables with a value specified by the > environment or, if the environment variable is not set, from > the build system. > > Signed-off-by: Mark Gray <mark.d.gray@redhat.com> > --- > lib/automake.mk | 2 +- > python/automake.mk | 13 +++++++------ > python/ovs/.gitignore | 1 + > python/ovs/dirs.py | 31 ------------------------------- > 4 files changed, 9 insertions(+), 38 deletions(-) > delete mode 100644 python/ovs/dirs.py > > diff --git a/lib/automake.mk b/lib/automake.mk > index 380a672287ac..8eeb6c3f676c 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -575,7 +575,7 @@ MAN_FRAGMENTS += \ > OVSIDL_BUILT += lib/vswitch-idl.c lib/vswitch-idl.h lib/vswitch-idl.ovsidl > > EXTRA_DIST += lib/vswitch-idl.ann > -lib/vswitch-idl.ovsidl: vswitchd/vswitch.ovsschema lib/vswitch-idl.ann > +lib/vswitch-idl.ovsidl: vswitchd/vswitch.ovsschema lib/vswitch-idl.ann > python/ovs/dirs.py > $(AM_V_GEN)$(OVSDB_IDLC) annotate > $(srcdir)/vswitchd/vswitch.ovsschema $(srcdir)/lib/vswitch-idl.ann > $@.tmp && > mv $@.tmp $@ > > lib/dirs.c: lib/dirs.c.in Makefile > diff --git a/python/automake.mk b/python/automake.mk > index 2f08c7701484..c4382ec60928 100644 > --- a/python/automake.mk > +++ b/python/automake.mk > @@ -107,12 +107,13 @@ ALL_LOCAL += $(srcdir)/python/ovs/dirs.py > $(srcdir)/python/ovs/dirs.py: python/ovs/dirs.py.template > $(AM_V_GEN)sed \ > -e '/^##/d' \ > - -e 's,[@]pkgdatadir[@],/usr/local/share/openvswitch,g' \ > - -e 's,[@]RUNDIR[@],/var/run,g' \ > - -e 's,[@]LOGDIR[@],/usr/local/var/log,g' \ > - -e 's,[@]bindir[@],/usr/local/bin,g' \ > - -e 's,[@]sysconfdir[@],/usr/local/etc,g' \ > - -e 's,[@]DBDIR[@],/usr/local/etc/openvswitch,g' \ > + -e 's,[@]pkgdatadir[@],$(pkgdatadir),g' \ > + -e 's,[@]RUNDIR[@],$(RUNDIR),g' \ > + -e 's,[@]LOGDIR[@],$(LOGDIR),g' \ > + -e 's,[@]bindir[@],$(bindir),g' \ > + -e 's,[@]sysconfdir[@],$(sysconfdir),g' \ > + -e 's,[@]DBDIR[@],$(sysconfdir)/openvswitch,g' \ > < $? > $@.tmp && \ > mv $@.tmp $@ > EXTRA_DIST += python/ovs/dirs.py.template > +CLEANFILES += python/ovs/dirs.py > diff --git a/python/ovs/.gitignore b/python/ovs/.gitignore > index 98527864664d..51030beca437 100644 > --- a/python/ovs/.gitignore > +++ b/python/ovs/.gitignore > @@ -1 +1,2 @@ > version.py > +dir.py > diff --git a/python/ovs/dirs.py b/python/ovs/dirs.py > deleted file mode 100644 > index c67aecbb46da..000000000000 > --- a/python/ovs/dirs.py > +++ /dev/null > @@ -1,31 +0,0 @@ > -# Licensed under the Apache License, Version 2.0 (the "License"); > -# you may not use this file except in compliance with the License. > -# You may obtain a copy of the License at: > -# > -# http://www.apache.org/licenses/LICENSE-2.0 > -# > -# Unless required by applicable law or agreed to in writing, software > -# distributed under the License is distributed on an "AS IS" BASIS, > -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > implied. > -# See the License for the specific language governing permissions and > -# limitations under the License. > - > -# The @variables@ in this file are replaced by default directories for > -# use in python/ovs/dirs.py in the source directory and replaced by the > -# configured directories for use in the installed python/ovs/dirs.py. > -# > -import os > - > -# Note that the use of """ is to aid in dealing with paths with quotes in them. > -PKGDATADIR = os.environ.get("OVS_PKGDATADIR", > """/usr/local/share/openvswitch""") > -RUNDIR = os.environ.get("OVS_RUNDIR", """/var/run""") > -LOGDIR = os.environ.get("OVS_LOGDIR", """/usr/local/var/log""") > -BINDIR = os.environ.get("OVS_BINDIR", """/usr/local/bin""") > - > -DBDIR = os.environ.get("OVS_DBDIR") > -if not DBDIR: > - sysconfdir = os.environ.get("OVS_SYSCONFDIR") > - if sysconfdir: > - DBDIR = "%s/openvswitch" % sysconfdir > - else: > - DBDIR = """/usr/local/etc/openvswitch""" > -- Hi Mark, LGTM, I'm aware Timothy requested the v3 changes so will hold on for him to provide ack as well. Regards Ian > 2.26.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wed, 11 Nov 2020 04:25:30 -0500 Mark Gray <mark.d.gray@redhat.com> wrote: > ovs/dirs.py should be auto-generated using the template > ovs/dirs.py.template at build time. This will set the > ovs.dirs python variables with a value specified by the > environment or, if the environment variable is not set, from > the build system. > > Signed-off-by: Mark Gray <mark.d.gray@redhat.com> Acked-By: Timothy Redaelli <tredaelli@redhat.com>
> On Wed, 11 Nov 2020 04:25:30 -0500 > Mark Gray <mark.d.gray@redhat.com> wrote: > > > ovs/dirs.py should be auto-generated using the template > > ovs/dirs.py.template at build time. This will set the > > ovs.dirs python variables with a value specified by the > > environment or, if the environment variable is not set, from > > the build system. > > > > Signed-off-by: Mark Gray <mark.d.gray@redhat.com> > > Acked-By: Timothy Redaelli <tredaelli@redhat.com> > Thanks Timothy, I've pushed this series to master now. Regards Ian > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 11/16/20 5:28 PM, Stokes, Ian wrote: >> On Wed, 11 Nov 2020 04:25:30 -0500 >> Mark Gray <mark.d.gray@redhat.com> wrote: >> >>> ovs/dirs.py should be auto-generated using the template >>> ovs/dirs.py.template at build time. This will set the >>> ovs.dirs python variables with a value specified by the >>> environment or, if the environment variable is not set, from >>> the build system. >>> >>> Signed-off-by: Mark Gray <mark.d.gray@redhat.com> >> >> Acked-By: Timothy Redaelli <tredaelli@redhat.com> >> > > Thanks Timothy, I've pushed this series to master now. Thanks, Ian. But we might need a follow up on this change. I think, the main reason why dirs.py existed is that we need it while doing setup of python package, i.e. we should have it during execution of python/ovs/setup.py. Setup script already checks if version.py exists, and we might want check for dirs.py too, otherwise python package will be non-functional. Also, gitignore file seems to have dir.py and not dirs.py. Best regards, Ilya Maximets.
On 16/11/2020 16:33, Ilya Maximets wrote: > On 11/16/20 5:28 PM, Stokes, Ian wrote: >>> On Wed, 11 Nov 2020 04:25:30 -0500 >>> Mark Gray <mark.d.gray@redhat.com> wrote: >>> >>>> ovs/dirs.py should be auto-generated using the template >>>> ovs/dirs.py.template at build time. This will set the >>>> ovs.dirs python variables with a value specified by the >>>> environment or, if the environment variable is not set, from >>>> the build system. >>>> >>>> Signed-off-by: Mark Gray <mark.d.gray@redhat.com> >>> >>> Acked-By: Timothy Redaelli <tredaelli@redhat.com> >>> >> >> Thanks Timothy, I've pushed this series to master now. > > Thanks, Ian. But we might need a follow up on this change. > I think, the main reason why dirs.py existed is that we need > it while doing setup of python package, i.e. we should have > it during execution of python/ovs/setup.py. Setup script > already checks if version.py exists, and we might want check > for dirs.py too, otherwise python package will be non-functional. Sounds small enough. I will update this. > > Also, gitignore file seems to have dir.py and not dirs.py. Probably why it got checked in the first time. > > Best regards, Ilya Maximets. >
On 16/11/2020 16:33, Ilya Maximets wrote: > On 11/16/20 5:28 PM, Stokes, Ian wrote: >>> On Wed, 11 Nov 2020 04:25:30 -0500 >>> Mark Gray <mark.d.gray@redhat.com> wrote: >>> >>>> ovs/dirs.py should be auto-generated using the template >>>> ovs/dirs.py.template at build time. This will set the >>>> ovs.dirs python variables with a value specified by the >>>> environment or, if the environment variable is not set, from >>>> the build system. >>>> >>>> Signed-off-by: Mark Gray <mark.d.gray@redhat.com> >>> >>> Acked-By: Timothy Redaelli <tredaelli@redhat.com> >>> >> >> Thanks Timothy, I've pushed this series to master now. > > Thanks, Ian. But we might need a follow up on this change. > I think, the main reason why dirs.py existed is that we need > it while doing setup of python package, i.e. we should have > it during execution of python/ovs/setup.py. Setup script > already checks if version.py exists, and we might want check > for dirs.py too, otherwise python package will be non-functional. This means that dirs.py was never being called from setup.py but it was at least (always) there. Looking at ovs-monitor-ipsec which uses the python bindings, it does `import dirs.py` which means this may be how python applications are using it. Also, I think the automake system should ensure that dirs.py is there when packaging but its good to include this change I think. I submit a patch to fix this: https://patchwork.ozlabs.org/project/openvswitch/patch/20201116170026.331164-1-mark.d.gray@redhat.com/ > > Also, gitignore file seems to have dir.py and not dirs.py. > > Best regards, Ilya Maximets. >
On 11/16/20 6:11 PM, Mark Gray wrote: > On 16/11/2020 16:33, Ilya Maximets wrote: >> On 11/16/20 5:28 PM, Stokes, Ian wrote: >>>> On Wed, 11 Nov 2020 04:25:30 -0500 >>>> Mark Gray <mark.d.gray@redhat.com> wrote: >>>> >>>>> ovs/dirs.py should be auto-generated using the template >>>>> ovs/dirs.py.template at build time. This will set the >>>>> ovs.dirs python variables with a value specified by the >>>>> environment or, if the environment variable is not set, from >>>>> the build system. >>>>> >>>>> Signed-off-by: Mark Gray <mark.d.gray@redhat.com> >>>> >>>> Acked-By: Timothy Redaelli <tredaelli@redhat.com> >>>> >>> >>> Thanks Timothy, I've pushed this series to master now. >> >> Thanks, Ian. But we might need a follow up on this change. >> I think, the main reason why dirs.py existed is that we need >> it while doing setup of python package, i.e. we should have >> it during execution of python/ovs/setup.py. Setup script >> already checks if version.py exists, and we might want check >> for dirs.py too, otherwise python package will be non-functional. > > This means that dirs.py was never being called from setup.py but it was > at least (always) there. Looking at ovs-monitor-ipsec which uses the > python bindings, it does `import dirs.py` which means this may be how > python applications are using it. > > Also, I think the automake system should ensure that dirs.py is there > when packaging but its good to include this change I think. I submit a > patch to fix this: > > https://patchwork.ozlabs.org/project/openvswitch/patch/20201116170026.331164-1-mark.d.gray@redhat.com/ > > >> >> Also, gitignore file seems to have dir.py and not dirs.py. >> >> Best regards, Ilya Maximets. >> > One more important thing. This commit broke windows build: https://ci.appveyor.com/project/blp/ovs/builds/36338443/job/bjsq27m27kworj4e Please, check this out. Best regards, Ilya Maximets.
On 17/11/2020 00:27, Ilya Maximets wrote: > On 11/16/20 6:11 PM, Mark Gray wrote: >> On 16/11/2020 16:33, Ilya Maximets wrote: >>> On 11/16/20 5:28 PM, Stokes, Ian wrote: > One more important thing. > > This commit broke windows build: > https://ci.appveyor.com/project/blp/ovs/builds/36338443/job/bjsq27m27kworj4e > > Please, check this out. Thanks for letting me know. It looks like it cannot find the generated file when building. However, the command used to generate it is being executed (and presumably not failing as there is no error) as per the dependency in the 'automake.mk' file. As a quick test, I generated a 'dirs.py' file and pushed it to a private branch. The build will then not fail at that point. I also added an "ls" command into the build and the file is there with the correct permissions (644). As I don't have a Windows machine to test this on, hopefully Alin can help? Mark > > Best regards, Ilya Maximets. >
On 11/17/20 10:47 AM, Mark Gray wrote: > On 17/11/2020 00:27, Ilya Maximets wrote: >> On 11/16/20 6:11 PM, Mark Gray wrote: >>> On 16/11/2020 16:33, Ilya Maximets wrote: >>>> On 11/16/20 5:28 PM, Stokes, Ian wrote: > >> One more important thing. >> >> This commit broke windows build: >> https://ci.appveyor.com/project/blp/ovs/builds/36338443/job/bjsq27m27kworj4e >> >> Please, check this out. > > Thanks for letting me know. It looks like it cannot find the generated > file when building. However, the command used to generate it is being > executed (and presumably not failing as there is no error) as per the > dependency in the 'automake.mk' file. OVSIDL_BUILT targets depends on ovsdb/ovsdb-idlc.in, but ovsdb/ovsdb-idlc.in depends on nothing. The build is parallel, so we might just see output of the commands printed in some order while actually dirs.py generation happened at the same time with execution of ovsdb/ovsdb-idlc.in. I'm guessing, but that is very likely since we're running 'make -j4' and these are two first commands executed. In general, I think you need to make 'ovsdb/ovsdb-idlc.in' target depend on dirs.py target to make things work in correct order. We might also need to check other targets that might use dirs.py, but doesn't have build dependencies on it. It looks like a pure luck that linux build is not broken. > > As a quick test, I generated a 'dirs.py' file and pushed it to a private > branch. The build will then not fail at that point. I also added an "ls" > command into the build and the file is there with the correct > permissions (644). > > As I don't have a Windows machine to test this on, hopefully Alin can help? You could configure and use appveyor for your own github repo. This is not a great way to debug things, but at least something. > > Mark >> >> Best regards, Ilya Maximets. >> >
On 17/11/2020 11:35, Ilya Maximets wrote: > On 11/17/20 10:47 AM, Mark Gray wrote: >> On 17/11/2020 00:27, Ilya Maximets wrote: >>> On 11/16/20 6:11 PM, Mark Gray wrote: >>>> On 16/11/2020 16:33, Ilya Maximets wrote: >>>>> On 11/16/20 5:28 PM, Stokes, Ian wrote: >> >>> One more important thing. >>> >>> This commit broke windows build: >>> https://ci.appveyor.com/project/blp/ovs/builds/36338443/job/bjsq27m27kworj4e >>> >>> Please, check this out. >> >> Thanks for letting me know. It looks like it cannot find the generated >> file when building. However, the command used to generate it is being >> executed (and presumably not failing as there is no error) as per the >> dependency in the 'automake.mk' file. > > OVSIDL_BUILT targets depends on ovsdb/ovsdb-idlc.in, but ovsdb/ovsdb-idlc.in > depends on nothing. The build is parallel, so we might just see output of > the commands printed in some order while actually dirs.py generation happened > at the same time with execution of ovsdb/ovsdb-idlc.in. I'm guessing, but > that is very likely since we're running 'make -j4' and these are two first > commands executed. > In general, I think you need to make 'ovsdb/ovsdb-idlc.in' target depend on > dirs.py target to make things work in correct order. > We might also need to check other targets that might use dirs.py, but doesn't > have build dependencies on it. It looks like a pure luck that linux build > is not broken. That was it. Good catch Ilya. I submitted a v2 patch to resolve this issue and the other points you raised. I think the only dependency is with the IDL generation code. It passes travis *and* appveyor! https://patchwork.ozlabs.org/project/openvswitch/patch/20201117180212.433352-1-mark.d.gray@redhat.com/ > >> >> As a quick test, I generated a 'dirs.py' file and pushed it to a private >> branch. The build will then not fail at that point. I also added an "ls" >> command into the build and the file is there with the correct >> permissions (644). >> >> As I don't have a Windows machine to test this on, hopefully Alin can help? > > You could configure and use appveyor for your own github repo. This is not > a great way to debug things, but at least something. That was what I was doing. > >> >> Mark >>> >>> Best regards, Ilya Maximets. >>> >> >
diff --git a/lib/automake.mk b/lib/automake.mk index 380a672287ac..8eeb6c3f676c 100644 --- a/lib/automake.mk +++ b/lib/automake.mk @@ -575,7 +575,7 @@ MAN_FRAGMENTS += \ OVSIDL_BUILT += lib/vswitch-idl.c lib/vswitch-idl.h lib/vswitch-idl.ovsidl EXTRA_DIST += lib/vswitch-idl.ann -lib/vswitch-idl.ovsidl: vswitchd/vswitch.ovsschema lib/vswitch-idl.ann +lib/vswitch-idl.ovsidl: vswitchd/vswitch.ovsschema lib/vswitch-idl.ann python/ovs/dirs.py $(AM_V_GEN)$(OVSDB_IDLC) annotate $(srcdir)/vswitchd/vswitch.ovsschema $(srcdir)/lib/vswitch-idl.ann > $@.tmp && mv $@.tmp $@ lib/dirs.c: lib/dirs.c.in Makefile diff --git a/python/automake.mk b/python/automake.mk index 2f08c7701484..c4382ec60928 100644 --- a/python/automake.mk +++ b/python/automake.mk @@ -107,12 +107,13 @@ ALL_LOCAL += $(srcdir)/python/ovs/dirs.py $(srcdir)/python/ovs/dirs.py: python/ovs/dirs.py.template $(AM_V_GEN)sed \ -e '/^##/d' \ - -e 's,[@]pkgdatadir[@],/usr/local/share/openvswitch,g' \ - -e 's,[@]RUNDIR[@],/var/run,g' \ - -e 's,[@]LOGDIR[@],/usr/local/var/log,g' \ - -e 's,[@]bindir[@],/usr/local/bin,g' \ - -e 's,[@]sysconfdir[@],/usr/local/etc,g' \ - -e 's,[@]DBDIR[@],/usr/local/etc/openvswitch,g' \ + -e 's,[@]pkgdatadir[@],$(pkgdatadir),g' \ + -e 's,[@]RUNDIR[@],$(RUNDIR),g' \ + -e 's,[@]LOGDIR[@],$(LOGDIR),g' \ + -e 's,[@]bindir[@],$(bindir),g' \ + -e 's,[@]sysconfdir[@],$(sysconfdir),g' \ + -e 's,[@]DBDIR[@],$(sysconfdir)/openvswitch,g' \ < $? > $@.tmp && \ mv $@.tmp $@ EXTRA_DIST += python/ovs/dirs.py.template +CLEANFILES += python/ovs/dirs.py diff --git a/python/ovs/.gitignore b/python/ovs/.gitignore index 98527864664d..51030beca437 100644 --- a/python/ovs/.gitignore +++ b/python/ovs/.gitignore @@ -1 +1,2 @@ version.py +dir.py diff --git a/python/ovs/dirs.py b/python/ovs/dirs.py deleted file mode 100644 index c67aecbb46da..000000000000 --- a/python/ovs/dirs.py +++ /dev/null @@ -1,31 +0,0 @@ -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at: -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -# The @variables@ in this file are replaced by default directories for -# use in python/ovs/dirs.py in the source directory and replaced by the -# configured directories for use in the installed python/ovs/dirs.py. -# -import os - -# Note that the use of """ is to aid in dealing with paths with quotes in them. -PKGDATADIR = os.environ.get("OVS_PKGDATADIR", """/usr/local/share/openvswitch""") -RUNDIR = os.environ.get("OVS_RUNDIR", """/var/run""") -LOGDIR = os.environ.get("OVS_LOGDIR", """/usr/local/var/log""") -BINDIR = os.environ.get("OVS_BINDIR", """/usr/local/bin""") - -DBDIR = os.environ.get("OVS_DBDIR") -if not DBDIR: - sysconfdir = os.environ.get("OVS_SYSCONFDIR") - if sysconfdir: - DBDIR = "%s/openvswitch" % sysconfdir - else: - DBDIR = """/usr/local/etc/openvswitch"""
ovs/dirs.py should be auto-generated using the template ovs/dirs.py.template at build time. This will set the ovs.dirs python variables with a value specified by the environment or, if the environment variable is not set, from the build system. Signed-off-by: Mark Gray <mark.d.gray@redhat.com> --- lib/automake.mk | 2 +- python/automake.mk | 13 +++++++------ python/ovs/.gitignore | 1 + python/ovs/dirs.py | 31 ------------------------------- 4 files changed, 9 insertions(+), 38 deletions(-) delete mode 100644 python/ovs/dirs.py