Skip to content

Refactor CMakeLists.txt files - #361

Open
dbear496 wants to merge 2 commits into
RetroShare:masterfrom
dbear496:cmake-refactor
Open

Refactor CMakeLists.txt files#361
dbear496 wants to merge 2 commits into
RetroShare:masterfrom
dbear496:cmake-refactor

Conversation

@dbear496

@dbear496 dbear496 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor
  • 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 .

- improve version detection with GetGitRevisionDecription module
- move dependency management implementation details to the superproject
- divorces creating the library target and adding sources
- use source/header file sets
- move all json api generator stuff to the src/jsonapi subdirectory
- use configure_file to configure the jsonapi doxygen file
- use 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)
@jolavillette

Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants