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

[Adrian Moreno]

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.

> 
> Regards,
> Dumitru
> 
> 

-- 
Adrián Moreno