Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

orogen_include for /std/vector</base/JointLimitRange> and friends #12

Closed
marvin2k opened this issue Sep 16, 2014 · 9 comments
Closed

orogen_include for /std/vector</base/JointLimitRange> and friends #12

marvin2k opened this issue Sep 16, 2014 · 9 comments

Comments

@marvin2k
Copy link
Contributor

This time asking about orogen_include... Running make -C build regen in base/orogen/types generates a .orogen/typekit/base.tlb. Inside this file there is an entry

<container name="/std/vector&lt;/base/JointLimitRange&gt;" of="/base/JointLimitRange" size="24" kind="/std/vector">
    <metadata key="orogen_defining_typekits"><![CDATA[base]]></metadata>
    <metadata key="orogen_exporting_typekits"><![CDATA[base]]></metadata>
    <metadata key="orogen_include"><![CDATA[base:base/Temperature.hpp]]></metadata>
    <metadata key="source_file_line"><![CDATA[/usr/include/c++/4.9/bits/stl_vector.h:214]]></metadata>
</container>

So, it contains an orogen_include for Temperature.hpp despite transporting a JointLimitRange -- not as expected. This is the same for all the "vector" related types. Tried to track it down, currently I would opt for something in to go wrong during parsing of the preprocessed gccxml output?

This is no issue for me, just stumpled upon it trying to integrate the clang-thingy. And now I use this to introduce my actual problem: Why is this arcane orogen_include needed at all? Every well-behaved type has a source_file_line which is exactly what is to be included (raise an error if is not a header). For containers recurse for the type in of, for opaques the one in marshal_as?

@doudou
Copy link
Contributor

doudou commented Sep 16, 2014

Definitely a bug.

However, source_file_line is not what is needed. What is needed at build and generation times are the name of the pkg-config project that provides the file as well as the include that should be used. The metadata that could be thrown away happily is actually source_file_line

@marvin2k
Copy link
Contributor Author

Ok, pkg-config -- fine.

But the file to be included to use the type in question -- is the file where the type itself is declared. Which is the where source_file_line points to?

@doudou
Copy link
Contributor

doudou commented Sep 16, 2014

Yes, but not really. First, it is made relative based on the CFLAGS of the pkg-config in orogen_include. Moreover, it is resolved to the toplevel include (the file priovided to #import_types_from), not the exact include, as for instance the header defining Eigen::Matrix cannot be included directly while Eigen/Core can.

@marvin2k
Copy link
Contributor Author

Hm. Have to think about this. Rationale: gccxml is called to "preprocess" the headers into one big file. There, some logic does the mapping which include-file should be used... And gccxml is bound to go.

@marvin2k
Copy link
Contributor Author

  • what do you mean with relative based on the CFLAGS of the pkg-config in orogen_include?
  • jeah, its converted from an absolute path to some relative path -- So that the compiler can look it up again during parsing of the template-derived code?
  • Example-Time: Eigen::Vector3d has an opaque and is used in a header. The generated tlb contains source_file_line = /usr/include/eigen3/Eigen/src/Core/Matrix.h:373. A manually created main.cpp like
#include "/usr/include/eigen3/Eigen/src/Core/Matrix.h"

int main(int argc, char *argv[])
{
    Eigen::Vector3d test;
    return 0;
}

compiles just fine... Where is my misunderstanding?

The clang-derived importer could provide everything which can be extracted from the cpp -- and orogen can only do worse.

@doudou
Copy link
Contributor

doudou commented Sep 23, 2014

Because you do not KNOW whether it can be or not. The typical case is C++ standard headers, they are split in tons of small headers that are NOT meant to be included. Only the standard headers (like ) are. And there's really nothing clang can help you with there.

The only headers you KNOW can be included directly are the ones that have been explicitly given to orogen. Matrix.h ? Not part of eigen's documentation ... This is not a header you are meant to include directly. That it works is just by chance, not by design.

As for the CFLAGS: the problem is to generate code that can be relocated / generated on one machine and compiled on another. The resolution basically takes the absolute path and uses the package's pkg-config -I options to find out what is the most likely #include<> line and the corresponding CMake code.

@marvin2k
Copy link
Contributor Author

Ok, got the point. But clang can help us here, like so for example. For me this feels more sane than orogen calling gcc(xml) again just to resolve this include-chain.

As for the CFLAGS: the problem is to generate code that can be relocated / generated on one machine and compiled on another. The resolution basically takes the absolute path and uses the package's pkg-config -I options to find out what is the most likely #include<> line and the corresponding CMake code

chuckle Did someone ever try to move the generated code from one machine to another? I very much doubt that this will reliably (read: reliably) work. So much interpreted magic eating and spitting options and files here and there... Installing stuff and not installing stuff, environment here and there... Using the just the full path in the generated code would prevent the later compiler from accidentally looking up the wrong header.


So what about the importer setting a metadata "full_include_path", and orogen then churning on this full path to create the "orogen_include" with a hopefully correct relative path given some pkg-config flags?

@doudou
Copy link
Contributor

doudou commented Sep 25, 2014

Ok, got the point. But clang can help us here, like so for example. For me this feels more sane than orogen calling gcc(xml) again just to resolve this include-chain

Streamlining the process is of course a good thing. Now, it does not change the fact that it needs to be done one way or another...

You don't need to sell clang to me ... I am in love with it already (and will happily sign the death sentence to the gccxml importer).

So what about the importer setting a metadata "full_include_path", and orogen then churning on this full path to create the "orogen_include" with a hopefully correct relative path given some pkg-config flags?

This is exactly what currently happens, with the importer setting source_file_line and orogen "massaging" it to the orogen_include

As to the portability: if the code was written manually, this is exactly the approach one would take: add the dependency on the pkg-config file, include the header from the pkg-config-resolved library. Might not be perfect, but - in general - following the way everyone does it in practice sounds like a pretty good idea to me.

@marvin2k
Copy link
Contributor Author

You don't need to sell clang to me ... I am in love with it already (and will happily sign the death sentence to the gccxml importer).

Yes. Sometimes I need more control over my moment of inertia... ;-)

doudou added a commit that referenced this issue Oct 13, 2014
The containers' source_file_line points to the template's
definition, which is then resolved to whichever header
included it first. This commit adds a special case to ensure
that the metadata is properly set

Note that it does not remove the corresponding hardcoded include values
in include_for_type as it opens a can of worms during opaque generation.
In this phase, we create our own types (in particular vectors-of-opaques)
for which the normal load mechanisms do not apply - and therefore for
which the orogen_include metadata is set *after* the current generation
pass needed it:

  - create m-types < this requires the orogen_include info
  - load m-types < this creates the orogen_include info

It can be fixed, but requires quite a bit of changes, so delay that
until we manage to make the Typekit class something looking less horrid
@doudou doudou closed this as completed in 05584ea Oct 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants