Enable osign gtest suite under CTest - #134
Conversation
Port the osign library from qmake to CMake and register its GoogleTest suite with CTest. This was the last un-migrated gtest suite in the list tracked by issue Euro-Office#93, blocked only by the missing osign build target. The suite needs no fixtures — it generates its own passwords and certificates at run time — so the target needs neither staged data nor a custom WORKING_DIRECTORY. Notes on the translation of osign.pro: - common_openssl.cpp is compiled into the library. osign.pro pulls it in through "CONFIG += open_ssl_common" (openssl.pri) and certificate.cpp calls NSOpenSSL::PBKDF2 and AES_*_desktop_GCM, so the test executable does not link without it. - Base64.cpp and File.cpp are deliberately not compiled in. The .pro compiled them directly because qmake did not link kernel; the CMake target links kernel, which already provides NSBase64 and NSFile. - KERNEL_USE_DYNAMIC_LIBRARY_BUILDING is not carried over. In the .pro it only served to export those two files from osign; the target now wants KERNEL_DECL = Q_DECL_IMPORT. - src/Certificate_openssl.h and the support_oform block are dropped — neither header exists under osign/lib/src and no source includes them. - The core_windows link list is ported verbatim but is untested here, as the port was developed and verified on Linux. Like ooxmlsignature, the library is not added to the default build; it is reached only through the test's guarded add_subdirectory, so nothing changes when EO_BUILD_TESTS is OFF. Verified locally: osign_test builds from a clean target directory and reports 6/6 passing; the seven other built CTest suites still pass alongside it. Assisted-by: Claude Code:claude-opus-5 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krzysztof Cieślik <132496025+Hathor875@users.noreply.github.com>
chrip
left a comment
There was a problem hiding this comment.
The engineering is correct and I could not fault a single translation decision;
the only substantive finding is a one-line factual error that would otherwise be baked into
repo documentation. Two things before merge:
1. Reword the ooxmlsignature comparison in TESTING.md. The new block currently ends:
Like
ooxmlsignature, the library is not part of the default build: it is reached only
through the test's guardedadd_subdirectory.
ooxmlsignature is in the default build — CMakeLists.txt:22 →
X2tConverter/build/cmake/CMakeLists.txt:11 →
X2tConverter/build/cmake/library/CMakeLists.txt:124-126, where x2tlib adds
DesktopEditor/xmlsec/src with no EO_BUILD_TESTS gate. Keeping osign test-only is still the
right call — nothing in x2tlib or the DocumentServer packaging references it — so only the
comparison is wrong. Suggested replacement:
Unlike
ooxmlsignature— whichx2tlibpulls into the default build —osignhas no
non-test consumer, so it is reached only through the test's guardedadd_subdirectory
and nothing changes whenEO_BUILD_TESTSis OFF.
The same sentence appears in the PR description and the commit message; worth fixing there too,
but TESTING.md is the copy that becomes permanent repo documentation.
2. CI approval — on me, not you. Nothing beyond the DCO check has run: the Build, Build
(Windows x64) and Build WASM runs are all sitting in action_required. As a first-time
contributor with read access pushing from a fork, you cannot release them yourself, so there is
nothing for you to do here — I'll approve the runs so the Linux job actually compiles osign
and executes osign_test.
With the reword in and a green Linux build, this is a straightforward approve. Nice, careful
piece of work otherwise — every translation decision in the description held up when I checked
it against the sources.
Assisted-by: ClaudeCode:claude-opus-5
|
Thanks for catching that. Will verify the x2tlib chain and fix the wording in TESTING.md, the commit message and the PR description. Understood on CI. |
The osign entry claimed ooxmlsignature is not part of the default build. It is: CMakeLists.txt:22 adds x2t, X2tConverter/build/cmake/CMakeLists.txt:11 adds x2tlib, and X2tConverter/build/cmake/library/CMakeLists.txt:123-125 pulls in DesktopEditor/xmlsec/src with no EO_BUILD_TESTS gate. Keeping osign test-only is still correct — nothing outside the test references it — so only the comparison was wrong. Reworded to say what actually distinguishes the two. Reported in review by @chrip on Euro-Office#134. Signed-off-by: Krzysztof Cieślik <132496025+Hathor875@users.noreply.github.com> Assisted-by: Claude Code:claude-opus-5 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Two notes on CI:
|
Part of #93
Port the osign library from qmake to CMake and register its GoogleTest suite with CTest. This was the last un-migrated gtest suite in the list tracked by issue #93, blocked only by the missing osign build target.
The suite needs no fixtures — it generates its own passwords and certificates at run time — so the target needs neither staged data nor a custom WORKING_DIRECTORY.
Notes on the translation of osign.pro:
Unlike ooxmlsignature — which x2tlib pulls into the default build — osign has no non-test consumer, so it is reached only through the test's guarded add_subdirectory, and nothing changes when EO_BUILD_TESTS is OFF.
Verified locally: osign_test builds from a clean target directory and reports 6/6 passing; the seven other built CTest suites still pass alongside it.
Built on Fedora, which needs two local deviations from TESTING.md (not part of this PR): libstdc++-static installed, and third-party deps built beforehand so -DTHIRD_PARTY_PREPARED=TRUE can skip the V8 step, which needs clang-13. osign does not depend on V8.
Assisted-by: Claude Code:claude-opus-5
Tests
The
-Wparentheseswarning above is pre-existing and cosmetic:certificate.cpp:153reads
if (1 != ASN1_STRING_set(...) == 1), which parses as(1 != ...) == 1and istherefore true exactly when the call fails, so the fallback behaves as intended.
Not touched here.
No regressions in the suites already migrated:
ooxml_testandtest_odfwere not built in this tree, hence the "Not Run" and theexclusion —
ooxml_testis additionally markedDISABLEDin the repo. Everything thatwas built passes.