[ovs-dev] [PATCH ovn v1 1/5] manpages.mk: fix dependencies path

[Dumitru Ceara]

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?

Regards,
Dumitru