[ovs-dev] [PATCH ovn v1 1/5] manpages.mk: fix dependencies path
Numan Siddique
numans at ovn.org
Tue Nov 9 21:08:25 UTC 2021
On Mon, Nov 8, 2021 at 6:55 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 11/8/21 12:45 PM, Adrian Moreno wrote:
> >
> >
> > On 11/5/21 17:12, Dumitru Ceara wrote:
> >> On 10/21/21 11:10 AM, Adrian Moreno wrote:
> >>> Currently, if "make" is run after the project is built, the root
> >>> manpage (ovn-detrace.1) is rebuilt unnecessarily.
> >>>
> >>> The reason is that its dependencies are wrong: files such as
> >>> lib/common.man or lib/ovs.tmac do not exist in the project's root
> >>> path so "make" will constantly rebuild the manpage target. Instead,
> >>> those
> >>> dependencies live in $ovs_src.
> >>>
> >>> Modify sodepends.py to generate a makefile that contains the variable
> >>> names that point the right paths.
> >>>
> >>> Signed-off-by: Adrian Moreno <amorenoz at redhat.com>
> >>> ---
> >>
> >> Hi Adrian,
> >>
> >> I confirm this fixes the issue. I have some questions though.
> >>
> >>> Makefile.am | 2 +-
> >>> build-aux/sodepends.py | 45 ++++++++++++++++++++++++++++++++++++++----
> >>> manpages.mk | 12 +++++------
> >>> 3 files changed, 48 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/Makefile.am b/Makefile.am
> >>> index 0169c96ef..3b0df8393 100644
> >>> --- a/Makefile.am
> >>> +++ b/Makefile.am
> >>> @@ -425,7 +425,7 @@ CLEANFILES += flake8-check
> >>> include $(srcdir)/manpages.mk
> >>> $(srcdir)/manpages.mk: $(MAN_ROOTS) build-aux/sodepends.py
> >>> $(OVS_SRCDIR)/python/build/soutil.py
> >>> -
> >>> @PYTHONPATH=$(OVS_SRCDIR)/python$(psep)$$PYTHONPATH$(psep)$(srcdir)/python
> >>> $(PYTHON3) $(srcdir)/build-aux/sodepends.py -I. -I$(srcdir)
> >>> -I$(OVS_MANDIR) $(MAN_ROOTS) >$(@F).tmp
> >>> +
> >>> @PYTHONPATH=$(OVS_SRCDIR)/python$(psep)$$PYTHONPATH$(psep)$(srcdir)/python
> >>> $(PYTHON3) $(srcdir)/build-aux/sodepends.py -I. -Isrcdir,$(srcdir)
> >>> -IOVS_MANDIR,$(OVS_MANDIR) $(MAN_ROOTS) >$(@F).tmp
> >>> @if cmp -s $(@F).tmp $@; then \
> >>> touch $@; \
> >>> rm -f $(@F).tmp; \
> >>> diff --git a/build-aux/sodepends.py b/build-aux/sodepends.py
> >>> index 45812bcbd..343fda1af 100755
> >>> --- a/build-aux/sodepends.py
> >>> +++ b/build-aux/sodepends.py
> >>> @@ -16,9 +16,44 @@
> >>> from build import soutil
> >>> import sys
> >>> +import getopt
> >>> +import os
> >>> -def sodepends(include_dirs, filenames, dst):
> >>> +def parse_include_dirs():
> >>> + include_dirs = []
> >>> + options, args = getopt.gnu_getopt(sys.argv[1:], 'I:', ['include='])
> >>> + for key, value in options:
> >>> + if key in ['-I', '--include']:
> >>> + include_dirs.append(value.split(','))
> >>> + else:
> >>> + assert False
> >>> +
> >>> + include_dirs.append(['.'])
> >>> + return include_dirs, args
> >>
> >> Why don't we use soutil.parse_include_dirs() directly and add the
> >> 'value.split(,)' there?
> >> >> +
> >>> +
> >>> +def find_include_file(include_info, name):
> >>> + for info in include_info:
> >>> + if len(info) == 2:
> >>> + dir = info[1]
> >>> + var = "$(%s)/" % info[0]
> >>> + else:
> >>> + dir = info[0]
> >>> + var = ""
> >>> +
> >>> + file = "%s/%s" % (dir, name)
> >>> + try:
> >>> + os.stat(file)
> >>> + return var + name
> >>> + except OSError:
> >>> + pass
> >>> + sys.stderr.write("%s not found in: %s\n" %
> >>> + (name, ' '.join(str(include_info))))
> >>> + return None
> >>> +
> >>
> >> Shouldn't this go to soutil.py in OVS instead?
> >>
> >>> +
> >>> +def sodepends(include_info, filenames, dst):
> >>> ok = True
> >>> print("# Generated automatically -- do not modify! "
> >>> "-*- buffer-read-only: t -*-")
> >>> @@ -28,6 +63,7 @@ def sodepends(include_dirs, filenames, dst):
> >>> continue
> >>> # Open file.
> >>> + include_dirs = [info[0] for info in include_info]
> >>> fn = soutil.find_file(include_dirs, toplevel)
> >>> if not fn:
> >>> ok = False
> >>> @@ -47,8 +83,9 @@ def sodepends(include_dirs, filenames, dst):
> >>> name = soutil.extract_include_directive(line)
> >>> if name:
> >>> - if soutil.find_file(include_dirs, name):
> >>> - dependencies.append(name)
> >>> + include_file = find_include_file(include_info, name)
> >>> + if include_file:
> >>> + dependencies.append(include_file)
> >>> else:
> >>> ok = False
> >>> @@ -62,6 +99,6 @@ def sodepends(include_dirs, filenames, dst):
> >>> if __name__ == '__main__':
> >>> - include_dirs, args = soutil.parse_include_dirs()
> >>> + include_dirs, args = parse_include_dirs()
> >>> error = not sodepends(include_dirs, args, sys.stdout)
> >>> sys.exit(1 if error else 0)
> >>
> >> +Numan
> >>
> >> In general, I'm not completely sure about why sodepends.py needs to be
> >> part of OVN and why we can't use the OVS version?
> >
> > OVS does not have this problem as everything is under the root tree so I
> > assumed it would be preferred to implement it in OVN rather than add
> > stuff to OVS that will not be used by OVS.
> > But I don't have a strong opinion on this. If you prefer to implement
> > this in OVS and use it from OVN, I'll remove this patch from the series,
> > send it to OVS and send a patch to OVN that uses it.
> >
>
> +Mark
>
> In any case, this doesn't really block the rest of the series, I think
> patches 2-5 can be applied already to main branch if maintainers agree.
Thanks Adrian for this patch series and Dumitru for the reviews.
I applied the patches 2-5 to the main branch.
For this patch
Acked-by: Numan Siddique <numans at ovn.org>
My 2 cents - I'm fine modifying the sodepends.py in OVN. I'd personally prefer
to move the OVS bits into OVN for man page generation so that we can change
the code (in future) if required as per OVN's requirements.
Thanks
Numan
>
> Thanks!
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list