Refactor CMakeLists.txt files

[dbear496]

• improves version detection with GetGitRevisionDecription module
• moves dependency management implementation details to the superproject (cleans up the FetchContent mess that existed before)
• divorces creating the library target and adding sources
• uses source/header file sets
• moves all json api generator stuff to the src/jsonapi subdirectory
• uses configure_file to configure the jsonapi doxygen file
• uses install components intead of the RS_LIBRETROSHARE_STANDALONE_INSTALL cache variable
• fixes the install destination paths (they should be relative to the install prefix--not absolute)

As it stands, this requires a pretty recent version of CMake because I use the SOURCES file set type. I can work on supporting older versions of CMake, but for now I'd like some feedback on what I have.

This PR should be paired with RetroShare/RetroShare#3288 .

[jolavillette]

I built this pair locally on Linux, Qt5 and Qt6, after applying three fixes. Commenting here for both PRs, with the two root-repo blockers noted in RetroShare/RetroShare#3288.

Worth saying up front: no CI run on either PR has reached the compile step. All of them fail at submodule initialisation, so none of those logs contains compiler output.

Blockers

1. CMakeLists.txt:433 — rnp and openpgpsdk are linked PRIVATE but exposed by a public header. pqi/authgpg.h includes pgp/rnppgphandler.h, which includes rnp/rnp.h. With a PRIVATE link the include directories don't propagate, so every consumer (retroshare-service, retroshare-friendserver, retroshare-gui) fails with fatal error: rnp/rnp.h: No such file or directory. master compensated with an explicit target_include_directories(${PROJECT_NAME} PUBLIC "${RNPLIB_SRC_DIR}/include" …) next to the PRIVATE link, and that block is gone. PUBLIC on both branches of if(RS_RNPLIB) fixes it — openpgpsdk has the same exposure via pgp/openpgpsdkhandler.h.

2. src/CMakeLists.txt — two .cc files land in the HEADERS file set and are never compiled. util/rskbdinput.cc and util/rsthreads.cc are listed in both RS_SOURCES and RS_IMPLEMENTATION_HEADERS. Now that the latter feeds target_sources(… FILE_SET HEADERS …), CMake sets HEADER_FILE_ONLY on them and excludes them from compilation. Both translation units are silently dropped from the library, and every executable then fails to link with thousands of undefined references to RsMutex::lock/unlock, RsThread::* and RsUtil::rs_getpass. Nothing fails earlier because a static library is archived without symbol resolution.

This duplicate listing predates your PR — it's on master too, harmless there because RS_IMPLEMENTATION_HEADERS only drives install(). Adopting file sets is what arms it. Removing the two .cc lines from the header list fixes it, and it's probably worth a separate cleanup on master regardless.

3. Windows and Android can't configure at all. set_target_properties and target_link_options on ${PROJECT_NAME} at lines 274-293 run before add_library(${PROJECT_NAME}) at line 299, so CMake fails with Can not find target to add properties to: retroshare. Both blocks are guarded by BUILD_SHARED_LIBS, and both platforms build shared: .github/workflows/windows-cmake.yml:102 and misc/Android/prepare-toolchain-clang.sh:922 both pass -DRS_LIBRETROSHARE_STATIC=OFF -DRS_LIBRETROSHARE_SHARED=ON. Only a static desktop build dodges it, which is exactly why a Linux run looks clean.

Same code path: CMAKE_POSITION_INDEPENDENT_CODE ON is commented out with "Not sure if this is needed". It is — both platforms link static dependencies (openpgpsdk, bitdht, rnp, and on Android also bzip2/OpenSSL/SQLite) into a shared object, which is what the original comment was about.

Other findings

• RS_DATA_DIR: I agree install destinations should be relative to the prefix, but this one is also compiled into the binary and used as a runtime path in src/rsserver/rsaccounts.cc:855 (dataDirectory = RS_DATA_DIR;). With share/retroshare a system install resolves it against the working directory. It needs to stay absolute at that use site.
• RS_LIBRETROSHARE_STANDALONE_INSTALL: the Android toolchain script passes it explicitly (misc/Android/prepare-toolchain-clang.sh:924), so replacing it with install components isn't a no-op — that script needs updating in the same change.
• RsInit::libRetroShareVersion(): the LIBRS_* macros are defined as already-quoted strings and rsinit.cc applies RS_PRIVATE_STRINGIFY on top. The built binary literally contains "0"."6"."7""-559-gc78f46135", quote characters included. It also drops the engine git hash master reported there, which is what distinguished the library's identity from the application's.
• rsinit.cc carries a fair amount of unrelated whitespace churn plus a duplicated #include "retroshare/rsversion.h"; it inflates the diff and will conflict with anything else touching that file.

Testing caveat

Linux desktop only so far, Qt5 and Qt6, which is a static build. The current build also works on Windows, macOS and Android, so that's the bar this pair needs to clear. Blocker 3 is Windows and Android only and I haven't triggered it — Android has no CI at all, so it would surface only when someone runs prepare-toolchain-clang.sh.

I have fixes for blockers 1 and 2 on a branch and can open a PR against cmake-refactor if that's useful.

[dbear496]

That all looks simple enough to fix.

Regarding CMAKE_POSITION_INDEPENDENT_CODE, this will no longer work in libretroshare because the dependencies are (or at least could be) configured before libretroshare. So this line should somehow be moved to supportlibs/CMakeLists.txt.

Oops. I didn't realize target_compile_definitions had some weird escaping rules, so I thought the quotes would removed my CMake.

It seems that a lot of these issues are triggered by Windows/Android/shared compilation. I don't have an Android toolchain, so I can't really test that, but I can try testing with mingw in the future. Also I should probably test a shared build on Linux.

[jolavillette]

Agreed on CMAKE_POSITION_INDEPENDENT_CODE moving to supportlibs/, before any dependency target is
created. Two notes on where it matters: Android already gets it from outside
(prepare-toolchain-clang.sh:253 passes -DCMAKE_POSITION_INDEPENDENT_CODE=ON inside andro_cmake(),
which configures both the dependencies and libretroshare), and on MinGW -fPIC is a no-op. The case
that actually needs it is a shared build on Linux — linking non-PIC .a dependencies into a .so
fails outright — so the Linux shared build you mentioned is exactly the right test for it.

One correction to my earlier comment: I said the two .cc in the header list were already on master.
They're not, this PR adds them. No master cleanup needed, just drop src/CMakeLists.txt:728 and :740.

That Linux shared build won't reach blocker 3 though — both offending blocks are inside if(WIN32) and
if(RS_ANDROID), so Linux skips them and configures fine; mingw hits it immediately. And blocker 3 is
actually a duplicated block: lines 274-293 are a verbatim copy of code that already exists after
add_library (279 vs 550, 284 vs 555, 291 vs 575), so deleting 274-293 is the whole fix.

[dbear496]

I addressed all your points except for the CI stuff. I pushed what I have, but I have done only basic testing so far.

This PR never added .cc files to RS_IMPLEMENTATION_HEADERS -- those .cc files were there on master until a recent patch, which came after this PR was created. I rebased onto the latest master to adopt the fix.

[jolavillette]

You're right about the .cc files and I was wrong to "correct" myself — they are in
RS_IMPLEMENTATION_HEADERS at the merge-base, 46e3789. I checked against current master instead of
the merge-base, after #362 had already cleaned it up. Sorry for the noise; the rebase settles it.

Confirmed the rest of 7b97501 on my side: PUBLIC on rnp/openpgpsdk, the duplicated platform block
gone, the version macros, RS_DATA_DIR, and the duplicate include.

The standalone question is now answered by your own CI

Ubuntu / CMake, macOS / CMake and Windows UCRT64 / CMake now run on this branch and all three fail
identically:

CMake Error at CMakeLists.txt:406 (find_package):
  Could not find a package configuration file provided by "rnp"

Those jobs build libretroshare on its own, outside the super-project, with only system packages. On
master the in-tree FetchContent_Declare calls covered rnp, openpgpsdk, bitdht, restbed and
udp-discovery; now they live in supportlibs/ and nothing provides them. Same for
find_package(SQLCipher) (450) and find_package(MiniUPnPc) (464), whose Find modules only exist in
the super-project's mk/cmake/ — libretroshare's own holds just the two GetGitRevisionDescription
files. misc/Android/prepare-toolchain-clang.sh:921 configures libretroshare standalone too.

So it's a real decision, not just red CI: either supportlibs/ becomes reachable from libretroshare
alone, or libretroshare keeps a fallback declaration when nothing has provided the dependency.

Three things that survive a green build

1. The JSON API generates nothing. src/jsonapi/jsonapi-generator-doxygen.conf.in:70 is
INPUT = @CMAKE_CURRENT_SOURCE_DIR@/src/, but configure_file runs from src/jsonapi/, so it expands
to <libretroshare>/src/jsonapi/src/, which doesn't exist — same for the EXCLUDE entries at 154-156.
On master CMake appended those from the top level, where ${CMAKE_CURRENT_SOURCE_DIR}/src/ was right.
Doxygen doesn't treat a missing INPUT as an error, so running it with the conf as configured gives
exit 0 and an xml/ directory with zero *_8h.xml; the generator then exits 0 and writes
jsonapi-includes.inl and jsonapi-wrappers.inl at 0 bytes each. It compiles, links, and comes up
with no registered endpoints, taking the web UI with it. INPUT = @PROJECT_SOURCE_DIR@/src/ fixes it.

2. Public headers are no longer installed. master set PUBLIC_HEADER as a target property, so
PUBLIC_HEADER DESTINATION at line 581 picked it up. The headers are now in a FILE_SET, and
PUBLIC_HEADER DESTINATION does not install a file set — I checked with a minimal project: the archive
installs, the header doesn't. It needs FILE_SET HEADERS DESTINATION "${RS_INCLUDE_INSTALL_DIR}". The
foreach over RS_IMPLEMENTATION_HEADERS still works, so it's the 48 retroshare/*.h that are lost.

3. EXCLUDE_FROM_ALL makes the default install a no-op. Only
cmake --install --component libretroshare installs anything. prepare-toolchain-clang.sh:930 runs
plain make install, so on Android nothing reaches ${PREFIX}. That script also still passes
RS_LIBRETROSHARE_STANDALONE_INSTALL=ON at line 927.

Minor, same area: JSONAPI_GENERATOR_OUTPUT_DIR at src/jsonapi/CMakeLists.txt:60 is never set (master
defined it as ${CMAKE_BINARY_DIR}/jsonapi-generator.workdir/src/), so it expands empty and silently
adds src/jsonapi/ to the include path. And libRetroShareVersion() no longer reports
RS_LIB_VERSION_HASH, which is what distinguished the engine's identity from the application's.

[dbear496]

Those jobs build libretroshare on its own, outside the super-project, with only system packages.

It is the intent of this PR to separate libretroshare from depending on and making assumptions about the superproject. libretroshare master assumes that it will be inside the superproject by referencing ../supportlibs/, and this is an anti-feature. In this PR, the superproject is treated as a convenience system that automatically makes dependencies available. If a user wants to build libretroshare standalone, it is on them to supply the dependencies.

Therefore, I propose changing the CI jobs to provide all necessary dependencies in typical system install locations. For dependencies that are not CMake-aware, libretroshare can include Find Modules to find those dependencies in their typical locations. An alternative is to move all the libretroshare dependencies from the superproject and make them submodules inside libretroshare.

[dbear496]

Could you describe what you mean here? The RsInit::libRetroShareVersion result contains LIBRS_EXTRA_VERSION which includes the sha of the current git commit.

[jolavillette]

You're right, and my remark doesn't stand — I mistook the removal of the RS_LIB_VERSION_HASH
variable for the loss of what it carried. Running both commands on the same commit gives the same
string: master's git describe --tags --always --dirty and your
git describe --tags --long --always --dirty --match v*.*.* both yield v0.6.7-594-gb2da4160d, and
git_describe_working_tree runs in CMAKE_CURRENT_SOURCE_DIR, so it describes libretroshare itself
even under the super-project. Your form is actually the better one — on the v0.6.7 tag itself,
master's collapses to v0.6.7 while yours keeps v0.6.7-0-gde2b4bd80. Withdrawn.

Also confirmed b2da4160: the doxygen INPUT fix works — rerunning the generator here now yields 392
*_8h.xml files and jsonapi-includes.inl / jsonapi-wrappers.inl at 733 bytes and 985 KB instead of
0. FILE_SET HEADERS DESTINATION and the JSONAPI_GENERATOR_OUTPUT_DIR cleanup look right too.

Two things left from that list. EXCLUDE_FROM_ALL still makes a plain make install a no-op, which is
what misc/Android/prepare-toolchain-clang.sh:930 does — that script needs a
--component libretroshare install, or it silently installs nothing. And dropping the
RS_IMPLEMENTATION_HEADERS loop means external consumers now get only the 48 public headers; master
installed the private ones on purpose, with the comment that libretroshare doesn't hide its
implementation details. Deliberate, given your TODO about exporting targets?

On the standalone question: your framing makes sense to me — the ../supportlibs/ reference on master
is an assumption libretroshare shouldn't be making. Supplying the dependencies from system locations in
those three CI jobs, plus Find modules in libretroshare for the ones that aren't CMake-aware, sounds
like the right shape. That means FindSQLCipher.cmake and FindMiniUPnPc.cmake want to live in
libretroshare rather than only in the super-project, since find_package(SQLCipher REQUIRED) and
find_package(MiniUPnPc REQUIRED) are called from there.

[dbear496]

I'm aware there is some work to be done on build scripts and CI that I haven't gotten to yet, and that includes prepare-toolchain-clang.sh.

I removed the loop that explicitly installs each element of RS_IMPLEMENTATION_HEADERS because I thought it was redundant--the HEADERS file set already includes RS_IMPLEMENTATION_HEADERS, so installing the file set should likewise install the implementation headers. The only headers not installed are the ones generated for the JSON API, and those are not exposed directly or transitively in public headers.

[jolavillette]

Confirmed, and my point was wrong. I mirrored the layout in a minimal project to check: installing the
file set preserves the directory structure, so retroshare/rsfiles.h and util/rsthreads.h land in
${RS_INCLUDE_INSTALL_DIR}/retroshare/ and ${RS_INCLUDE_INSTALL_DIR}/util/ — the destination the loop
was aiming for with ${P_DIR} — and the private file set isn't installed. RS_IMPLEMENTATION_HEADERS
does go into the public file set (src/CMakeLists.txt:879), so the loop was indeed redundant. Nothing
includes the generated .inl files from a header either, only jsonapi.cpp, so nothing is missing
there.

One thing that makes it even safer than you assumed: that loop wasn't installing anything at all. The
list(APPEND RS_IMPLEMENTATION_HEADERS …) calls live in src/CMakeLists.txt with no PARENT_SCOPE,
and the loop sat in the top-level file after add_subdirectory(src), so it iterated over an empty list.
Which means before b2da4160 no headers were installed at all — the public ones lost to
PUBLIC_HEADER DESTINATION not applying to a file set, the private ones to that empty loop. Your commit
fixes both.

Good to know the build scripts and CI are on your list; that covers my make install /
prepare-toolchain-clang.sh point, so I'll leave it with you.

[jolavillette]

Nice to see Ubuntu / CMake green — first time on this branch. Three things in 7b8962fb.

install(FILES "${BD_BOOT_FILE}" …) has no COMPONENT or EXCLUDE_FROM_ALL, unlike the
install(TARGETS) right above it. I reproduced the combination in a minimal project: a plain
cmake --install installs bdboot.txt and nothing else. (BD_BOOT_FILE itself is fine — the bitdht
config package exports it from both the build tree and the install tree.)

-DRAPIDJSON_ROOT=${{github.workspace}}/rapidjson/build points at a directory that never exists —
there's no rapidjson step in the workflow, it comes from rapidjson-dev via apt. Leftover.

The workflow hard-codes dbear496/RetroShare_BitDHT branch cmake-refactor, and clones rnp at
whatever master happens to be. That makes an upstream project's CI depend on a personal fork branch
and on a moving HEAD; worth pinning both once RetroShare/BitDHT#3 lands.

Also, only ubuntu-cmake.yml was updated: macOS / CMake and Windows UCRT64 / CMake still fail at
CMakeLists.txt:415 (find_package) on rnp. And on macOS specifically, FindMiniUPnPc.cmake and
FindSQLCipher.cmake are pkg-config-only — Findrestbed and Findudp-discovery-cpp use
find_library, which has no such requirement — so those two will need pkg-config present on the runner.

[dbear496]

I'm not really sure how the mingw workflow is supposed to work because it references submodules that never were a part of this git repo.

[dbear496]

I tried transferring my changes to the Ubuntu CI to the Windows and Mac CI jobs. For the Ubuntu workflow, I was able to use act to test it locally, but I don't have a good way to test the other workflows and iteratively debug.