Conversation
|
DDvO
left a comment
DDvO
left a comment
There was a problem hiding this comment.
Thank you for your contribution.
Here are a couple of questions and suggestions.
Can you please also add a Windows CI build to .github/workflows/ ?
|
Ah, just realized that @rajeev-0 had already been working on this and earlier today rebased his branch on your work: master...add_windows_CI |
…rate static library instead of dynamic library
|
|
I have made the changes requested above and now all the 16b checks are passes. Requesting you to re-review the changes |
DDvO
left a comment
DDvO
left a comment
There was a problem hiding this comment.
Thank you for doing the requested changes, including the README extension.
Here is a couple of more/follow-up comments and a question on the CI checks.
| $env:GENCMP_NO_SECUTILS=1; cmake .. | ||
| cmake --build . --config Release |
There was a problem hiding this comment.
So far, this tests only building in Release mode a dynamic library.
I suggest adding here one more build using the new static option and the Debug mode (for covering also the Debug mode at the same time; I do not see the need for testing all possible four combinations).
| cd build | ||
| $env:GENCMP_NO_SECUTILS=1; cmake .. | ||
| cmake --build . --config Release | ||
|
|
There was a problem hiding this comment.
Is there any simple way of checking that the built library is actually usable?
For non-Windows builds, this is is done simply by invoking the cmpClient CLI app, but with Windows we do not have it.
| message(STATUS "generic CMP client version " ${GENCMPCLIENT_VERSION}) | ||
|
|
||
| # Option to build shared or static library (default: SHARED) | ||
| option(BUILD_STATIC_LIBS "Build static libraries instead of shared" OFF) |
There was a problem hiding this comment.
Since this new option affects only libgencmp, it should better not be called ..._LIBS.
For consistency with other options (GENCMP_NO_SECUTILS and GENCMP_NO_TLS), I suggest calling it GENCMP_STATIC_LIB.
| if(NOT DEFINED GENCMP_NO_SECUTILS) | ||
| message(FATAL_ERROR "Windows build is not (yet) supported for libSecUtils") | ||
| endif() |
There was a problem hiding this comment.
Please also mention in README.md that when using CMake, libgencmp can be built natively under Windows while this so far requires using GENCMP_NO_SECUTILS.
Even better, make GENCMP_NO_SECUTILS be implied in this case (which then needs to be done before the if(DEFINED ENV{GENCMP_NO_SECUTILS})),
document it that way, and transform the fatal error message into a warning like this:
implying GENCMP_NO_SECUTILS because with native Windows builds, libSecUtils and CLI are not supported so far
| endif() | ||
|
|
||
| if(NOT CRYPTO_LIB_EXISTS) | ||
| message(FATAL_ERROR "Neither OpenSSL crypto library exists: ${OPENSSL_CRYPTO_LIBRARY}") |
There was a problem hiding this comment.
| message(FATAL_ERROR "Neither OpenSSL crypto library exists: ${OPENSSL_CRYPTO_LIBRARY}") | |
| message(FATAL_ERROR "No OpenSSL crypto library file (neither optimized or debug variant) found for: ${OPENSSL_CRYPTO_LIBRARY}") |
3 participants
Motivation
(Please write out your motivation here.)
Proposed Changes
BEFORE (causing error)
set(USERS "^(\w:)?\Users\")
AFTER (fixed)
set(USERS "^([a-zA-Z]:)?\\Users\\")
GCC-specific compiler flags like -Woverflow, -pedantic not supported by MSVC. Added following.
if(MSVC)
MSVC compiler flags
add_compile_options(/W4) # High warning level
add_compile_options(/wd4996) # Disable deprecated function warnings
add_compile_definitions(_CRT_SECURE_NO_WARNINGS)
else()
GCC/Clang compiler flags
add_compile_options(-pedantic)
add_compile_options(
-Wall -Woverflow -Wextra -Wmissing-prototypes -Wstrict-prototypes -Wswitch
-Wsign-compare -Wformat -Wtype-limits -Wundef -Wconversion -Wunused-parameter)
add_compile_options(-Wno-c99-extensions -Wno-language-extension-token -Wno-declaration-after-statement -Wno-expansion-to-defined)
if(NOT DEFINED GENCMP_NO_SECUTILS)
add_compile_options(-Wno-sign-conversion -Wno-shorten-64-to-32 -Wno-shadow)
endif()
endif()
2. • Added stub for syslog for windows: src/genericCMPClient_util.c - #include <syslog.h> not available on Windows
3. Used string literals for const- Problem in using macros as a constant string. For example, following does not work on windows.
static const char *app_name = GENCMP_NAME;
static severity verbosity = LOG_WARNING;
Test Plan
(Please provide clear instructions on how to verify that your changes work.)