search

laurencelundblade / QCBOR

Comprehensive, powerful, commercial-quality CBOR encoder/ decoder that is still suited for small devices.
Other
183 stars 47 forks source link

Update CMakeLists.txt #169

Closed davidvincze closed 1 year ago

davidvincze commented 1 year ago

This is a successor of PR #159

laurencelundblade commented 1 year ago

Hi David,

The main cmake works for me now, but I don't know what the test cmake is supposed to do. I couldn't get it to do anything useful. It looks like it builds the tests into a library. But that's weird because they're not a library. They should build into a command line that can be run, perhaps by CI.

Generally speaking, cmake should be brought up to the level of makefile allowing install, making the shared lib and so on. There should be documentation in the README on how to use it... I'm convinced that QCBOR should fully support cmake. However at this point I'm not a cmake expert.

It should also be aligned with the Makefile, particularly the compiler flags. The ones in the Makefile are chosen to work everywhere and compile without warning.

I know that's more than you signed up for here. What I'd like to resolve before merging is what the test cmake is for.

Thx.

davidvincze commented 1 year ago

Thanks for the review Laurence, I've updated the PR. I also added a CMake section to the README.

davidvincze commented 1 year ago

No install rules have been added to CMake, do you think it would be required to support the installation of the qcbor library at an environment dependent location in CMake? I can do this part this if you think so.

My main concern was the unwanted process of the installation when qcbor is included by an other project as a CMake subdirectory, but it looks like there is a solution to it. I'm no CMake expert either.

laurencelundblade commented 1 year ago

Hi David,

This is all working as advertised for me. Thanks for the documentation and all. I'm OK merging as is.

A question on shared libraries. I can have cmake output for regular libraries or shared libraries as advertised and it works for me. In one case it builds a .a and in the other a .dylib. The thing is I can't see why it works. The -Os option is for size and the -fpic is for position independent code. The regular makefile adds a -shared option to get it to be a shared library. Any ideas?

Install should be supported at some point, but doesn't need to merge this.

Thanks!

davidvincze commented 1 year ago

Hi Laurence,

Thanks for the review and approval. The shared library creation is entirely handled by CMake. As the QCBOR documentation mentions the BUILD_SHARED_LIBS option controls this - it is a built-in global CMake option that will affect the add_library() command if the type of the library was not specified explicitly. CMake is responsible for generating the input (with the required options; e.g -shared) for the native build system.