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

Christian Mauderer christian.mauderer at embedded-brains.de
Thu Aug 10 05:34:35 UTC 2017


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 are some cases, where a header is installed into a directory with
>> a different name then it's source directory. In that case, the build
>> might fail because the header is not found. One example would be the
>> <openssl/opensslv.h>. The source for this file is in
>> freebsd/crypto/openssl/crypto/opensslv.h.
>>
>> To allow the build to work in such cases too, copy such files into a
>> temporary location in the build tree.
>> ---
>>  builder.py       |  1 +
>>  libbsd_waf.py    | 15 +++++++++++++++
>>  waf_generator.py | 23 +++++++++++++++++++++++
>>  3 files changed, 39 insertions(+)
>>
>> diff --git a/builder.py b/builder.py
>> index bb633cb..53802c7 100755
>> --- a/builder.py
>> +++ b/builder.py
>> @@ -172,6 +172,7 @@ def commonNoWarnings():
>>  
>>  def includes():
>>      return ['-Irtemsbsd/include',
>> +            '-Ilibbsd_build/include',
> 
> Is this path under the 'build' directory?

Yes.

> 
> Why the 2 directories in the path? Could the path simply be 'build-include' or
> even 'include' ? We know the context because we are looking under the build
> directory.
> 

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.

> I would prefer:
> 
> def buildInclude():
>       return 'libbsd_build/include'
> 
> The path can then be referenced in waf_generator.py where needed, for example:
> 
>    for i in builder.includes() + ['-I' + builder.buildInclude()]:
>       ....
> 

Agreed. I'll change that.

>>              '-Ifreebsd/sys',
>>              '-Ifreebsd/sys/contrib/pf',
>>              '-Ifreebsd/sys/net',
>> diff --git a/libbsd_waf.py b/libbsd_waf.py
>> index aee2e7a..1784f8b 100644
>> --- a/libbsd_waf.py
>> +++ b/libbsd_waf.py
>> @@ -62,6 +62,7 @@ def build(bld):
>>          for i in ['-Irtemsbsd/@CPU@/include', '-Ifreebsd/sys/@CPU@/include']:
>>              includes += ["%s" % (i[2:].replace("@CPU@", "x86"))]
>>      includes += ["rtemsbsd/include"]
>> +    includes += ["libbsd_build/include"]
>>      includes += ["freebsd/sys"]
>>      includes += ["freebsd/sys/contrib/pf"]
>>      includes += ["freebsd/sys/net"]
>> @@ -123,6 +124,20 @@ def build(bld):
>>          rule = "sed -e 's/@NET_CFG_SELF_IP@/%s/' -e 's/@NET_CFG_NETMASK@/%s/' -e 's/@NET_CFG_PEER_IP@/%s/' -e 's/@NET_CFG_GATEWAY_IP@/%s/' < ${SRC} > ${TGT}" % (net_cfg_self_ip, net_cfg_netmask, net_cfg_peer_ip, net_cfg_gateway_ip),
>>          update_outputs = True)
>>  
>> +    # copy headers if necessary
>> +    header_build_copy_paths = [
>> +                              ]
>> +    for headers in header_build_copy_paths:
>> +        target = os.path.join("libbsd_build/include", headers[2])
>> +        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. 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.

>> +            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.

> 
>> +            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.

> 
>> +
>>      # 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?

> 
>> +            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.

> 
> 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.

> 
>> +                self.add('                               %s,' % (str(hp)))
>> +        self.add('                              ]')
>> +        self.add('    for headers in header_build_copy_paths:')
>> +        self.add('        target = os.path.join("libbsd_build/include", headers[2])')
> 
> Use builder.buildInclude() rather than hard coding the path.
> 

OK.

>> +        self.add('        start_dir = bld.path.find_dir(headers[0])')
>> +        self.add('        for header in start_dir.ant_glob("**/" + headers[1]):')
>> +        self.add('            relsourcepath = os.path.relpath(str(header), start=str(start_dir))')
>> +        self.add('            targetheader = os.path.join(target, relsourcepath)')
>> +        self.add('            bld(features = \'subst\',')
>> +        self.add('                target = targetheader,')
>> +        self.add('                source = header,')
>> +        self.add('                is_copy = True)')
>> +        self.add('')
>> +
>> +        #
>>          # Add the specific rule based builders for generating files.
>>          #
>>          if 'KVMSymbols' in data:
>>
> 
> 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.

Kind regards

Christian

-- 
--------------------------------------------
embedded brains GmbH
Christian Mauderer
Dornierstr. 4
D-82178 Puchheim
Germany
email: christian.mauderer at embedded-brains.de
Phone: +49-89-18 94 741 - 18
Fax:   +49-89-18 94 741 - 08
PGP: Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.



More information about the devel mailing list