fastcdr is now required if security is on

[mikaelarguedas]

Fast-RTPS now requires fastcdr to compile the security plugins https://github.com/eProsima/Fast-RTPS/blob/8d89897fc9ec3f7e49057bfb2fbaa0d10b7abcf4/src/cpp/security/cryptography/AESGCMGMAC_Transform.h#L24

So it needs to be find_package with the "REQUIRED" option if SECURITY is passed to cmake.

Without this patch Fast-RTPS cmake prints -- fastcdr library not found... and then fails at compilation time:

In file included from /tmp/fastrtps_ws/src/eProsima/Fast-RTPS/src/cpp/rtps/security/../../security/cryptography/AESGCMGMAC.h:27:0,
                 from /tmp/fastrtps_ws/src/eProsima/Fast-RTPS/src/cpp/rtps/security/SecurityPluginFactory.cpp:22:
/tmp/fastrtps_ws/src/eProsima/Fast-RTPS/src/cpp/rtps/security/../../security/cryptography/AESGCMGMAC_Transform.h:24:25: fatal error: fastcdr/Cdr.h: No such file or directory

[richiware]

@mikaelarguedas Sorry, I've rebased internally feature/accesscontrol branch. Can you apply your change in the new commit tree? Thanks

[richiware]

I have a question about ament. Is it able to read the CMakeList.txt files and find the find_package instructions?

Currently we use a special macro eprosima_find_package to find FastCDR. Internally it makes a find_package(fastcdr). I've tried to add here the REQUIRED option, but ament is still failing when I compile using --parallel.

However, ament works with your PR. Do you know the reason? Thanks

[mikaelarguedas]

Currently we use a special macro eprosima_find_package to find FastCDR. Internally it makes a find_package(fastcdr). I've tried to add here the REQUIRED option, but ament is still failing when I compile using --parallel.

However, ament works with your PR. Do you know the reason? Thanks

TL;DR

ament just looks for find_package(<PKG_NAME>) in the CMakeLists and doesnt know about custom eprosima macros. So it works on this branch because I call find_package(fastcdr) directly but doesn't work with find_eprosima_package(fastcdr)

In more details:

ament is searching for calls to the find_package CMake function and looks if it has a package in the workspace with the same name as the string litteral that is passed to find_package().
It is not aware of custom eprosima macros neither does it parse / interpret CMake code, so cannot resolve that find_eprosima_package(fastcdr) will result in a call to find_package(fastcdr).

ament identifies the dependency successfully on this branch because I call the find_package function directly.

Notes for the future:

We are in the process of switching our infrastructure to a new build tool, that tool is checking for any function call "<ANY_PREFIX>find_package" so this new tool could recognize eprosima_find_package and extract the dependency.

But it will still not work in this case because your macro not called eprosima_find_package but find_eprosima_package.
We can create a plugin for the new tool to recognize find_eprosima_package as long as it takes the same arguments as the native find_package. (We have a prototype locally that does that).

But it would require additional work to be able to recognize other macros like find_eprosima_thirdparty as the arguments of the macro are not identical to those of find_package so it would need custom regular expressions

[dirk-thomas]

Just reading this kind of from the side line: as @mikaelarguedas mentioned we can write custom logic to detect your custom macro name. But I would argue that the naming is awkward. Let me try to explain why:

find_eprosima_package: I would read this name as" this function can only find eProsima specific packages". Looking at the implementation that doesn't seem to be the case.

eprosima_find_package: on the other hand reads to me like "an eprosima specific function finding (any) packages which does what the native CMake function does plus some eprosima specific magic and sugar"

Therefore I think the second name would make much more sense. Just my 2 cents but as we know naming and cache invalidation are the two most difficult parts of programming... 😉

[richiware]

For now i merge this PR. I'm agree with renaming our macros. When i finish the renaming i will warn you. Thanks

[dirk-thomas]

You can do the rename in a backwards compatible fashion. After renaming the macro introduce a macro with the old name and just call the new name from it passing all args.

Then existing code keeps working and you can update your code to call the new macro name.