[PATCH 1/3] waf_generator: Copy headers if necessary.

[Chris Johns]

On 10/08/2017 15:34, Christian Mauderer wrote:
> Am 10.08.2017 um 01:08 schrieb Chris Johns:
>> On 10/08/2017 02:29, Sichen Zhao wrote:
>>> From: Christian Mauderer <christian.mauderer at embedded-brains.de>
> There was no real reason. I just chose an arbitrary name.
> 'build-include' is fine for me too. I would avoid just 'include' because
> it's a very generic name. If it is added somewhere else, that might lead
> to a conflict.

Ah yes, 'build-include' is fine.

>>> +        start_dir = bld.path.find_dir(headers[0])
>>> +        for header in start_dir.ant_glob("**/" + headers[1]):
>>
>> Do we always want to copy all the files?
>>
> 
> What do you mean by "all the files"? As far as I understood, only the
> files that are changed should be copied by waf.
> 
> Or do you mean the ant_glob? In that case: I modeled that after the
> install target of the headers. 

Should this be an ant_glob or a list? I am wondering about needing to copy a
selected number of files from a group a glob would catch. If this is not
happening that is fine, it can be added if the situation arises.

> I already suggested in the first
> discussion regarding the patch that we move the "**" to builder.py
> instead. I planed to create a extra patch for that.

I had forgotten and thanks.

> 
>>> +            relsourcepath = os.path.relpath(str(header), start=str(start_dir))
>>
>> Is 'str(header)' really 'header.abspath()' ? See
>> https://waf.io/apidocs/Node.html#waflib.Node.Node.__str__.
>>
>> I prefer the explicit use of .abspath() than the conversion operator.
> 
> You are right. I didn't take a thorough look at the documentation. I'll
> rework that part. Maybe I can also use something like "path_from()" or
> some other waf Node function.
> 

Nice.

>>
>>> +            targetheader = os.path.join(target, relsourcepath)
>>> +            bld(features = 'subst',
>>> +                target = targetheader,
>>> +                source = header,
>>> +                is_copy = True)
>>
>> Does a clean remove these files?
> 
> Just checked: Yes it does.
> 

Excellent.

>>
>>> +
>>>      # KVM Symbols
>>>      bld(target = "rtemsbsd/rtems/rtems-kernel-kvm-symbols.c",
>>>          source = "rtemsbsd/rtems/generate_kvm_symbols",
>>> diff --git a/waf_generator.py b/waf_generator.py
>>> index 35fe35f..fb52250 100755
>>> --- a/waf_generator.py
>>> +++ b/waf_generator.py
>>> @@ -445,6 +445,29 @@ class ModuleManager(builder.ModuleManager):
>>>          self.add('')
>>>  
>>>          #
>>> +        # Add a copy rule for all headers where the install path and the source
>>> +        # path are not the same.
>>> +        #
>>> +        self.add('    # copy headers if necessary')
>>> +        headerPaths = builder.headerPaths()
>>
>> Can we remove this line and then ....
>>
>>> +        self.add('    header_build_copy_paths = [')
>>> +        for hp in headerPaths:
>>
>> .... use:
>>
>>            for hp in builder.headerPaths():
>> ?
> 
> Should work. I'll try it. Maybe it should be changed on the install part
> too?

Sure.

>>> +            if hp[2] != '' and not hp[0].endswith(hp[2]):
>>
>> I am ok with another boolean field being add to tuples rather than needing to
>> encode this some how if that is useful.
> 
> I would disagree here. It would be a redundant information in that tuple
> and therefore allow a invalid line if that boolean is not correctly set.
> Copying file is only necessary in exactly that case: If the dest paths
> name is not the same as the local path.

OK.

>> If a new field is not added can you please update builder.py with this rule so
>> we know what to do when adding headers to builder.headerPaths()?
> 
> You mean some comment that describes that behavior? I can add that also
> I don't really think that it is necessary. It's just a workaround so
> that we can build the binaries without having to install the headers
> before the build.

It took me a while to see what was happening. The table in builder.py is a long
way from here so even a note would help.

>> Do any of these files copied into the build tree need to be installed so users
>> can access then? I cannot see an install component.
> 
> The install component is unchanged. It should still work exactly like
> before. So all headers should be installed correctly just like before.

Great and thanks for this change.

Chris