elucideye/acf

api tests vs unit tests

headupinclouds opened this issue · 5 comments

headupinclouds commented

Review API tests (public/exported symbols) vs unit tests (private or hidden utility classes and functions). Most of the ACF tests fall under the API test category (they link against public symbols in the ACF_EXPORT classes + functions. We may also have utility hidden/private code that is tested only indirectly through the API calls, and that warrants more rigorous direct testing elsewhere. Two examples of private code that warrants direct testing are the local ACF shader classes and the optimized SIMD RGBA/texture unpacking code. We don't want to add such functionality to the API just to support devops testing tasks, so we have a few options.

  1. Ideally, we would build this code in OBJECT libraries so it could be reused in a test app and by the main ACF library, but OBJECT libraries aren't portable in general.
  • pros: optimal
  • cons: OBJECT libraries aren't portable
  1. As an alternative we can build that code as a support STATIC library, and the main ACF library can link to that code. The additional library should be more or less transparent to the user if they are using a SHARED ACF library (it is absorbed by the library) or if they are using a STATIC ACF library through CMake find_package(acf CONFIG REQUIRED) target_link_libraries(foo PUBLIC acf::acf) since the generated package configuration code will provide the transitive linking transparently.
  • pros: supports reuse (avoid recompilation)
  • cons: adds another library (mostly an issue for STATIC builds and non-cmake users -- in practice, it would be fairly messy to use ACF as a STATIC lib without CMake/Hunter anyway, due to the STATIC 3rdparty dependencies, so that seems to weaken the argument against introducing support STATIC libraries)
  1. If we want to avoid exporting an additional private library (it wouldn't have public headers but would be exported in the installation step), then we could collect the required source (sugar, etc) and just recompile the code directly into the test exe or via a test only static lib.
  • pros: avoid introducing an extra support lib in the ACF export set
  • cons: requires recompilation of the code (not a huge concern in ACF) and adds some additional build complexity

Since the ACF API tests will link to the compiled ACF library, and the ACF unit tests will link to some copy of private ACF code (already inside the the ACF library) the two tests should be managed in separate executables to avoid ODR conflicts.

headupinclouds commented

Separate public vs private tests added in #93 , will leave open for now in case of follow up efforts.

ruslo commented

As an alternative we can build that code as a support STATIC library

Additional "cons": leads to pretty hairy CMake code, something like this if you want to support static/shared variants:

add_library(foo foo.cpp) # public API
add_library(boo STATIC boo.cpp) # private API

if(BUILD_SHARED_LIBS)
  # boo objects absorbed by foo, boo will not be installed
  set_target_properties(boo PROPERTIES POSITION_INDEPENDENT_CODE)
  target_link_libraries(foo PUBLIC $<BUILD_INTERFACE:boo>)
else()
  target_link_libraries(foo PUBLIC boo)
  install(TARGETS boo ...)
endif()

Also we will hit issue similar to this one if there will be usage requirements and some private headers installed:

we could collect the required source (sugar, etc) and just recompile the code directly into the test exe or via a test only static lib

Additional "cons": we may miss usage requirements

OBJECT libraries aren't portable

For the reference: https://cgold.readthedocs.io/en/latest/rejected/object-libraries.html

So as a summary:

(1) should be used in such cases, "target_link_libraries" and "Usage requirements" issues should be fixed in CMake itself, for the "No real sources" and "Name conflict" need to think about workarounds (CMake can add dummy source, CMake can use renamed copy of file).

(3) sounds like a simplest/cleanest workaround so far, affects only build performance, doesn't affect user's side.

headupinclouds commented 
  add_library(foo SHARED ${srcs})
  set_target_properties(boo PROPERTIES POSITION_INDEPENDENT_CODE)
  target_link_libraries(foo PUBLIC $<BUILD_INTERFACE:boo>)

☝️ Isn't the POSITION_INDEPENDENT_CODE redundant here? If you are building everything from source and are using a toolchain that can build aSHARED library, then I believe any STATIC library resulting from add_library(boo STATIC ...) will be compatible ("absorbable").

headupinclouds commented

Also we will hit issue similar to this one if there will be usage requirements and some private headers installed: https://gitlab.kitware.com/cmake/cmake/issues/16324

From this comment (linked to in your initial issue) it seems CMake 3.12 will propagate usage requirements for OBJECT libraries. If I'm reading that correctly, that would leave Xcode OBJECT library incompatibilities outlined in your cgold post as the only remaining issue to be resolved for widespread portable use.

I see other people resorting to STATIC lib approximations of OBJECT libs in that post.

UPDATE: See https://gitlab.kitware.com/cmake/cmake/issues/18010. It seems CMake 3.12 will indeed "modernize" OBJECT libraries. 😄

ruslo commented

Isn't the POSITION_INDEPENDENT_CODE redundant here? If you are building everything from source and are using a toolchain that can build a SHARED library

You mean toolchain like gcc-pic? Yes, but PIC applied automatically if you have add_library(foo foo.cpp) without specifier and you are using BUILD_SHARED_LIBS=ON.

E.g. if you want to build without toolchain (Linux + GCC) and have no POSITION_INDEPENDENT_CODE:

> git clone https://github.com/forexample/static-shared-pic-needed
> cd static-shared-pic-needed
> cmake -H. -B_builds -DBUILD_SHARED_LIBS=ON
> cmake --build _builds
Scanning dependencies of target boo
[ 25%] Building CXX object CMakeFiles/boo.dir/boo.cpp.o
[ 50%] Linking CXX static library libboo.a
[ 50%] Built target boo
Scanning dependencies of target foo
[ 75%] Building CXX object CMakeFiles/foo.dir/foo.cpp.o
[100%] Linking CXX shared library libfoo.so
/usr/bin/ld: libboo.a(boo.cpp.o): relocation R_X86_64_32 against `.rodata' can not be used when making a shared object; recompile with -fPIC
libboo.a: error adding symbols: Bad value

it seems CMake 3.12 will propagate usage requirements for OBJECT libraries

I saw that but haven't really tried since the other issues still remain.