Make $<INSTALL_INTERFACE... use INCLUDE_INSTALL_DIR, and install headers to include/${PROJECT_NAME}

[sloretz]

Same as eProsima/Fast-CDR#120

The first commit is a bug fix. The exported targets currently always say the include directory is include/ regardless of INCLUDE_INSTALL_DIR, so this fixes it by using ${INCLUDE_INSTALL_DIR} instead.

The second commit is an enhancement supporting ros2/ros2#1150

[EduPonz]

Hi @sloretz ,

First of all, I'm using this answer to respond both to this PR and to eProsima/Fast-CDR#120.

I'm afraid these change cannot be merged as they are right now, since it will break Fast DDS applications that do not use CMake to build. I'll give you and example:

Fast DDS Link Program

Consider the following application

#include <iostream>

#include <fastcdr/FastBuffer.h>
#include <fastdds/dds/domain/DomainParticipantFactory.hpp>
#include <fastdds/dds/domain/DomainParticipant.hpp>

int main()
{
    /* Check that Fast CDR can be used */
    eprosima::fastcdr::FastBuffer fastbuffer;
    std::cout << "FastBuffer created!" << std::endl;

    /* Check that Fast DDS can be used */
    eprosima::fastdds::dds::DomainParticipant* participant =
            eprosima::fastdds::dds::DomainParticipantFactory::get_instance()->create_participant(0,
                    eprosima::fastdds::dds::PARTICIPANT_QOS_DEFAULT);
    std::cout << "DomainParticipant created!" << std::endl;

    return 0;
}

Then, I use this Dockerfile to test building it with g++ using Fast CDR and Fast DDS master branches, and also with your branches.

FROM ubuntu:20.04

ARG fastcdr_repo=https://github.com/eProsima/Fast-CDR.git
ARG fastcdr_version=master
ARG fastdds_repo=https://github.com/eProsima/Fast-DDS.git
ARG fastdds_version=master

# Needed for a dependency that forces to set timezone
ENV TZ=Europe/Madrid
RUN ln -snf /usr/share/zoneinfo/$TZ /etc/localtime && echo $TZ > /etc/timezone

SHELL ["/bin/bash", "-c"]

WORKDIR /fastdds_ws

# Install build and run dependencies
RUN apt update && \
    apt install --yes --no-install-recommends \
        git \
        build-essential \
        cmake \
        libssl-dev \
        libasio-dev \
        libtinyxml2-dev \
        openjdk-8-jre-headless

# Install foonathan/memory
RUN git clone https://github.com/eProsima/foonathan_memory_vendor.git && \
    mkdir -p foonathan_memory_vendor/build && \
    cd foonathan_memory_vendor/build && \
    cmake .. -DCMAKE_INSTALL_PREFIX=/usr/local/ -DBUILD_SHARED_LIBS=ON && \
    cmake --build . --target install && \
    cd ../..

# Install Fast CDR
RUN git clone ${fastcdr_repo} fastcdr && \
    cd fastcdr && \
    git checkout ${fastcdr_version} && \
    mkdir -p build && \
    cd build && \
    cmake .. && \
    cmake --build . --target install && \
    cd ../..

# Install Fast DDS
RUN git clone ${fastdds_repo} fastdds && \
    cd fastdds && \
    git checkout ${fastdds_version} && \
    mkdir -p build && \
    cd build && \
    cmake .. && \
    cmake --build . --target install && \
    cd ../..

# Compile and run simple example
COPY test_fastdds_link.cpp .
RUN g++ test_fastdds_link.cpp -o test_fastdds_link -l fastcdr -l fastrtps -L /usr/local/lib -I /usr/local/include
RUN LD_LIBRARY_PATH=/usr/local/lib ./test_fastdds_link

Build master versions

The following command succeeds at building an running the test program:

docker build \
    -t fastdds:master \
    -f Dockerfile .

Build sloretz versions

The following command fails at building an running the test program:

docker build \
    --build-arg fastdds_repo=https://github.com/sloretz/Fast-RTPS.git \
    --build-arg fastdds_version=sloretz__Fast-DDS_include_project_name \
    --build-arg fastcdr_repo=https://github.com/sloretz/Fast-CDR.git \
    --build-arg fastcdr_version=sloretz__fastcdr__include_project_name \
    -t fastdds:sloretz \
    -f Dockerfile .

This means, that upgrading Fast DDS would break the compilation of such program, which is something that cannot happen unless increasing major version.

Solution

One option would be to only change the installation directory if certain new CMake option is passed when building the library.
Of course, the default behaviour must be what it's done now, only changing the installation if the user explicitly states so.
However, this has the drawback that ROS 2 package generation should be update to use said option.
Furthermore, compiling ROS 2 from sources would become more cumbersome, as users would need to pass that option to avoid colcon issues. What do you think?

[sloretz]

Thanks for the dockerfile, I made a few edits to make it build with these PRs.

Updated Dockerfile

FROM ubuntu:20.04

# Needed for a dependency that forces to set timezone
ENV TZ=Europe/Madrid
RUN ln -snf /usr/share/zoneinfo/$TZ /etc/localtime && echo $TZ > /etc/timezone

SHELL ["/bin/bash", "-c"]

WORKDIR /fastdds_ws

# Install build and run dependencies
RUN apt update && \
    apt install --yes --no-install-recommends \
        git \
        build-essential \
        cmake \
        libssl-dev \
        libasio-dev \
        libtinyxml2-dev \
        openjdk-8-jre-headless

ARG fastcdr_repo=https://github.com/eProsima/Fast-CDR.git
ARG fastcdr_version=master
ARG fastdds_repo=https://github.com/eProsima/Fast-DDS.git
ARG fastdds_version=master

# Install foonathan/memory
RUN git clone https://github.com/eProsima/foonathan_memory_vendor.git --depth 1 && \
    mkdir -p foonathan_memory_vendor/build && \
    cd foonathan_memory_vendor/build && \
    cmake .. -DCMAKE_INSTALL_PREFIX=/usr/local/ -DBUILD_SHARED_LIBS=ON && \
    cmake --build . --target install && \
    cd ../..

# Install Fast CDR
RUN git clone ${fastcdr_repo} fastcdr -b ${fastcdr_version} --depth 1 && \
    cd fastcdr && \
    mkdir -p build && \
    cd build && \
    cmake .. && \
    cmake --build . --target install --parallel 16 && \
    cd ../..

# Install Fast DDS
RUN git clone ${fastdds_repo} fastdds -b ${fastdds_version} && \
    cd fastdds && \
    mkdir -p build && \
    cd build && \
    cmake .. && \
    cmake --build . --target install --parallel 16 && \
    cd ../..

COPY test_fastdds_link.cpp .

# Compile simple example -- hardcoding the include directory to /usr/local/include
# RUN g++ test_fastdds_link.cpp -o test_fastdds_link -l fastcdr -l fastrtps -L /usr/local/lib -I /usr/local/include

# Compile simple example -- hardcoding the include directories to /usr/local/include/fastcdr and /usr/local/include/fastrtps
RUN g++ test_fastdds_link.cpp -o test_fastdds_link -l fastcdr -l fastrtps -L /usr/local/lib -I /usr/local/include/fastcdr -I /usr/local/include/fastrtps

# Run simple example
RUN LD_LIBRARY_PATH=/usr/local/lib ./test_fastdds_link

This means, that upgrading Fast DDS would break the compilation of such program, which is something that cannot happen unless increasing major version.

That's true, if someone hard-codes the include directory then this PR would break them. I agree this change would require bumping the major version. I think adding a pkg-config file would make this easier on non-CMake users in the future (See this example on CycloneDDS).

One option would be to only change the installation directory if certain new CMake option is passed when building the library.

Adding a CMake option makes sense. I'll add the same one I did for urdfdom to this PR: ros/urdfdom#167 APPEND_PROJECT_NAME_TO_INCLUDEDIR.

Furthermore, compiling ROS 2 from sources would become more cumbersome, as users would need to pass that option to avoid colcon issues. What do you think?

It becomes cumbersome in that the option would need to be passed, but that's doable. I think ROS 2 is already passing --cmake-args -DSECURITY=ON for Fast-DDS, so the same solution could be done here.

[sloretz]

@EduPonz I've updated this PR and eProsima/Fast-CDR#120 to have a CMake option APPEND_PROJECT_NAME_TO_INCLUDEDIR (matching ros/urdfdom#167) which defaults to OFF. When ON, ${PROJECT_NAME} is appended to INCLUDE_INSTALL_DIR.

[sloretz]

Friendly ping @MiguelBarro . Any thoughts on this PR now that eProsima/Fast-CDR#120 has been merged?

[EduPonz]

Hi @sloretz ,

Sorry for the late reply. Our Quality Declaration enforces that all new features must be documented, would you mind submitting a PR to Fast-DDS-docs? In particular, you'd need to document the new option here. In any case, we are not going to block the merging of these PR until the documentation is ready; once we get @MiguelBarro approval I think we are good to go.

I'd like to take the opportunity to ask how are you planning on activating the flag on ROS 2 builds, specially from sources. I think this would probably be a good place, although extending the ROS 2 installation guide would be required so that users update their colcon metadata. In fact, I think this step is already relevant without the feature in this PR, since right now there is a mismatch between building from sources or getting the binary installation (regarding security) that is dangerous when compiling Fast DDS in an overlay.

[MiguelBarro]

LGTM

[sloretz]

would you mind submitting a PR to Fast-DDS-docs?

Sure! eProsima/Fast-DDS-docs#355

I'd like to take the opportunity to ask how are you planning on activating the flag on ROS 2 builds, specially from sources. I think this would probably be a good place, although extending the ROS 2 installation guide would be required so that users update their colcon metadata. In fact, I think this step is already relevant without the feature in this PR, since right now there is a mismatch between building from sources or getting the binary installation (regarding security) that is dangerous when compiling Fast DDS in an overlay.

That's a good question, and great suggestions. I opened colcon/colcon-metadata-repository#7 to set the option for Fast-DDS and Fast-CDR. One idea is to patch the release repo to set the CMake option to cover the case when using Debian packages: ros2-gbp/fastcdr-release#9 . I also opened a PR to make the ROS 2 CI jobs set the option, which includes the packaging jobs: ros2/ci#629

[EduPonz]

Lots of thanks @sloretz for taking care of it so promptly!