Install mbedtls also when used as static library

[Oxymoron79]

Since the iotivity-lite headers include mbedtls headers, the mbedtls library needs to be installed also when it is statically linked. The CMake property EXCLUDE_FROM_ALL prevented that.

[ocf-conformance-test-tool]

🎉 Thank you for your code contribution! To guarantee the change/addition is conformant to the OCF Specification, we would like to ask you to execute OCF Conformance Testing of your change ☝️ when your work is ready to be reviewed.

---

ℹ️ To verify your latest change (fd024ad), label this PR with OCF Conformance Testing.

⚠️ Label is removed with every code change.

[Danielius1922]

@Oxymoron79 Isn't changing directory to build/mbedtls and calling make install there sufficient?

Though I don't recall any special reason why not to do it, I think I did it because I wanted to keep the make install commands consistent between Makefile (from port/linux) and CMake builds.

[Oxymoron79]

@Oxymoron79 Isn't changing directory to build/mbedtls and calling make install there sufficient?

1. It's very inconvenient if you want to package iotivity-lite (.deb, .rpm) or integrate into other build systems (Zephyr, Yocto). Because those work the easiest when the source complies to the usual three step build: configure, make, make install.
2. The current implementation is inconsistent in that regard: If USE_SHARED_MBEDTLS_LIBRARY is set, make install will also install mbedtls, if USE_SHARED_MBEDTLS_LIBRARY is not set, make install requires an additional installation step for mbedtls.
3. IMO, if a build system has compilation steps for a component (mbedtls in this case) that need to be installed, I expect the build system to properly install that component as well - mostly to guarantee that the same environment is used (same destination directory, compilation flags, etc.).

During investigations for this answer, I noticed that my initial statement that the "iotivity-lite headers include mbedtls headers" is not true. But instead I found that if iotivity-lite links mbedtls statically, the user of iotivity-lite needs to have the static library of mbedtls installed.

Though I don't recall any special reason why not to do it, I think I did it because I wanted to keep the make install commands consistent between Makefile (from port/linux) and CMake builds.

If I understand the Makefile correctly, it does not need a separate installation of mbedtls, since it includes the mbedtls sources into the iotivty-lite sources (it does not make use of the mbedtls build system as the CMake does now) and thus does not need to link to the mbedtls library.

So, this PR actually brings the CMake build closer to the Makefile build.

[Danielius1922]

@Oxymoron79 What's the use case? Maybe I'm missing it, because I only use installation on Linux...? Or do you want to use mbedTLS code from inside your binary?

Here's my understanding of our mbedTLS usage:
The exception for USE_SHARED_MBEDTLS_LIBRARY was a special case for a constrained device to be able to preinstall the patched mbedtls on the device and use it globally to save space.
However, given that we use a custom patched version of mbedTLS, I don't think we want to make it visible to the consumers of the iotivity-lite library by default. When USE_SHARED_MBEDTLS_LIBRARY is false then mbedtls targets are linked directly into the iotivity-lite libraries. The build compiles mbedtls and uses that, so an installed instance of mbedTLS shouldn't be needed during building and all mbedTLS symbols should be inside iotivity-lite libraries, so it shouldn't be needed in runtime either.
I see that mbedtls stuff is in security/oc_certs.h and security/oc_tls.h headers, but the security folder is not part of the installation, so that shouldn't be a problem.

[Oxymoron79]

@Oxymoron79 What's the use case? Maybe I'm missing it, because I only use installation on Linux...? Or do you want to use mbedTLS code from inside your binary?

@Danielius1922 We use the iotivity-lite library as a dependency of a C++ library which is used in applications that run on a wide range of platforms. So I package iotivity-lite for a wide variety of platforms: Linux with deb packages, embedded linux devices with Yocto (ARM & Intel architectures), Windows 10 and we are looking into adding microcontrollers using Zephyr.

The iotivity-lite version we currently use is an older one (2.2.5), where the CMake build of iotivity-lite builds mbedtls from the mbedtls source files. Now we want to update to a newer version of iotivity-lite where @jkralik was actually working on for us and I am reviewing his proposed changes. This is where I stumbled across the extra manual step to install the mbedtls library. For the wide variety of build systems and packaging I do, such an extra step usually turns out to be hard to maintain properly.

I see that mbedtls stuff is in security/oc_certs.h and security/oc_tls.h headers, but the security folder is not part of the installation, so that shouldn't be a problem.

Yes, that's true - I noticed this today as well: The mbedtls headers are not the reason that require that extra installation step. Sorry about this wrong statement of mine.

However...

When USE_SHARED_MBEDTLS_LIBRARY is false then mbedtls targets are linked directly into the iotivity-lite libraries. The build compiles mbedtls and uses that, so an installed instance of mbedTLS shouldn't be needed during building and all mbedTLS symbols should be inside iotivity-lite libraries, so it shouldn't be needed in runtime either.

This seems not to be the case with my test application: The application uses the shared library of iotivity-lite (which was built with USE_SHARED_MBEDTLS_LIBRARY set to false) and I got linker errors for all the mbedtls functions that iotivity-lite uses (the application does not use any mbedtls function directly). The linker errors were resolved by installing the static libraries of mbedtls.

And when I check the symbols in the iotivity-lite libraries with nm, it lists the mbedtls symbols as undefined (U):

$ nm libiotivity-lite-client-server-static.a | grep mbed
                 U mbedtls_ctr_drbg_free
...
$ nm -D libiotivity-lite-client-server.so | grep mbed
                 U mbedtls_ctr_drbg_free
...

So, it seems to me, that the installation of the mbedtls libraries is required, even if USE_SHARED_MBEDTLS_LIBRARY is false. Or am I missing something?

[Danielius1922]

@Oxymoron79 Ah sorry, it turns out that CMake doesn't work like I thought it did. When you link a static library with another static library (in our case iotivity-lite libs with mbedtls libs) it doesn't archive the dependency (mbedtls) into the library (iotivity-lite), but simply takes note of the depedency and propagates everything when you link a (non-library) target with your library.
There seems to be a solution - https://stackoverflow.com/questions/14199708/cmake-include-library-dependencies-in-a-static-library - but it's not really standard. :/ I'll talk with @jkralik about what to do with this tomorrow.

[Oxymoron79]

There seems to be a solution - https://stackoverflow.com/questions/14199708/cmake-include-library-dependencies-in-a-static-library - but it's not really standard.

@Danielius1922 The other solution would be to install the static mbedtls library 😉 That also seems to be the standard way. What would be the downside of that solution?

[Danielius1922]

@Danielius1922 The other solution would be to install the static mbedtls library 😉 That also seems to be the standard way. What would be the downside of that solution?

Yes indeed. We will install it, the only thing I'm thinking about is the fact that we modify the mbedtls and whether that can cause any issues or not. Whether it would maybe make sense to rename the library from mbedtls to iotivty-lite-mbedtls or something to avoid clashing with an installation of a standard mbedtls.

[Oxymoron79]

Yes indeed. We will install it, the only thing I'm thinking about is the fact that we modify the mbedtls and whether that can cause any issues or not. Whether it would maybe make sense to rename the library from mbedtls to iotivty-lite-mbedtls or something to avoid clashing with an installation of a standard mbedtls.

That's a good point. Renaming the mbedtls library is a good suggestion!

I intended to address this issue when packaging iotivity-lite by marking the package that it provides/conflicts with the mbedtls package. But a renamed mbedtls library (my suggestion would be "ocf-mbedtls") would be even better.

[jkralik]

I'm not sure with the renaming to ocf-mbedtls via soname. Because If someone want to use own mbedtls with OCF patches this could be a issue.

[Danielius1922]

Yeah, we seem to have a use case when a client wants to use a single instance of mbedtls library with iotivity-lite and some other binaries. Though I don't know how exactly are they are using it, renaming the mbedtls targets might break something for them, so we have to clarify that with them. For now we agreed with @jkralik on a compromise solution - to control whether mbedtls is installed by a CMake option.

set(OC_INSTALL_MBEDTLS ON CACHE BOOL "Include mbedtls in installation")

...
if (OC_INSTALL_MBEDTLS)
  add_subdirectory(${PROJECT_SOURCE_DIR}/deps/mbedtls)
else()
  add_subdirectory(${PROJECT_SOURCE_DIR}/deps/mbedtls EXCLUDE_FROM_ALL)
endif()

(Also something like OC_USE_PREINSTALLED_MBEDTLS which would make CMake use find_package to find preinstalled mbedtls instead of using the submodule might be useful. I will add this to my ideas notebook.)

[Oxymoron79]

For now we agreed with @jkralik on a compromise solution - to control whether mbedtls is installed by a CMake option.

set(OC_INSTALL_MBEDTLS ON CACHE BOOL "Include mbedtls in installation")

...
if (OC_INSTALL_MBEDTLS)
  add_subdirectory(${PROJECT_SOURCE_DIR}/deps/mbedtls)
else()
  add_subdirectory(${PROJECT_SOURCE_DIR}/deps/mbedtls EXCLUDE_FROM_ALL)
endif()

@Danielius1922 I will update this PR with the option that you propose.

(Also something like OC_USE_PREINSTALLED_MBEDTLS which would make CMake use find_package to find preinstalled mbedtls instead of using the submodule might be useful. I will add this to my ideas notebook.)

I am already working on exactly such a solution 😃 I'm almost finished - I just want to do some more tests and understand the patches a bit better.

Shall I create a separate PR to present and discuss it?

[Danielius1922]

I am already working on exactly such a solution 😃 I'm almost finished - I just want to do some more tests and understand the patches a bit better.

Shall I create a separate PR to present and discuss it?

Nice, seems that it is an obvious thing to add since we both thought of it. Yes, make the PR, I'll check it out.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[Danielius1922]

@Oxymoron79 Could you push the branch with the CMake updates to enable use of preinstalled mbedTLS as we talked about back in December? We want to allow to use both preinstalled mbedTLS and tinyCBOR. If you have something (even if unfinished) you'll save me some work.

[Oxymoron79]

@Danielius1922 Sorry for the delay. I have now created the draft PR: #356