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

[Dumitru Ceara]

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!