build(cmake): minimal fixes in CMakeLists.txt for transitive restbed headers, i2pcommon, and versioning by jolavillette · Pull Request #298 · RetroShare/libretroshare

jolavillette · 2026-05-18T14:14:17Z

build(cmake): minimal fixes in CMakeLists.txt for transitive restbed headers, i2pcommon, and versioning

This PR stabilizes the CMake configuration of libretroshare to support downstream modular builds (such as retroshare-gui and retroshare-service).

To ensure maximum stability and ease of review, the modifications made to the CMakeLists files are kept strictly minimal and highly targeted:

Transitive Headers (1 line change): Promoted the restbed include directory to PUBLIC in CMakeLists.txt so that downstream modules cleanly inherit the HTTP/JSON API headers without duplicate include paths.
Link Fixes (2 lines change): Reactivated the compilation of util/i2pcommon.cpp/.h under CMake, resolving undefined reference linking errors in downstream GUI settings panels.
Version Display (minimal additions):
Directed the GUI human-readable version query to run in the parent directory's .git if present, ensuring the GUI gets the correct overall application version instead of submodule tags.
Added the RS_LIB_VERSION_HASH compile definition on the core target, fixing the [version not available] display bug in the about box.

All changes are non-intrusive and strictly limited to resolving compile/link-time blockers.

…, and versioning

…DEPRECATED to OFF

G10h4ck

Good things intermixed with wrong stuff, @jolavillette please fix them and answhere the inline questions ;-)

G10h4ck · 2026-06-01T13:58:02Z

+						hasInput = True
+					if 'out' in tmpD:
+						tmpParam._out = True
+						hasOutput = True


This change seems unrelated to the PR description, what it is supposed to do?

Thank you for reviewing this PR. Note that since I pushed it I managed to compile also on Windows and MacOS. I had to make a few more changes in some CMakeLists.txt files. I just pushed the new cross platform PR on github: see #304

Please understand that even if I did my best along the development and cross platform testing of this PR to closely monitor what was going on, the replies below were AI generated,

About json-pi generator:
This fixes a crash of the JSON API generator with some Doxygen versions (especially on Windows/macOS). The original code accesses tmpPN.text directly, which returns None when the XML node contains sub-elements instead of plain text (behavior varies across Doxygen versions). Using getText(tmpPN) (which does "".join(e.itertext())) is more robust. The if pName in paramsMap guard prevents a KeyError if Doxygen generates unexpected entries.

G10h4ck · 2026-06-01T13:58:24Z

@@ -1,4 +1,4 @@
-/*******************************************************************************
+/*******************************************************************************


The line ending was wrong?

Yes, there was trailing whitespace at end of line. Automatic cleanup by the editor. Cosmetic only, no functional impact.

G10h4ck · 2026-06-01T13:59:02Z

+#ifdef X509_EXTENSIONS
+#undef X509_EXTENSIONS
+#endif
+


This change seems unrelated to the PR description, what it is supposed to do?

This is a required fix for Windows compilation. Windows headers (<wincrypt.h>, pulled in transitively via system headers) define X509_NAME and X509_EXTENSIONS as preprocessor macros, which conflicts with their use as type identifiers in function signatures declared in sslfns.h (e.g. SignX509Certificate(X509_NAME *issuer, ...)). Without these #undef, compilation fails on Windows/MSVC.

This is a classic Windows compatibility pattern, also used by OpenSSL itself.

in that case the #if should be platform specific, like #if def(WINDOWS) && def(X509_EXTENSIONS) it also makes more explicit the intent of code, imagine reading that 2 years from now, surely you would exclaim WTF is this? also having some comment near this kind of stuff, makes it even more future readable. So please apply this adjustments to all the similar #ifdef introduced

Done. I have wrapped the #undef inside #if defined(WINDOWS_SYS) && defined(X509_EXTENSIONS) to make it strictly platform-specific. I also added an explicit comment explaining that Windows headers (<wincrypt.h>) define this as a macro which conflicts with RetroShare types.

G10h4ck · 2026-06-01T13:59:12Z

+#ifdef ERROR
+#undef ERROR
+#endif
+


This change seems unrelated to the PR description, what it is supposed to do?

Same issue as previous point: on Windows, <wingdi.h> (included transitively via <windows.h>) does #define ERROR 0. This breaks enum class members using ERROR as an enumerator, such as RsTorHiddenServiceStatus::ERROR and RsTorConnectivityStatus::ERROR. The preprocessor replaces ERROR with 0, causing a compilation error.

see previous comment

Done. Similarly, I wrapped this inside #if defined(WINDOWS_SYS) && defined(ERROR) and added a comment explaining the conflict with <wingdi.h>.

G10h4ck · 2026-06-01T13:59:24Z

+#ifdef ERROR
+#undef ERROR
+#endif
+


This change seems unrelated to the PR description, what it is supposed to do?

Same as previous points. RsLoggerCategories::ERROR is broken by the Windows macro #define ERROR 0. The #undef is required for rsdebug.h to compile on Windows.

see previous comment

Done. Same as the previous point, the #undef is now platform-specific with an explanatory comment.

G10h4ck · 2026-06-01T14:03:03Z

-		${CMAKE_CURRENT_BINARY_DIR}/../supportlibs/librnp/Build/src/libsexpp
+	target_link_directories(${PROJECT_NAME} PUBLIC
+		${CMAKE_CURRENT_SOURCE_DIR}/../supportlibs/librnp/Build/src/lib
+		${CMAKE_CURRENT_SOURCE_DIR}/../supportlibs/librnp/Build/src/libsexpp


Is the Build directory name always the same?

The Build directory is created by the librnp build script (supportlibs/librnp) and its name is hardcoded in that script. It's the same name on all 3 platforms.

The change from CMAKE_CURRENT_BINARY_DIR to CMAKE_CURRENT_SOURCE_DIR is because librnp is built outside of libretroshare's CMake build tree (via a separate script), so its build artifacts live in the source tree, not in CMake's build directory.

ok, having a short comment about that would be helpful ;-)

Done. I have added a short comment explaining that the Build directory name is hardcoded in librnp's custom external build script.

G10h4ck · 2026-06-01T14:04:48Z

+		json-c
+		bz2
+		z
 	)


Here you are changing the linking of all those libraries from private to public, that means that ALL symbols will be visible when linking against libretroshare, that is unwanted also it raise an error like "too many symbols" on some platforms. Also I think libretroshare has no reason to expose all those symbols, if some downstream user need them, they will need to link to those libraries by themselves.

You are right, PUBLIC is too broad and exposes headers/symbols unnecessarily. The reason we switched to PUBLIC was to fix downstream link errors on Windows. However, the root cause wasn't PRIVATE itself (CMake does propagate PRIVATE static dependencies for linking), but rather the use of target_link_directories(PRIVATE), which didn't propagate the -L search paths to the downstream executables.

I will revert to PRIVATE. To fix the Windows link errors properly, I will remove target_link_directories and instead link directly to the absolute paths of these static libraries (e.g., ${CMAKE_CURRENT_SOURCE_DIR}/.../librnp.a). This allows CMake to correctly resolve the libraries for downstream consumers without polluting the public interface.

seems legit

Done. I reverted it to PRIVATE. To fix the downstream Windows link errors cleanly, I removed target_link_directories and instead linked directly to the absolute paths of the static libraries (e.g., librnp.a). This prevents exposing symbols unnecessarily.

G10h4ck · 2026-06-01T14:05:42Z

-		"	${CMAKE_CURRENT_SOURCE_DIR}/src/tests"
-		"	${CMAKE_CURRENT_SOURCE_DIR}/src/unused"
-		"	${CMAKE_CURRENT_SOURCE_DIR}/src/unfinished\n" )
+	if(NOT EXISTS "${JSON_API_GENERATOR_DOXYFILE}")


Why the conditional? In which case do you want it to not be overridden by the updated one?

Without this condition, every CMake invocation (even a simple cmake --build that triggers a reconfigure) re-executes file(APPEND ...) and duplicates the OUTPUT_DIRECTORY, INPUT, EXCLUDE lines in the Doxyfile. After N reconfigurations, the file contains N copies of those directives, which generates Doxygen warnings and can cause unexpected behavior.

The original file(COPY ...) + file(APPEND ...) runs at every configure step, not just the first time. The condition prevents accumulation.

if the original is copied and then the directives appended every-time, how is it possible for the directives to accumulate?

The accumulation happens because CMake's file(COPY) skips the copy if the destination file is newer. Since file(APPEND) modifies the destination, its timestamp becomes newer than the original. On the next CMake run, COPY is silently skipped, but APPEND runs again, duplicating the text. To fix this robustly, I have removed the if(NOT EXISTS) check and completely refactored the logic to use file(READ), followed by a string(APPEND) in memory, and finally a single file(WRITE). This prevents any accumulation.

G10h4ck · 2026-06-01T14:06:18Z

 	set(BUILD_TESTS OFF CACHE BOOL "Do not build restbed tests")
 	set(BUILD_SSL OFF CACHE BOOL "Do not build restbed SSL support")
+	set(BUILD_SHARED_LIBRARY OFF)
+	set(BUILD_SHARED_LIBS OFF)


Are the two of them needed?

BUILD_SHARED_LIBS is the standard CMake variable (honored by add_library() when no type is specified). BUILD_SHARED_LIBRARY is a custom variable used specifically by restbed's CMakeLists. Both are needed because restbed uses its own convention on top of the standard CMake one.

Is it necessary to hardcode disabling them? What if a downstream libretroshare user what to link dinamically? Having the linker linking also the .so should not harm.

Done. I have removed the hardcoded OFF values. They are now dynamically set to mirror RetroShare's library preference (${RS_LIBRETROSHARE_SHARED}). Downstream users can now link dynamically if they want.

G10h4ck · 2026-06-01T14:08:10Z

 			GIT_PROGRESS TRUE
 			TIMEOUT 10
-#			EXCLUDE_FROM_ALL # Available only in CMake >= 3.28
+			PATCH_COMMAND sed -i -e "s|/wd4251||g" -e "s|VERSION 3.1.0|VERSION 3.5.0|g" CMakeLists.txt


What is this line supposed to do? At first glange it is quite brittle to hot patching CMake files line that, but above all why?

This line does two things:

Removes /wd4251: this is an MSVC flag (/wd = disable warning) that restbed puts in its CMakeLists. This flag causes an error with GCC/Clang (which don't recognize it). Since we use FetchContent on all platforms, it must be removed on non-MSVC platforms.
Changes VERSION 3.1.0 → VERSION 3.5.0: restbed's CMakeLists specifies cmake_minimum_required(VERSION 3.1.0), which triggers a deprecation warning with CMake ≥ 3.27 (old policies are deprecated). Bumping to 3.5.0 silences this warning.
You're right that it's brittle — it breaks if restbed changes those exact strings in an update.

At least a comment is needed explaining what is going on, also an upstream PR is the right thing to do

Done. I have added an explanatory comment above the command clarifying that the sed patch removes the MSVC-specific /wd4251 flag (which breaks GCC/Clang builds) and bumps the CMake VERSION to 3.5.0 to suppress deprecation warnings. I will also submit an upstream PR to restbed separately to fix this at the source.

G10h4ck · 2026-06-02T14:59:12Z

Thank you for reviewing this PR. Note that since I pushed it I managed to compile also on Windows and MacOS. I had to make a few more changes in some CMakeLists.txt files. I just pushed the new cross platform PR on github: see #304

@jolavillette should I keep reviewing this code or the #304 ? Please one thing at time so we avoid wasting code review time

jolavillette · 2026-06-02T17:52:55Z

The changes following your comments are in 304, not 298.
I am sorry for the mess.
304 was built on top of 298.
It seemed reasonnable to me to proceed step by step and I chose to implemente the cmake migration for windows first and then macos.
I now realized that handling macos first would probably have been a better idea.
304 does not add many changes in libretroshare.
It would be better if you can switch to 304 from now.

G10h4ck · 2026-06-02T19:40:49Z

should this be closed then ?

build(cmake): minimal fixes for transitive restbed headers, i2pcommon…

ffcfda9

…, and versioning

jolavillette mentioned this pull request May 18, 2026

C make migration #297

Closed

jolavillette added 8 commits May 19, 2026 07:57

Fix jsonapi generator and OpenSSL macro collisions

df1642b

Fix RNP include paths and jsonapi recompilation cache in CMake

ef67686

build(cmake): support dynamic plugin linkage by exposing RNP directories

39df2f2

build: encapsulate RNP and PGP dependencies as PUBLIC targets

60e127e

build: use target_compile_options instead of global CMAKE_CXX_FLAGS

c9804e8

build: fix deprecation warning encapsulation and set default RS_WARN_…

ad0dcf6

…DEPRECATED to OFF

Merge branch 'master' into CMakeMigration-Modular

62d1c60

Merge branch 'master' into CMakeMigration-Modular

06a059f

G10h4ck requested changes Jun 1, 2026

View reviewed changes

jolavillette closed this Jun 5, 2026

jolavillette deleted the CMakeMigration-Modular branch June 5, 2026 05:31

		@@ -1,4 +1,4 @@
		/*******************************************************************************
		/*******************************************************************************

Conversation

jolavillette commented May 18, 2026

Uh oh!

G10h4ck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jolavillette Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jolavillette Jun 2, 2026 •

edited

Loading

jolavillette commented Jun 2, 2026 •

edited

Loading