microsoft / rego-cpp

A C++ interpreter for the OPA policy language Rego
https://microsoft.github.io/rego-cpp/
MIT License
33 stars 9 forks source link

Various targets are not marked private #125

Closed davidchisnall closed 2 months ago

davidchisnall commented 8 months ago

Some flags, such as -Werror are used when building the library but should not be propagated to users.

Most of the targets do not need to be built. For example, libsnmallocshim is added as an install target to any project that is using rego-cpp, as are all of the libraries that rego-cpp uses. The install targets should not be exported for consumers wishing to vendor the library.

mjp41 commented 8 months ago

I think this is probably also a Trieste problem? Trieste has:

https://github.com/microsoft/Trieste/blob/58a6eeeaeda4f5c96dad30c23b2d4e3feae7a60c/CMakeLists.txt#L107

Which adds snmalloc to the target. This is required as Trieste depends on one header from snmalloc for the defines.h. But perhaps we need a more granular set of things exposed from snmalloc? I.e. just headers?

davidchisnall commented 8 months ago

We don't need to install snmalloc at all, since we're statically linking to it. The same is true for the Trieste headers. It looks as if Trieste is not exposing a modern CMake config file, so we are not differentiating between the 'build and install Trieste' and 'consume Trieste as a dependency' cases. In the latter case, the only thing that are implicit install targets should be shared libraries. In the former, you want all static libraries and headers installed as well.

This can be tested in the rego-cpp repository: It should be possible to build the tool in such a way that ${PREFIX}/bin/rego is the only thing that is installed.

mjp41 commented 8 months ago

Interesting. I don't know enough CMake to do that. Can you point me at the bits about "'consume Trieste as a dependency' cases". It will take me a while to get to this, as I will need to understand more CMake.

davidchisnall commented 8 months ago

I'm not really sure how the Trieste logic is working. The 'official' way of exporting things from CMake to be consumed by other projects is to expose an export configuration: https://cmake.org/cmake/help/latest/guide/tutorial/Adding%20Export%20Configuration.html

I think the Trieste bits are just included, which means that the same kind of separation needs to be done via extra options. Maybe just a 'TRIESTE_IS_JUST_A_DEPENDENCY=TRUE' or something.

mjp41 commented 6 months ago

I think this is due to the way FetchContent calls add_directory. With CMake 3.28 there is an option to not include the targets from the included projects. Following stack overflow, I have created a function that performs the add_directory step with ExcludeFromAll set.

This appears to make the set of things built much smaller.

mjp41 commented 6 months ago

So it seems that the static libraries don't get merged into one by CMake, which is causing issues. We can't get a single librego.a. There are various things on StackOverflow about merging multiple .a into a single:

https://cristianadam.eu/20190501/bundling-together-static-libraries-with-cmake/

This seems to be the most comprehensive version. @davidchisnall is there something obvious I am missing here?

matajoh commented 5 months ago

@davidchisnall I believe this has been resolved to the degree that I can do so from downstream. Are you OK with me closing this?