CMake migration modular cross platform#304
Conversation
…of local submodule
…DEPRECATED to OFF
…d symbols to avoid PE limit
… privately via absolute paths
|
libnrp does use CMake too? In that case it shouldn't be needed to compile it manually before, if added properly as a dependency CMake will deal with compiling of that too automatically just like the other dependencies (ex: restbed) |
| /******************** notify of new Cert **************************/ | ||
|
|
||
| #if defined(WINDOWS_SYS) && defined(X509_NAME) | ||
| // Workaround: Windows headers (<wincrypt.h>) define X509_NAME which conflicts with RetroShare types. |
There was a problem hiding this comment.
Is RetroShare which define X509_NAME as a type? In that case RS shouldn't pollute the namespace like that, and the type should be renamed to RS_X509_NAME
There was a problem hiding this comment.
X509_NAME is actually not a RetroShare type; it is an internal OpenSSL struct. We cannot rename it to RS_X509_NAME because it is defined in OpenSSL's headers. The #undef is the only safe workaround to allow using OpenSSL's X509_NAME if Windows <wincrypt.h> (which defines it as a macro) was previously included.
There was a problem hiding this comment.
double check if this still happens with WIN32_LEAN_AND_MEAN
There was a problem hiding this comment.
Done. I removed the #undef workaround and rely on WIN32_LEAN_AND_MEAN instead, which keeps <windows.h> from pulling in <wincrypt.h> (where the colliding X509_NAME macro comes from). It was already defined globally for the qmake build and for the CMake shared target, but not for the CMake static target — which is the default (RS_LIBRETROSHARE_STATIC=ON) — so I moved the definition out of the RS_LIBRETROSHARE_SHARED block to cover both. The only explicit #include <wincrypt.h> is in rsserver/rsloginhandler.cc (for DPAPI), and that file doesn't include sslfns.h, so no translation unit ever sees both. I also fixed the misleading comment: it's an OpenSSL type, not a RetroShare one.
Compilation OK linux, windows, macos
| #undef X509_NAME | ||
| #endif | ||
| #if defined(WINDOWS_SYS) && defined(X509_EXTENSIONS) | ||
| // Workaround: Windows headers (<wincrypt.h>) define X509_EXTENSIONS which conflicts with RetroShare types. |
There was a problem hiding this comment.
Same as above. X509_EXTENSIONS is an OpenSSL internal type, not a RetroShare type. We must undefine the Windows macro to be able to use OpenSSL properly.
There was a problem hiding this comment.
double check if this still happens with WIN32_LEAN_AND_MEAN
There was a problem hiding this comment.
Same as above — removed together with the X509_NAME workaround. With WIN32_LEAN_AND_MEAN now applied to the default (static) CMake target as well, <wincrypt.h> is never included in a translation unit that uses the OpenSSL types, so the X509_EXTENSIONS macro collision can no longer occur.
| }; | ||
|
|
||
| #if defined(WINDOWS_SYS) && defined(ERROR) | ||
| // Workaround: Windows headers (<wingdi.h>) define ERROR as 0, which breaks enum members named ERROR. |
There was a problem hiding this comment.
Being this a public header I think we should not mess with undefining a windows symbol, because that would happen also for any library downstream user. It is strange this is happening bacause the enum is scoped enum class. If failing anyway better investigating where the actual error is happening rstor.h doesn't include <wingdi.h> directly so it should not happen there. Another workaround could be to rename the enum values, but that should be a last resort.
There was a problem hiding this comment.
Good point. Undefining ERROR in a public header is dangerous for downstream users. I have removed the #undef ERROR hack entirely from rstor.h. Instead, I added WIN32_LEAN_AND_MEAN globally in CMakeLists.txt (as you suggested below), which completely prevents <wingdi.h> from polluting the namespace.
|
|
||
| #if defined(WINDOWS_SYS) && defined(ERROR) | ||
| // Workaround: Windows headers (<wingdi.h>) define ERROR as 0, which breaks enum members named ERROR. | ||
| #undef ERROR |
There was a problem hiding this comment.
Being this someway internal implementation here is less dangerous, but still why this used to work and now it doesn't? I remember to compile on windows a general thing must be set, it was called something "Windows lean and mean headers" or something like that so it stop polluting all around, please check that instead of undeffing things around, which is quite ugly and be done just as a last resort.
There was a problem hiding this comment.
I have removed the #undef ERROR hack here as well, and I added target_compile_definitions(${PROJECT_NAME} PUBLIC WIN32_LEAN_AND_MEAN) in CMakeLists.txt for Windows builds. This cleanly solves the macro pollution issue without touching the headers.
| if(WIN32) | ||
| set_target_properties(${PROJECT_NAME} PROPERTIES WINDOWS_EXPORT_ALL_SYMBOLS ON) | ||
| if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang") | ||
| target_link_options(${PROJECT_NAME} PRIVATE "-Wl,--export-all-symbols,--exclude-libs,libcrypto.a:libssl.a:libsqlite3.a:libopenpgpsdk.a:libbitdht.a:librestbed.a:librestbed-static.a:libminiupnpc.a:librnp.a:libsexpp.a:libbotan.a:libbotan-3.a:libbotan-2.a:libz.a:libbz2.a:libjson-c.a") |
There was a problem hiding this comment.
This doesn't smell good at all, hardcoding all those library here, is basically unmaintenable code, CMake is here to do this kind of things automatically, so if a new library get added it automatically sets the proper flags, same goes if a library dependency get removed
There was a problem hiding this comment.
I have replaced that hardcoded list with a simple -Wl,--exclude-libs,ALL. This cleaner approach prevents any statically linked library from blindly re-exporting its symbols on Windows, automatically handling any future dependency changes.
| ${PROJECT_NAME} PUBLIC "${RESTBED_DEVEL_DIR}/source/" ) | ||
| target_link_directories(${PROJECT_NAME} PUBLIC "${CMAKE_CURRENT_BINARY_DIR}/../supportlibs/restbed") | ||
| target_link_libraries(${PROJECT_NAME} PRIVATE restbed) | ||
| target_link_libraries(${PROJECT_NAME} PRIVATE restbed-static) |
There was a problem hiding this comment.
This sounds it will breaks all the targets that where working before (tens of Linux distributions, Android...) double check this thing and investigate how to avoid hard-coding static here
There was a problem hiding this comment.
You are right, hardcoding restbed-static was too restrictive and broke systems that don't install a system-wide librestbed.so. Simply reverting it to restbed caused ld: library 'restbed' not found locally because the fetched restbed project does not create an alias named restbed, so CMake was emitting -lrestbed instead of linking the local target. I have updated the logic to dynamically resolve the correct target (restbed-shared or restbed-static) based on BUILD_SHARED_LIBS, and link to that specific target using a ${RESTBED_TARGET} variable.
| ) | ||
| FetchContent_MakeAvailable(restbed) | ||
|
|
||
| if(TARGET restbed-shared) |
There was a problem hiding this comment.
Static/dynamic linking hard-coding again, is this really necessary? Do it properly not hard-coding
There was a problem hiding this comment.
I have removed the hardcoded EXCLUDE_FROM_ALL and static/shared checks. We now rely strictly on the generic BUILD_SHARED_LIBS property and seamlessly link the resolved ${RESTBED_TARGET}.
|
|
||
| if(NOT RS_WARN_DEPRECATED) | ||
| target_compile_definitions(${PROJECT_NAME} PRIVATE RS_NO_WARN_DEPRECATED) | ||
| target_compile_definitions(${PROJECT_NAME} PUBLIC RS_NO_WARN_DEPRECATED) |
There was a problem hiding this comment.
That was a mistake. I have changed it to PRIVATE so we don't suppress deprecation warnings for downstream targets.
a705f77 to
0c3a395
Compare
|
about librnp: You are completely right. We previously avoided adding it as a native CMake dependency because statically linking the librnp CMake target re-exported all of its internal dependencies (Botan, json-c, etc.) and broke the MinGW 65535 exported symbol limit on Windows. However, now that we have implemented the -Wl,--exclude-libs,ALL fix from your other comment, this limitation is gone. We can absolutely integrate librnp properly via CMake |
0c3a395 to
8f55057
Compare
8f55057 to
2225487
Compare
- Build librnp via add_subdirectory under RS_RNPLIB, saving/restoring BUILD_SHARED_LIBS so libretroshare stays shared while librnp is static. - Windows: propagate UNICODE/_UNICODE to subprojects, link winmm/iphlpapi, attach socket libs to whichever restbed target exists. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…the sslfns.h X509_NAME/X509_EXTENSIONS #undef workaround
UNICODE / _UNICODE: libbitdht/src/util/bdfile.cc:47 calls MoveFileExW() with wchar_t* arguments. Without UNICODE, the Windows headers resolve the call to MoveFileExA() which expects char*, producing: error: cannot convert 'const wchar_t*' to 'LPCSTR'. NOGDI: <windows.h> includes <wingdi.h> which does #define ERROR 0. This macro silently replaces every occurrence of ERROR in RetroShare's logging code (e.g. RsLog levels in rsdebug.h). Note: WIN32_LEAN_AND_MEAN does not exclude <wingdi.h>, so NOGDI is needed explicitly. This replaces the #undef ERROR hacks that were previously in rstor.h and rsdebug.h (removed per your earlier review comments #4 and #5). NOMINMAX: <windows.h> defines #define min(a,b) and #define max(a,b) macros that break every call to std::min() / std::max() throughout the codebase. NOCRYPT: <windows.h> includes <wincrypt.h> which does #define X509_NAME ((LPCSTR) 7) and #define X509_EXTENSIONS ((LPCSTR) 5). These macros shadow OpenSSL's X509_NAME and X509_EXTENSIONS structs, making any #include <openssl/x509.h> fail. This is the clean replacement for the #undef workaround that was in sslfns.h (removed per your review comments #2 and #3).
The exclusion you're referring to is a separate fix, on the shared/DLL build only: it had to be narrowed from --exclude-libs,ALL (which broke the build) down to just the heavy third-party archives (libbitdht, librnp, libsexpp, libudp-discovery, librestbed, libsqlcipher, libbotan-2/3, libz, libbz2) — enough to stay under the PE/COFF ~65535 exported-symbol cap while keeping libretroshare's own API exported. The default static build is unaffected. So AND NOT WIN32 should stay. |
|
@jolavillette please keep replying comment by comment, I have very little time, so try to optimize for the reviewer time ;-) |
| # <wincrypt.h> (whose X509_NAME / X509_EXTENSIONS macros collide with the | ||
| # OpenSSL types of the same name used in pqi/sslfns.h). | ||
| target_compile_definitions(${PROJECT_NAME} PUBLIC WIN32_LEAN_AND_MEAN) | ||
| endif() |
There was a problem hiding this comment.
I think WIN32_LEAN_AND_MEAN should go at line 278 or so, this also would avoid another if block and code lines to maintain
There was a problem hiding this comment.
I don't know why I can reply here and not under the next comments...
-
I have moved WIN32_LEAN_AND_MEAN to the main if(WIN32) block alongside the other Windows definitions, and removed the redundant if(WIN32) block entirely.
-
"Done. I have added inline comments right above each definition inside the CMakeLists.txt file explaining exactly what header pollution or compilation error they prevent (MoveFileExW translation, <wingdi.h> ERROR macro collision, min/max collision, and OpenSSL X509 types shadowing).
-
You're right, that find_library(BOTAN_LIBRARY ...) line was dead code leftover from before we introduced add_subdirectory(librnp). Now that librnp handles its own dependencies internally through CMake, we don't need to manually find Botan in libretroshare's CMakeLists anymore. I have completely removed it.
-
I have just added a detailed comment above the --exclude-libs line explaining that we use an explicit list instead of ALL because ALL inadvertently hides symbols from CMake's internal objects.a archive, preventing libretroshare from exporting its own core C++ symbols.
-
Regarding WIN32_LEAN_AND_MEAN and the inline comments for the definitions, this was already addressed and fixed in my latest commit. The flags are now cleanly grouped together with proper comments for each of them.
| ${CMAKE_CURRENT_BINARY_DIR}/../supportlibs/librnp/Build/src/lib | ||
| ${CMAKE_CURRENT_BINARY_DIR}/../supportlibs/librnp/Build/src/libsexpp | ||
| ) | ||
| find_library(BOTAN_LIBRARY NAMES botan botan-3 botan-2 libbotan-3 libbotan-2 REQUIRED) |
| # <wincrypt.h> (whose X509_NAME / X509_EXTENSIONS macros collide with the | ||
| # OpenSSL types of the same name used in pqi/sslfns.h). | ||
| target_compile_definitions(${PROJECT_NAME} PUBLIC WIN32_LEAN_AND_MEAN) | ||
| endif() |
- Add detailed comments explaining the necessity of an explicit --exclude-libs list over ALL (due to CMake objects.a export stripping). - Group WIN32_LEAN_AND_MEAN with other Windows compile definitions. - Add inline comments explaining each Windows compile definition (_UNICODE, NOGDI, NOMINMAX, NOCRYPT). - Remove dead find_library(BOTAN_LIBRARY) code now that librnp is a CMake subdirectory.
Implement full modular cmake compilation on linux, windows and macos
Works with the following PRs in RetroShare and i2p/libsam3
RetroShare/RetroShare#3229
i2p/libsam3#30
Example of script to compile everything:
#!/bin/bash
Exit immediately if any command fails
set -e
Clean build directories if the "clean" argument is passed
if [ "$1" == "clean" ]; then
echo "=== Cleaning build directories ==="
rm -rf libretroshare/Build
rm -rf retroshare-service/Build
rm -rf retroshare-friendserver/Build
rm -rf retroshare-gui/Build
fi
Detect number of CPU cores for fast parallel compilation
NPROC=$(sysctl -n hw.ncpu 2>/dev/null || nproc 2>/dev/null || echo 4)
Detect Qt5 path on macOS if Homebrew is installed
QT_PREFIX=""
if command -v brew &> /dev/null; then
QT_PREFIX=$(brew --prefix qt@5)
fi
echo "=== Starting RetroShare Compilation ==="
1. Compile libretroshare (core library) and librnp automatically
echo ">>> Step 1: Compiling libretroshare..."
mkdir -p libretroshare/Build && cd libretroshare/Build
cmake -DCMAKE_POLICY_VERSION_MINIMUM=3.5 -DRS_LIBRETROSHARE_STATIC=OFF -DRS_LIBRETROSHARE_SHARED=ON ..
cmake --build . -j${NPROC}
cd ../..
2. Compile retroshare-service (headless daemon)
echo ">>> Step 2: Compiling retroshare-service..."
mkdir -p retroshare-service/Build && cd retroshare-service/Build
cmake -DCMAKE_POLICY_VERSION_MINIMUM=3.5 ..
cmake --build . -j${NPROC}
cd ../..
3. Compile retroshare-friendserver
echo ">>> Step 3: Compiling retroshare-friendserver..."
mkdir -p retroshare-friendserver/Build && cd retroshare-friendserver/Build
cmake -DCMAKE_POLICY_VERSION_MINIMUM=3.5 ..
cmake --build . -j${NPROC}
cd ../..
4. Compile retroshare-gui (graphical user interface)
echo ">>> Step 4: Compiling retroshare-gui..."
mkdir -p retroshare-gui/Build && cd retroshare-gui/Build
cmake -DCMAKE_POLICY_VERSION_MINIMUM=3.5
-DCMAKE_PREFIX_PATH="${QT_PREFIX}"
-DENABLE_GUI=ON
-DENABLE_QT_TRANSLATIONS=ON ..
cmake --build . -j${NPROC}
cd ../..
echo "=== Compilation Successfully Completed! ==="
The PR also provides an orchestrator
See "make help"