* [CI] Re-assert the ccache timestamp sloppiness after configure
CMakeLists.txt sets sloppiness=pch_defines,time_macros at configure
time, and ccache --set-config replaces the whole list rather than
extending it. Setting the four entries before cmake would therefore
leave the build with only two, so the entries CI needs have to be
written after the configure step and before the build.
The mtime and ctime entries cover files created during the build
itself: moc/uic/rcc output and protobuf headers are generated after
ccache starts, so without them every TU that includes one is read as
a race and never cached. Checked-out sources are older than the
compile that follows them and do not trip the check.
* [Build] Keep risky ccache sloppiness out of the global config
USE_CCACHE defaults to ON, so CMakeLists.txt runs on a developer
machine, and ccache --set-config writes the user's global ccache.conf.
The include_file_mtime/include_file_ctime entries turn off the
modified-during-compile check for every project on the machine, not
only this one, so they belong in CI (compile.sh) and not here. The two
entries PCH caching needs stay.
* [CI] Size each compiler cache against the cache quota
The 600M bound sat below what a leg writes: ccache spent each build
deleting the entries the next run asked for, with 2624-3720 cleanup
passes and about 8% of calls served in the runs inspected on
2026-11-04.
Raising it needs room first. The repository's 10 GB quota currently
holds 1.1 GB of CodeQL trap, 1.1 GB of vcpkg, 0.7 GB of Qt and 2.1 GB
of docker buildkit layers. Two of those are now cheap to reclaim:
- docker-release wrote its layer cache with mode=max, but the build
stage sits behind COPY . ., so its compile layer is invalidated by
every source change and was never read back. mode=min keeps only the
final image stage.
- the five nightly-only legs kept their own compiler caches, 600M
each, that no pull request ever reads. They now keep none, and the
prune job retires their leftover keys immediately instead of waiting
for GitHub's whole-entry LRU eviction to pick them.
That funds per-leg bounds: 700M for one configuration, 850M for the
Debian 13 release that also runs the suite, 900M for Fedora 44's
Debug and Release, 800M for the instrumented leg, 400M for the
server-only leg, and 700M for each macOS leg. 6.45G of ccache plus the
3.2G of fixed entries leaves about 350M under the quota.
The prune step also covers the buildkit entries by age now, so the
mode=max generation of layer blobs is reclaimed instead of living
forever: a layer a build needs is downloaded and its date refreshed,
and dropping one that an index still lists only costs docker-release a
rebuilt layer, which gates nothing.
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
The root already set CMAKE_AUTOMOC; add CMAKE_AUTOUIC and CMAKE_AUTORCC
next to it and drop the per-directory copies from 18 CMakeLists files.
These variables are inherited by the subdirectories, so the duplicates
never changed anything.
qt_standard_project_setup is not used: besides the AUTO* variables it
sets CMAKE_RUNTIME_OUTPUT_DIRECTORY on Windows, which breaks the
install(DIRECTORY "$<TARGET_FILE_DIR:...>") copies and the per-app
symbol paths, and appends to CMAKE_INSTALL_RPATH on Linux.
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* [Build] Use versionless Qt CMake commands
Replace the versioned qt6_add_executable, qt6_add_resources, qt6_add_shaders,\nqt6_add_translations and qt6_wrap_cpp calls with their unversioned forms.\nAll five are documented as available at the project's 6.4 minimum and accept\nthe same arguments.\n\nVerification: full client, Oracle, server and test build passed.
* versionless qt_finalize_target and _project
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
Co-authored-by: tooomm <tooomm@users.noreply.github.com>
Add the deprecation cutoff after FindQtRuntime so the compiler rejects any
API deprecated before the project's minimum Qt version. Qt 6.5 renamed the
switch, so the fallback name is used on 6.4.
The full client, Oracle, server and test build passes with the cutoff
enabled.
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* [Refactor] Move game logic into libcockatrice_game
* [Refactor] Include card_state.h in player_actions.h
* [Game] Inject settings providers into libcockatrice_game, drop Widgets
* [Game] Follow base rename to createCardRequested after rebase
* [Game] Drop unused CardItem forward declaration
* [Game] Fix includes after moving game logic into libcockatrice_game
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* [CrashReporting] Log which files and processes blocked the installer purge
A purge that fails aborts the upgrade with a message that can only guess
at the cause, and the log only listed which DLLs survived. Record what
Windows reports for each survivor and which processes still have the
install directory mapped, then repeat the holder list in the abort
message.
The diagnostics are additive and run only on the abort path: they open
each survivor for DELETE access and close it again without deleting, so a
failed upgrade can never end up half-removed. The purge itself is
unchanged.
* [CrashReporting] Name the holders of a failed purge with Restart Manager
The process-and-module scan could only name a holder whose image lives in the
install directory or that loaded one of its DLLs, so the case this diagnostic
exists for - an antivirus scanner holding a plain handle - was reported as "no
holder named, only a code". Reaching the Restart Manager from the NSIS script
would have meant marshalling RM_PROCESS_INFO through System::Call as a table of
hand-computed offsets, which cannot be checked without a Windows toolchain, and
that is why it was dropped when this diagnostics was written.
Move the call into tools/installer-holder-probe: a small executable the same
Windows build compiles, linking nothing from this project so it still starts
when the installed runtime is what the purge could not replace. It rides inside
the installer and is extracted to $PLUGINSDIR only when the purge has already
failed, so the successful path is unchanged and nothing new is installed.
.ci/compile.sh runs it against a file held with FileShare.None on the Windows
runners, so the lookup is exercised on real Windows instead of only compiled.
* [CrashReporting] Fix the holder probe check and cap the holder line
The probe block ran after the package steps, where the working directory is
already the build dir, so its $BUILD_DIR-prefixed paths named build/build and
the PowerShell file could not be written. Move it ahead of install/package,
where the build dir is still the cwd, and name both files relative to it.
Guard it on MAKE_NO_CLIENT, since the probe is only built with the client.
Cap the rendered holder line at HOLDER_LINE_CHAR_MAX: the count limit alone
does not keep long service names inside the NSIS string that also carries the
abort text and the log path. Whole entries are kept and the rest counted, with
the first holder always shown.
Also run cmake-format on the probe's CMakeLists so the lint job accepts it.
* [CrashReporting] Report a read-only purge survivor and bound the probe check
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* [CrashReporting] Installer log send offer on abort (inc 4)
DSN parsed to CPACK_SENTRY_* (scheme-aware for http self-hosts) before
include(CPack); new SendInstallerLog.ps1.in builds a byte-accurate
Sentry envelope (128 KiB tail cap, X-Sentry-Auth, exit 0/1) with the
abort reason read from the log file itself, so the NSIS side needs no
string functions and quoting stays trivial. AbortInstall asks consent
(attended installs only, silent path untouched, abort never
swallowed). Empty/malformed DSN disables the offer; no secrets in repo.
* [CrashReporting] Embed installer-log sender via a relative NSIS path
The AbortInstall hook embedded SendInstallerLog.ps1 through the absolute
CPACK_SENTRY_PS1_PATH, which carries CMake's forward slashes (D:/a/...).
makensis does not treat a forward-slash drive path as absolute, so it
resolved it relative to project.nsi, found nothing and aborted packaging
on the Windows jobs of every branch in the stack.
Embed it with the same '..\..\..\' relative path that already resolves
NSIS.definitions.nsh: both are configure_file'd into the build root,
three levels up from where CPack places project.nsi. Drop the now-unused
CPACK_SENTRY_PS1_PATH cache variable.
* [CrashReporting] Drop the installer /CRASHTEST hook, tidy the log sender
Remove the /CRASHTEST switch and the matching COCKATRICE_SENTRY_DSN
environment override on the installer abort path. With no deliberate
in-installer trigger left, a package built without a baked DSN must never
offer to post a user's log to an endpoint chosen by whatever happens to be
in the environment, so the prompt is now gated on CPACK_SENTRY_ENABLED
alone. This also retires the comment that claimed /S /R /CRASHTEST reaches
the prompt: the switch was evaluated before /R set ReinstallMode, so that
form never showed it.
Alongside: correct the CPACK_SENTRY_RELEASE comment (PROJECT_VERSION keeps
its "-<label>" on Development tags where CPACK_PACKAGE_VERSION does not),
delete the unused CPACK_SENTRY_HOST/CPACK_SENTRY_PROJECT_ID cache vars,
document the DSN regex's path-prefix fallback, log a declined send, keep
the uploaded installer log free of helper-injected newlines, and bound
the post with -TimeoutSec/-NonInteractive.
* [CrashReporting] Derive the installer DSN vars as plain variables
Review nit: every CPACK_SENTRY_* value is recomputed from
COCKATRICE_SENTRY_DSN on each configure, so CACHE ... FORCE bought
nothing. Set empty defaults once, override them in the match branch, and
drop the duplicated else() copies. Still CPACK_-prefixed, so include(CPack)
still snapshots them into CPackConfig.cmake.
* [CrashReporting] Gate the installer log send at build time
Review nit: test CPACK_SENTRY_ENABLED with the NSIS preprocessor
instead of copying it into $SentryEnabled and branching at runtime, so a
package built without a DSN compiles the prompt out rather than carrying
dead code. The configure_file that produces the helper is gated the same
way, so a build without reporting neither references nor embeds it.
* [CrashReporting] Disclose the Windows user name in the send prompt
Review nit: say that the log can include the Windows user name (a real
part of what gets uploaded) and drop the "No account needed." clause,
which answered a question the prompt never raised.
* [CrashReporting] Show progress while the installer log is sent
Review nit: DetailPrint a line before nsExec runs, so the details pane
says something while the send is in flight instead of staying blank for
up to the 60 s /TIMEOUT.
* [CrashReporting] Rename the Sentry event local in the installer helper
Review nit: $Event is a PowerShell automatic variable, so name the local
$sentryEvent instead. Also drop $firstLine: Select-String returns the
matching line alone, so the reason is already one physical line and the
extra split added nothing.
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* [CrashReporting] Mock dialog, sentry skeleton and CI prerequisites
Mock: DlgCrashReport preview dialog (no account needed, redacted log
preview), CrashReportingMode Ask/Always/Never setting in DebugSettings,
--simulate-crash flag and Help-menu preview entry. Send writes a local
mock JSON only.
Vendor sentry-native 0.17.3 via FetchContent behind
WITH_CRASH_REPORTING (breakpad everywhere, backend none under
sanitizers) plus a CrashReporter facade with early init and post-loop
shutdown. DSN is a placeholder: empty CMake default with
COCKATRICE_SENTRY_DSN env override, so SDK init is skipped and mock
behavior is unchanged until a real DSN is configured.
CI prerequisites riding here so every layer below builds green:
libcurl dev plus CA bundles in the Linux images and CodeQL env, sentry
Qt integration off (its own find_package cannot see the macOS thin Qt
layout), and the StrFunc Using construction makensis requires.
Tests: ctest green, format clean, code plus UI gates PASS.
* [CrashReporting] Forward mode changes to SDK and fix Linux CI opt-in
Connects PersonalSettings::crashReportingModeChanged to
giveConsent()/revokeConsent() so switching from AlwaysSend to
NeverSend (or Ask) mid-session reaches the SDK instead of leaving
consent given until restart.
Forwards WITH_CRASH_REPORTING into the docker container the way
CCACHE_DIR is, so the Linux package leg actually builds with crash
reporting ON instead of silently building OFF.
* [CrashReporting] Give crash-report logging its own category per review
* [CrashReporting] Drop unused DebugSettings constructor visibility per review
* [CrashReporting] Document reporting modes on the enum per review
* [CrashReporting] Match the dump scan to the writer's temp fallback
Review nit: when the handler's SHGetKnownFolderPath lookup fails it writes to
<temp>/../.., but the startup scan looked in QDir::tempPath(), so a dump
produced through the fallback would never be found and its crash would never
offer to be reported. Walk up the same two levels in the scan.
The path is Windows-only (#ifdef Q_OS_WIN); the statements were compiled and
run standalone against Qt 6 to confirm the API use.
* [CrashReporting] Complete the dump-scan fallback path
The previous commit walked the scan's fallback up two levels but dropped the
CrashDumps/Cockatrice suffix that the writer appends unconditionally, so a dump
written through the fallback was still invisible to the scan.
Evaluate the writer's expression verbatim rather than mirroring it with QDir:
cdUp() is existence-checked and silently leaves the path alone when the
directory is missing, parent_path() is not, and QDir::absoluteFilePath() joins
the shared suffix without risking a doubled separator when the walk lands on a
root. Verified by compiling the block verbatim against Qt 6 with -Wextra
-Werror and comparing it with the writer's join for both the normal and the
fallback case.
Also record that %LOCALAPPDATA% is the scan's stand-in for
FOLDERID_LocalAppData and why the two normally agree.
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
CMAKE_GENERATOR_PLATFORM is Visual-Studio-only and stays empty under the
Ninja generator that Windows CI switched to in #7380, so TRICE_IS_64_BIT
was always 0 and every Windows artifact was mislabeled 32-bit. That fired
the installer's 'installing a 32-bit version on a 64-bit system' warning
and routed a genuinely 64-bit build into Program Files (x86) with the
32-bit registry view.
Use CMAKE_SIZEOF_VOID_P, which reflects the actual target architecture
for every generator (same test FindVCredistRuntime.cmake already uses).
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* [Tests] Register each test case with CTest instead of each test binary
Registering a test cost four edits across a dozen CMakeLists files: add_test,
add_executable, a conditional add_dependencies on gtest, and target_link_libraries.
The conditional branch was the trap -- it is only needed when GTest was fetched and
add_subdirectory'd, and omitting it yields undefined gtest symbols at link time --
so it was easy to leave out and impossible to notice until the link failed.
Fold all four steps into cockatrice_add_test() and migrate every test directory
onto it, with a cockatrice_add_manual_test() wrapper for the three benchmarks that
are built but deliberately kept out of ctest. The calls now read as a declaration
of what a test is rather than a transcript of how to register one.
While rewriting the registrations, replace add_test(NAME x COMMAND x) with
gtest_discover_tests(). Naming the binary as a single CTest entry meant a CI failure
reported only which binary failed, and ctest -R Suite.Case could not select an
individual case. Discovery raises the suite from 35 opaque entries to 469 real ones:
ctest -R ThemeMigrationTest.IsIdempotent now runs exactly that case, and
ctest -L server / -LE bench work off labels applied at registration.
Give every test a 60s TIMEOUT. CTest's implicit default is 1500s, so one hung
socket wait would stall CI for 25 minutes, and single_instance_manager_test already
does real QLocalSocket waits.
Dropping per-case granularity costs wall-clock time, since the suite becomes
process-spawn bound rather than CPU bound: serial ctest went from 3.8s to 30.8s. Run
it with -j in CI to recover it -- 7.6s at -j4, and 3.5s at -j8, both at or below
the old serial time. The binaries are parallel-safe: the only test claiming a fixed
resource scopes its local socket name to its own pid, so concurrent copies cannot
collide.
Also delete dummy_test, which asserted 1 == 1 and only proved the harness was
wired up -- something the other 468 cases already establish.
No test is added, removed, or changed by this commit; the 469 cases are the same
ones the 35 binaries contained.
* [Tests] Make QtTest available to test targets
QtTest was not among the modules test targets could link, so the Qt model and
signal contracts had no way to be verified from a test: QAbstractItemModelTester
and QSignalSpy both live there.
Add Test to _TEST_NEEDED in cmake/FindQtRuntime.cmake. That is the single place the
union of test-only Qt modules is declared, so this is the only file that needs to
change to make the module reachable from every test target.
Nothing uses it yet. This lands the capability ahead of the model-layer tests that
need it, so those can be added as tests alone rather than as test plus build-system
plumbing.
* [Tests] Correct the note on the oracle test compression defines
add_definitions() populates the directory's COMPILE_DEFINITIONS property, which is
consumed at generate time, so it reaches every target in the directory no matter
where it is called relative to add_executable(). The previous note claimed the
flags stayed confined to the benchmark because the find_package block came after
it; oracle_importer_test, created above that block, carries both defines too.
Verified harmless rather than a live bug: HAS_ZLIB and HAS_LZMA gate code only in
oracle/src/pages.cpp and oracle/src/pagetemplates.cpp, and no test target compiles
either. The ordering and the leaked defines are both pre-existing, so this only
corrects the explanation.
* [Tests] Carry one CTest label per test instead of several
gtest_discover_tests() can hand CTest exactly one label. GoogleTestAddTests.cmake
accumulates its generated script with `foreach(arg ${ARGN})`, so a label list reaches
the CTestTestfile as bare `LABELS ci server`, and CTest reads only the first value of a
property and drops the rest without a word.
The `ci` label that is appended to every registered test was therefore absent from every
test that also asked for `server` or `bench`: `ctest -L ci` reported 400 of 469 cases,
and `ctest --print-labels` never listed a second label anywhere in the build.
Reduce each test to exactly one of `ci`, `server`, or `bench`, which is all the
selections need. The three now partition the suite exactly, 400 + 64 + 5 = 469, and
`ctest -LE bench` still means everything but benchmarks. A second label is now a
configure-time FATAL_ERROR rather than a line that quietly means something else.
Anything orthogonal to a test's kind belongs in a helper flag beside the label, so it
composes with the group instead of competing for the single slot.
Also raise DISCOVERY_TIMEOUT off GoogleTestAddTests' 5s default. A plain debug binary
answers --gtest_list_tests in 0.04s, but that budget is thin once several binaries
discover concurrently behind a -j build, and being over it fails at the *link* step with
a bare "Process terminated due to timeout" that points nowhere near the cause.
* [Tests] Link gtest_main on the fetched-gtest path
The downloaded-gtest branch set GTEST_BOTH_LIBRARIES to `gtest` alone, while
FindGTest sets it to both halves. Every test target links that list rather than
picking an entry point, so on this path nothing provides main() and any test
source that does not define its own fails at link with `undefined reference to
main` pointing at crt1.o, naming neither gtest nor the line responsible.
That stayed latent while every test carried its own four-line main(). A test that
omits it now fails, and the error points at the C runtime rather than at gtest,
which is a long way from the cause. Restoring both halves makes the two paths
agree and drops the boilerplate requirement on new test sources.
The 18 sources that still define main() keep linking: gtest_main.cc defines
nothing but main, so the linker leaves its object in the archive unused rather
than reporting a duplicate.
* [Build] Add an ENABLE_SANITIZERS option for ASan and UBSan
Adds -DENABLE_SANITIZERS, off by default, building with AddressSanitizer and
UndefinedBehaviorSanitizer. The bulk of the work is in the three ways the obvious
implementation silently does the wrong thing.
Probe the flags on the link step, not just the compile step. check_cxx_compiler_flag()
only compiles, so every sanitizer flag comes back unsupported, because linking an ASan
object without -fsanitize=address leaves __asan_init undefined. The result is not a
build failure but a build that appears instrumented and is not, which is worse. Use
check_cxx_source_compiles() with CMAKE_REQUIRED_LINK_OPTIONS instead, and treat an
unsupported sanitizer pair as fatal since the cosmetic flags are allowed to degrade.
Opt vendored sources out per source, through a cockatrice_disable_sanitizers_for()
helper rather than a list of set_source_files_properties() calls at the root. The
helper has to exist because CMake resolves a source-file property against the directory
scope its target was declared in: setting the property from the root with absolute paths
is a no-op, silently, which is why the call sites sit next to the add_library and
add_executable that pull the vendored code in. SFMT, minizip, the LZMA SDK and Qxt SMTP
are excluded. oracle/src/qt-json is in the history but not in the tree, removed in
#7214.
Also mark those sources SKIP_PRECOMPILE_HEADERS. The PCH is built with the sanitizer
flags, so it defines __SANITIZE_ADDRESS__; a translation unit compiled with
-fno-sanitize=all does not and GCC then refuses the PCH, which is a fatal
-Werror=invalid-pch in the servatrice and oracle builds.
-fno-sanitize=vptr is not optional. Qt constructs QObject subclasses into raw storage
and writes the vptr afterwards, so the vptr check reports on essentially every QObject
instantiation, and it has no value here because nothing outside moc-generated code
inherits QObject.
Verified on the resulting build: our own translation units carry ASan and UBSan calls,
SFMT.o carries neither, and the full suite of 464 cases passes clean under
ASAN_OPTIONS=detect_leaks=0:abort_on_error=1 with UBSAN_OPTIONS=halt_on_error=0.
The test suite needs two adjustments to make this option usable at all, both of which
land here because they are consequences of COCKATRICE_SANITIZERS existing rather than
independent changes.
DISCOVERY_TIMEOUT rises from 60s to 300s under sanitizers. An instrumented binary takes
2.8s to answer --gtest_list_tests against 0.04s uninstrumented, and discovery runs
several binaries at once behind a -j build.
One target is excluded via a NO_SANITIZE flag, server_developer_role_test. Reaching
google::protobuf::TextFormat::Printer's destructor aborts on an absl assertion inside
system protobuf 33.4 whenever the consumer translation unit is built without NDEBUG,
which a Debug sanitizer build is. This reproduces with no Cockatrice code at all, just
"TextFormat::Printer p; p.PrintToString(msg, &out);" built with -fsanitize=address, so it
is upstream rather than ours. It is a flag and not a fourth label because "skip this in
the sanitizer leg" has to compose with a test's group rather than compete for the single
label slot, and because a flag excludes by not registering the cases at all, which
cannot rot the way a label silently can. The target still builds and still runs on the
legs without sanitizers.
* [CI] Run the suite once under ASan and UBSan
Adds --sanitize to .ci/compile.sh, mirroring --test, and turns it on for one entry of
the existing Linux Docker matrix: Ubuntu 26.04, which already had a test-enabled leg.
That reuses the image, the ccache layer and the existing step shape, so the CI diff is a
matrix key plus the argument plumbing. Debian 13 and Fedora 44 keep covering the
uninstrumented suite, so nothing is lost by instrumenting one leg.
Sanitizers stay off the release and package legs, which build with --release for
packaging: instrumenting those would change the shipped binaries' optimization profile
and build time for no benefit. macOS and Windows stay uninstrumented too, since ASan is
unreliable against the Qt build there and MSVC has no equivalent without /fsanitize=address.
--sanitize does not imply --test, so an instrumented build can be produced locally
without paying for ctest. When both are given, ctest runs with detect_leaks=0 because Qt
and the sanitizer runtimes both leak at exit by design and LeakSanitizer's default buries
real findings under hundreds of process-teardown reports, and with UBSan halt_on_error=0 so
one run collects many findings instead of stopping at the first. ASan keeps
abort_on_error=1, since a bad access leaves the process unusable and continuing only
produces a second, more confusing report from the wreckage.
* [CI] Publish a gcovr line-coverage report from the test run
Sanitizers tell us what a run did wrong; they say nothing about what it never
reached. This adds the other half: an instrumented build whose report says which
of our own lines no test executes.
Add an ENABLE_COVERAGE option that compiles with --coverage -O0 -g, mirroring the
ENABLE_SANITIZERS opt-in including the per-source carve-out that keeps vendored
trees out of the denominator. -O0 is deliberate: at higher optimisation levels GCC
merges and relocates lines, so the report would describe code that no longer exists
in that form. Only GCC is supported, since Clang's intermediate format needs
llvm-cov; asking for coverage under Clang is a hard error rather than a build that
silently produces unreadable .gcno files.
Keep the filters in a committed gcovr.cfg rather than in the invocation. The tests
themselves are excluded because they are compiled with the same instrumentation
and are only ever executed by the run doing the measuring, so leaving them in would
report a healthier project than we have. The process entry points and the client's
window chrome go too: each needs a display, an interactive terminal or a live
database before it does anything a headless test could observe, so they would
otherwise sit at 0% forever and bury real movement.
Give coverage its own matrix leg rather than a second flag on the sanitizer leg.
The two answer different questions, and combining them slows both down without
telling us more. The new variant key gives the leg its own Docker image and ccache
entry so the two legs cannot race on the same tag.
The leg publishes an HTML report with per-file pages, LCOV, Cobertura XML and a
JSON summary. gcovr is a report-time dependency rather than a build dependency, so
a missing one warns and skips instead of failing an otherwise green build.
* [CI] Resolve the coverage source root before the build dir cd
The previous commit computed SRC_ROOT after `cd "$BUILD_DIR"`, under a comment
claiming it ran before. `$0` is relative to the directory the script was invoked
from, so from inside the build dir `dirname "$0"` no longer resolves and the
subshell's `cd` fails. Under `set -e` that is fatal rather than an empty variable,
which took down every leg in the matrix, coverage or not, including the ones this
change does not otherwise touch.
Move it above the cd and guard it on MAKE_COVERAGE, since the coverage report is
the only consumer. Resolving it unconditionally meant a coverage-only detail was
paid for by every build in the fleet, and gave every one of them a way to fail.
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* [Tests] Register each test case with CTest instead of each test binary
Registering a test cost four edits across a dozen CMakeLists files: add_test,
add_executable, a conditional add_dependencies on gtest, and target_link_libraries.
The conditional branch was the trap -- it is only needed when GTest was fetched and
add_subdirectory'd, and omitting it yields undefined gtest symbols at link time --
so it was easy to leave out and impossible to notice until the link failed.
Fold all four steps into cockatrice_add_test() and migrate every test directory
onto it, with a cockatrice_add_manual_test() wrapper for the three benchmarks that
are built but deliberately kept out of ctest. The calls now read as a declaration
of what a test is rather than a transcript of how to register one.
While rewriting the registrations, replace add_test(NAME x COMMAND x) with
gtest_discover_tests(). Naming the binary as a single CTest entry meant a CI failure
reported only which binary failed, and ctest -R Suite.Case could not select an
individual case. Discovery raises the suite from 35 opaque entries to 469 real ones:
ctest -R ThemeMigrationTest.IsIdempotent now runs exactly that case, and
ctest -L server / -LE bench work off labels applied at registration.
Give every test a 60s TIMEOUT. CTest's implicit default is 1500s, so one hung
socket wait would stall CI for 25 minutes, and single_instance_manager_test already
does real QLocalSocket waits.
Dropping per-case granularity costs wall-clock time, since the suite becomes
process-spawn bound rather than CPU bound: serial ctest went from 3.8s to 30.8s. Run
it with -j in CI to recover it -- 7.6s at -j4, and 3.5s at -j8, both at or below
the old serial time. The binaries are parallel-safe: the only test claiming a fixed
resource scopes its local socket name to its own pid, so concurrent copies cannot
collide.
Also delete dummy_test, which asserted 1 == 1 and only proved the harness was
wired up -- something the other 468 cases already establish.
No test is added, removed, or changed by this commit; the 469 cases are the same
ones the 35 binaries contained.
* [Tests] Make QtTest available to test targets
QtTest was not among the modules test targets could link, so the Qt model and
signal contracts had no way to be verified from a test: QAbstractItemModelTester
and QSignalSpy both live there.
Add Test to _TEST_NEEDED in cmake/FindQtRuntime.cmake. That is the single place the
union of test-only Qt modules is declared, so this is the only file that needs to
change to make the module reachable from every test target.
Nothing uses it yet. This lands the capability ahead of the model-layer tests that
need it, so those can be added as tests alone rather than as test plus build-system
plumbing.
* [Tests] Correct the note on the oracle test compression defines
add_definitions() populates the directory's COMPILE_DEFINITIONS property, which is
consumed at generate time, so it reaches every target in the directory no matter
where it is called relative to add_executable(). The previous note claimed the
flags stayed confined to the benchmark because the find_package block came after
it; oracle_importer_test, created above that block, carries both defines too.
Verified harmless rather than a live bug: HAS_ZLIB and HAS_LZMA gate code only in
oracle/src/pages.cpp and oracle/src/pagetemplates.cpp, and no test target compiles
either. The ordering and the leaked defines are both pre-existing, so this only
corrects the explanation.
* [Tests] Carry one CTest label per test instead of several
gtest_discover_tests() can hand CTest exactly one label. GoogleTestAddTests.cmake
accumulates its generated script with `foreach(arg ${ARGN})`, so a label list reaches
the CTestTestfile as bare `LABELS ci server`, and CTest reads only the first value of a
property and drops the rest without a word.
The `ci` label that is appended to every registered test was therefore absent from every
test that also asked for `server` or `bench`: `ctest -L ci` reported 400 of 469 cases,
and `ctest --print-labels` never listed a second label anywhere in the build.
Reduce each test to exactly one of `ci`, `server`, or `bench`, which is all the
selections need. The three now partition the suite exactly, 400 + 64 + 5 = 469, and
`ctest -LE bench` still means everything but benchmarks. A second label is now a
configure-time FATAL_ERROR rather than a line that quietly means something else.
Anything orthogonal to a test's kind belongs in a helper flag beside the label, so it
composes with the group instead of competing for the single slot.
Also raise DISCOVERY_TIMEOUT off GoogleTestAddTests' 5s default. A plain debug binary
answers --gtest_list_tests in 0.04s, but that budget is thin once several binaries
discover concurrently behind a -j build, and being over it fails at the *link* step with
a bare "Process terminated due to timeout" that points nowhere near the cause.
* [Tests] Link gtest_main on the fetched-gtest path
The downloaded-gtest branch set GTEST_BOTH_LIBRARIES to `gtest` alone, while
FindGTest sets it to both halves. Every test target links that list rather than
picking an entry point, so on this path nothing provides main() and any test
source that does not define its own fails at link with `undefined reference to
main` pointing at crt1.o, naming neither gtest nor the line responsible.
That stayed latent while every test carried its own four-line main(). A test that
omits it now fails, and the error points at the C runtime rather than at gtest,
which is a long way from the cause. Restoring both halves makes the two paths
agree and drops the boilerplate requirement on new test sources.
The 18 sources that still define main() keep linking: gtest_main.cc defines
nothing but main, so the linker leaves its object in the archive unused rather
than reporting a duplicate.
* [Build] Run the test suite under ASan and UBSan (#7400)
* [Build] Add an ENABLE_SANITIZERS option for ASan and UBSan
Adds -DENABLE_SANITIZERS, off by default, building with AddressSanitizer and
UndefinedBehaviorSanitizer. The bulk of the work is in the three ways the obvious
implementation silently does the wrong thing.
Probe the flags on the link step, not just the compile step. check_cxx_compiler_flag()
only compiles, so every sanitizer flag comes back unsupported, because linking an ASan
object without -fsanitize=address leaves __asan_init undefined. The result is not a
build failure but a build that appears instrumented and is not, which is worse. Use
check_cxx_source_compiles() with CMAKE_REQUIRED_LINK_OPTIONS instead, and treat an
unsupported sanitizer pair as fatal since the cosmetic flags are allowed to degrade.
Opt vendored sources out per source, through a cockatrice_disable_sanitizers_for()
helper rather than a list of set_source_files_properties() calls at the root. The
helper has to exist because CMake resolves a source-file property against the directory
scope its target was declared in: setting the property from the root with absolute paths
is a no-op, silently, which is why the call sites sit next to the add_library and
add_executable that pull the vendored code in. SFMT, minizip, the LZMA SDK and Qxt SMTP
are excluded. oracle/src/qt-json is in the history but not in the tree, removed in
#7214.
Also mark those sources SKIP_PRECOMPILE_HEADERS. The PCH is built with the sanitizer
flags, so it defines __SANITIZE_ADDRESS__; a translation unit compiled with
-fno-sanitize=all does not and GCC then refuses the PCH, which is a fatal
-Werror=invalid-pch in the servatrice and oracle builds.
-fno-sanitize=vptr is not optional. Qt constructs QObject subclasses into raw storage
and writes the vptr afterwards, so the vptr check reports on essentially every QObject
instantiation, and it has no value here because nothing outside moc-generated code
inherits QObject.
Verified on the resulting build: our own translation units carry ASan and UBSan calls,
SFMT.o carries neither, and the full suite of 464 cases passes clean under
ASAN_OPTIONS=detect_leaks=0:abort_on_error=1 with UBSAN_OPTIONS=halt_on_error=0.
The test suite needs two adjustments to make this option usable at all, both of which
land here because they are consequences of COCKATRICE_SANITIZERS existing rather than
independent changes.
DISCOVERY_TIMEOUT rises from 60s to 300s under sanitizers. An instrumented binary takes
2.8s to answer --gtest_list_tests against 0.04s uninstrumented, and discovery runs
several binaries at once behind a -j build.
One target is excluded via a NO_SANITIZE flag, server_developer_role_test. Reaching
google::protobuf::TextFormat::Printer's destructor aborts on an absl assertion inside
system protobuf 33.4 whenever the consumer translation unit is built without NDEBUG,
which a Debug sanitizer build is. This reproduces with no Cockatrice code at all, just
"TextFormat::Printer p; p.PrintToString(msg, &out);" built with -fsanitize=address, so it
is upstream rather than ours. It is a flag and not a fourth label because "skip this in
the sanitizer leg" has to compose with a test's group rather than compete for the single
label slot, and because a flag excludes by not registering the cases at all, which
cannot rot the way a label silently can. The target still builds and still runs on the
legs without sanitizers.
* [CI] Run the suite once under ASan and UBSan
Adds --sanitize to .ci/compile.sh, mirroring --test, and turns it on for one entry of
the existing Linux Docker matrix: Ubuntu 26.04, which already had a test-enabled leg.
That reuses the image, the ccache layer and the existing step shape, so the CI diff is a
matrix key plus the argument plumbing. Debian 13 and Fedora 44 keep covering the
uninstrumented suite, so nothing is lost by instrumenting one leg.
Sanitizers stay off the release and package legs, which build with --release for
packaging: instrumenting those would change the shipped binaries' optimization profile
and build time for no benefit. macOS and Windows stay uninstrumented too, since ASan is
unreliable against the Qt build there and MSVC has no equivalent without /fsanitize=address.
--sanitize does not imply --test, so an instrumented build can be produced locally
without paying for ctest. When both are given, ctest runs with detect_leaks=0 because Qt
and the sanitizer runtimes both leak at exit by design and LeakSanitizer's default buries
real findings under hundreds of process-teardown reports, and with UBSan halt_on_error=0 so
one run collects many findings instead of stopping at the first. ASan keeps
abort_on_error=1, since a bad access leaves the process unusable and continuing only
produces a second, more confusing report from the wreckage.
* [Build] Fix sanitizer flag leak into release package and make UBSan fail CI
- compile.sh always passes -DENABLE_SANITIZERS explicitly so the
release-package reconfigure in the same build dir cannot inherit ON
from the sanitized debug step (which shipped libasan/libubsan DEB deps).
- Sanitizers.cmake resets COCKATRICE_SANITIZERS to OFF when the option is
off (CACHE INTERNAL otherwise sticks forever, keeping NO_SANITIZE skips
and the 300s discovery timeout).
- UBSan now fails the job: -fno-sanitize-recover=undefined at compile time
plus UBSAN_OPTIONS halt_on_error=1 (recover mode exits 0, so the old
halt_on_error=0 could never fail).
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* [Windows] Purge every runtime DLL on upgrade instead of a named few
A user updating from 3.0.2 to 3.1 got "abseil_dll.dll is either not designed
to run on Windows or it contains an error" and could not start the client.
abseil_dll.dll comes from vcpkg, not Qt. It rides into the install directory
on the "*.dll" glob in cockatrice/CMakeLists.txt, alongside libprotobuf.dll,
liblzma.dll and z.dll, and none of those were named in the upgrade purge. So
on an upgrade the old uninstaller's RMDir /r skipped whichever of them was
locked, the purge did not look for them, HasStaleRuntimeFiles did not look
for them either, and the installer reported success - having written the new
executables next to the previous version's protobuf runtime. Windows then
refused to load it.
The mask list was also already wrong where it existed: it matches zlib*.dll
while vcpkg ships z.dll.
Delete every top-level DLL instead of naming library families. The payload is
written after the purge, so a DLL the new build still needs is put straight
back; what the purge has to guarantee is only that nothing from the previous
build is left to be loaded next to the new executables. Enumerated families
cannot do that - every transitive dependency that arrives between two
releases is one more name to forget, and the three previous rounds of this
work each reasoned about Qt because that is what the reports named at the
time.
HasStaleRuntimeFiles collapses to the matching *.dll probe, so the abort
fires on any survivor rather than only on the families it was told about.
IsExistingCockatriceInstall stays on Qt6*.dll on purpose: it decides whether a
directory is ours at all, and a folder that merely holds some DLLs of its own
must not be purged. Qt6*.dll is enough even when only the protobuf runtime
survived, because the whole Qt set is mapped by the same process that maps
abseil_dll.dll and so is locked or free with it.
* [CI] Assert the whole Windows runtime, not just the Qt modules
required-qt-runtime.txt listed only the Qt modules COCKATRICE_QT_MODULES
names, so it could not see any other runtime go missing. The three previous
rounds of installer work all reasoned about Qt*.dll because that was all the
build knew how to ask about, and the one runtime that actually broke the
reported upgrade - abseil_dll.dll - was outside the set entirely.
- Rename it to required-runtime.txt and take the third party runtime names
from the vcpkg bin directory the app-local copies come from, so a
dependency bump cannot leave the list behind and a versioned name such as
libcrypto-3-x64.dll becoming libcrypto-4-x64.dll cannot either
- Union the three applications' module lists rather than the client's alone:
Servatrice contributes Qt6Sql.dll, servatrice.exe is installed next to
cockatrice.exe and cannot start without it, and nothing asserted it
- Add Core to the client's module list. It is the one module nothing can do
without and it only ever arrived transitively, so Qt6Core.dll was shipped
and never checked - the new unknown-runtime check flagged it on the first
run
- Fail the build on a missing runtime, as before, and warn on a runtime
nothing requires. Only the test framework is a hard failure there: "no
shipped executable links gtest" is a fact, whereas a runtime missing from
the manifest may be one the manifest failed to derive, and a check that has
never run before should not be able to block a release
The warning is meant to be read. Qt6::Quick pulls in Qml, QmlModels,
QmlWorkerScript, QuickBasic, QuickTemplates2 and OpenGL transitively and none
are named, so the manifest is known to be a partial list; the first Windows
run reports the real one, which is evidence rather than a guess about what Qt
links. Fold that back in when it lands.
* [CI] Stop deriving the required runtime from the vcpkg bin directory
The first Windows run of this check failed, and the seven files it named as
missing are not runtime at all: gmock.dll, gmock_main.dll, legacy.dll,
libprotobuf-lite.dll, libprotoc.dll and pthreadVC3/VCE3/VSE3.dll. None of them
is linked by any shipped executable, so a correct package leaves them out.
vcpkg_installed/x64-windows/bin is everything vcpkg installed, not everything
the applications import. The protoc compiler, the GoogleMock library, a port
tool and three CMake FindThreads probes sit beside the genuine runtimes, and a
directory listing cannot tell them apart. Asserting it fails the build over a
package that is right - which is what happened, and it is the same mistake as
the one that produced this bug report: reasoning about a list of names instead
of about the build.
The real source of truth is the three executables' import tables, which
fixup_bundle already walks to decide what to copy. Until CMake can read that
result, the Qt half is what can be derived honestly, from the same module lists
the targets are built against. So assert that, and report the rest rather than
require it:
- Drop the vcpkg bin glob, its gtest filter and the vcpkg-runtime-known marker.
All three existed only to support the glob.
- Report top level DLLs the manifest does not name without failing. These are
expected: protobuf, Abseil, OpenSSL, liblzma, ffmpeg and the Qt debug DLLs all
arrive on the build output glob because nothing in the build declares them by
name. The list is a report of what the package carries, not a verdict, and it
is how the missing Qt6Qml/Quick/OpenGL entries above became visible.
- Report a leaked gtest or gmock instead of failing on it. "No shipped
executable links this" is a fact, but a check that has never passed on a real
package has not earned the right to block a release, and the first run of an
enforcing version is exactly when its own wrong assumptions show up.
- Skip DLLs under a subdirectory, which are platform plugins rather than
imported runtimes, and the NSIS plugins, which extract to $PLUGINSDIR and so
reach the comparison looking like payload.
Verified against the paths 7-Zip actually listed in the failing package: the
check passes on it, still fails when Qt6Sql.dll or the qwindows plugin is
removed, and still reports gtest.dll and gmock.dll if either is added.
The purge in cmake/NSIS.template.in covers the runtimes the manifest does not
name. That fix is independent of any list and is what makes the reported update
failure go away.
* [Installer] Fix the /R upgrade aborting before it does anything
WaitForLauncherToExit read the wrong registers. System.dll writes its
.rN outputs to $N and its .RN outputs to $RN, but the wait used
p.r0/i.r1 while testing $R1. $R1 therefore stayed empty, and the "did
not exit" test compared "" with "0" - true every time. Every in-app
update passes /PID=, so every upgrade hit AbortInstall before writing
anything; because /R also sets silent, the override just vanished with
exit code 1 and no message. The bug came in with #7372 and is inherited
by this branch, not introduced by it.
Use the uppercase .RN forms, and treat a null or plugin-failed
OpenProcess as the launcher already being gone - the case the old
${Errors} check never caught, because System.dll does not set the NSIS
error flag from the callee's return value.
Also restore the abort dialog for the in-app updater: AbortInstall and
the two inline warnings gated their message box on ${IfNot} ${Silent},
which is false for a /R upgrade, so any abort on that path was equally
silent. The unattended case is a /S install without /R, not /R, so show
the box for $ReinstallMode = 1 or an interactive install. The
stale-runtime abort now goes through AbortInstall so that decision
lives in one place.
* [Installer] Log every upgrade step to %TEMP%\Cockatrice-installer.log
* [Installer] Actually append to the upgrade log (FileSeek END)
* [Installer] Wait for the old uninstaller before replacing files
An NSIS uninstaller copies itself to %TEMP% and runs from there so that it
can delete its own directory; the process the installer started exits
immediately. A plain ExecWait therefore returns before the uninstall has
run, and the temp copy's "RMDir /r $INSTDIR" then executes concurrently
with the install section, deleting files the payload just wrote. That is
why an upgrade could leave Qt6Core/Gui/Multimedia and the other
alphabetically-first DLLs missing while the tail of the directory survived:
a recursive delete marching from the front while the final listing ran.
Pass _?=$INSTDIR to the old uninstaller so it runs in place and ExecWait
actually blocks until the uninstall completes. _?= must be the last
argument and unquoted, even for paths with spaces. Applied to both the
silent /R path (AutoUninstallIfNeeded) and the interactive path
(componentsPagePre).
* [Installer] Tighten the wording of the upgrade comments
Drop the dash-heavy phrasing from the comments this PR adds, and say plainly
in the _?= comment that ExecWait returns before the old uninstaller has run
and the temp copy then deletes the payload the section is still writing.
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* [Installer] Abort instead of installing over leftover runtime files
The silent upgrade could leave a mix of old and new Qt DLLs next to the new
executables, which is what makes the next start fail with "The procedure entry
point X could not be located in the dynamic link library ...Qt6QuickWidgets.dll"
or "The code execution cannot proceed because Qt6Multimedia.dll was not found".
The check that was supposed to catch this probed only Qt6Core.dll and
Qt6Network.dll, so it was blind to the two files the reports name. It also only
warned and then installed anyway, so a caught problem was still compounded.
- Scan every file the purge targets with FindFirst, which takes the same
wildcards the Delete/RMDir calls use, instead of two hardcoded names
- Widen IsExistingCockatriceInstall from Qt6Core.dll to Qt6*.dll so a directory
left holding only some Qt modules is still recognised as an install
- Gate the scan on that, so a first-time install into a directory that happens to
hold unrelated Qt5 DLLs is not turned into an abort
- Abort with a non-zero exit code instead of warning, so the new payload is
never written next to files of the previous version
- Skip the message box on an unattended /S install, which would block on it
forever, matching the two existing abort sites
* [Installer] Cancel a silent upgrade when the running-instance guard fails
The running instance guard is what stands between a locked install directory
and the mixed Qt DLL set that breaks the next start, but it failed open in two
places and let the install carry on anyway:
- powershell.exe could not be started: the check warned and returned
- the process lookup answered "could not determine": WaitForAppToClose returned
on $R0 != 1, so "unknown" was treated exactly like "not running"
On a silent upgrade there is nobody at the keyboard to close the applications in
response to that warning, which is precisely the state in which the launcher was
still holding its Qt DLLs mapped.
- Treat "could not determine" as its own state rather than folding it into "not
running", in all three places WaitForAppToClose inspects the result
- Cancel a silent upgrade in both cases; keep an interactive install on the
existing warn-and-continue path, since refusing there would leave an installer
that can never complete and the stale-runtime check still applies
- Route every abort through one AbortInstall so the dialog-vs-silent decision
lives in a single place, and stop the two pre-existing warning dialogs that a
/S install without /R would have blocked on forever
* [Installer] Wait on the process that launched the update installer
The in-app updater starts the installer and only then quits itself, so the
installer has to decide whether the launching process is still alive while it is
running. It did that by polling powershell.exe for an image name under the
install directory, which is a proxy for the real question - is the process that
started me still holding its Qt DLLs mapped - and a proxy that can answer "not
running" or "could not determine" while the process is very much alive. Either
answer let the install carry on into the mixed Qt DLL set.
Take /PID= from the command line and wait on that process handle directly:
- OpenProcess with PROCESS_SYNCHRONIZE only, which is all that is needed to wait
and the least likely to be refused for an unrelated reason
- WaitForSingleObject for the same 60 s the name based check allows, so a client
slow to flush its card database is not squeezed
- Treat an unopenable handle as "already exited", which is what it means for a
dead process
- Cancel the upgrade if the launcher has not exited, rather than replacing files
it is still using
Run it before EnsureAppsNotRunning, not after: it needs no powershell.exe, so it
is the one check that still works on a machine where the process lookup cannot
run, and it is what lets that case keep upgrading instead of being cancelled by
what was only ever a proxy for it. Keep an older client, which passes no /PID=,
on the existing path.
An older installer ignores /PID=, and /D= stays the last parameter either way.
* [Client] Pass our process id to the update installer
The updater starts the installer and only then quits itself, so the installer
begins replacing files while this process still has every Qt runtime DLL mapped,
and a mapped DLL cannot be deleted. It has to work out that it must wait, and
until now all it had was its own image name to go on.
Pass /PID= so it can wait on this process directly instead of guessing, and
cancel the update rather than replace files this process is still using. /D=
stays the last parameter, which NSIS requires.
* [CI] Fail the build when the installer is missing a Qt runtime DLL
The Windows payload is assembled by globbing every *.dll out of the build output
directories, so nothing but luck decides whether a module the client links
statically - Qt6Multimedia.dll, Qt6QuickWidgets.dll - reaches the installer at
all. A module that is left out is invisible until a user installs an update and
the client then refuses to start, with "The code execution cannot proceed because
Qt6Multimedia.dll was not found" or a missing procedure entry point in a
Qt6QuickWidgets.dll carried over from the previous version.
Derive the required list from SERVATRICE_QT_MODULES, COCKATRICE_QT_MODULES and
ORACLE_QT_MODULES, the same module lists the targets are actually built against,
so it cannot drift from what the binaries need, and write it next to the CMake
cache as required-qt-runtime.txt. Add the platform plugin, without which the
client cannot start at all.
Assert it in the existing 7-Zip pass that already rejects build-tree artifacts,
comparing exact relative paths so a same-named file in the wrong directory does
not satisfy a requirement, and normalizing the archive's own separators so it
does not matter which one NSIS chose. Missing manifest, missing entry or missing
7-Zip-independent failures all stop the build rather than warn, which is the
whole point: the previous check already failed hard for artifacts, and a warning
here is what let a broken package ship.
* [Client] Point the Windows qt.conf translations path at the real one
The Windows qt.conf carried Translations = Resources/translations, copied from
the macOS bundle block, but the Windows install has no Resources directory: the
.qm files are installed to COCKATRICE_WIN32_QM_INSTALL_DIR, which is
translations, and main.cpp looks for them at applicationDirPath() +
"/translations". Leave the macOS value alone, which is correct for it.
* [Installer] Close the /? help string so the installer compiles again
The last line of the continued MessageBox string ended in $\n\" instead of $\n".
NSIS reads \" as an escaped quote, so the string never terminated and makensis
rejected the script, which took the CPack packaging step down with it. The
Windows build failed on "Problem running NSIS command" before ever reaching the
new required-runtime check.
* [Installer] Give FindFirst the two arguments it takes
FindFirst takes the pattern and the output variable and leaves the search
handle in $0. There is no handle argument, and FindNext taking only a variable
is the giveaway: the handle has to live somewhere implicit, and it is $0.
The call passed three arguments, so makensis rejected the script and the CPack
packaging step failed with "Problem running NSIS command" before ever reaching
the required-runtime check.
This was not only a compile error. The handle variable was never populated, so
the stale-runtime check that commit 844a1a9b added could never see a match: it
probed $StaleFindHandle, which stayed empty, and the widened wildcard scan over
the purge set was a no-op. Pass the pattern, test $0, and close $0. No FindNext
is needed, because $R0 is a yes/no flag rather than a list.
Drops the now unused StaleFindHandle Var.
* [CI] Dump the NSIS compile log when packaging fails
CPack reports a failed makensis run as "Problem running NSIS command" plus a
path to NSISOutput.log, and that log exists only on the runner, so the actual
compile error never reaches the workflow output. An installer script that does
not compile is therefore undiagnosable from CI alone, which is exactly how a
three-argument FindFirst cost two build cycles here.
* [CI] Run the NSIS log dump under bash
The dump step used the default pwsh shell, where `find` resolves to Windows'
text-search find.exe rather than a file finder, so it died with "FIND:
Parameter format not correct" instead of printing anything. Pin shell: bash.
* [Installer] Test the error flag with LogicLib's ${Errors} condition
makensis rejected the script with "macro _If requires 4 parameter(s), passed
2" at the ClearErrors/OpenProcess pair. A ${If} condition cannot be a bare
$Var: LogicLib needs either a comparison or one of its flag macros, and a lone
$Errors splits into too few arguments for _If. ${Errors} is the documented flag
condition and is the same mechanism as the ${Silent} test already used here.
The dump step earned its keep - this took one run to locate rather than another
round of guessing.
* [Installer] Give FindFirst its handle back and stop blocking silent aborts
makensis: "FindFirst expects 3 parameters, got 2. Usage: FindFirst
$(user_var: handle output) $(user_var: filename output) filespec".
The previous commit was wrong about this. FindFirst does take the handle
explicitly, and FindNext finds it in $0 as a side effect of the FindFirst that
set it, which is why FindNext takes only a variable. The three-argument form the
stale-runtime check was written with was correct all along, and the two-argument
form broke it. Restored, along with Var StaleFindHandle.
That same commit inverted the interactive check in both abort paths.
`${If} $ReinstallMode = 1` with `${OrIfNot} ${Silent}` shows the message box
exactly when the install is the silent /R upgrade - the one case the comment
says must never block, since nothing will dismiss it and the installer hangs
until it is killed. Replaced with `${IfNot} ${Silent}`, so the box appears only
for an interactive install and a silent abort goes straight to SetErrorLevel.
Audited every construct added by this branch against master, which compiles:
all 9 LogicLib condition shapes have a master precedent, FindFirst is the only
FindFirst and is back to 3 arguments, Function/Section and block depth balance,
and the 12 lines with an odd number of quotes are the same pre-existing string
continuations master has.
* [CI] Strip the CR 7-Zip leaves on its paths before comparing them
The installer check reported all 14 required files missing, including
Qt6Core-era basics an 82 MB package obviously contains, so the payload was fine
and the comparison was broken. 7-Zip writes CRLF on Windows, sed 's/^Path = //p'
kept the \r on every line, and grep -qxF matches whole lines - so every entry
carried a trailing \r and nothing matched. The build-tree artifact check next to
it uses an unanchored grep -E and was unaffected, which is what made the two
checks disagree.
Reproduced locally against a 7z archive with CRLF injected: the old pipeline
matches nothing, the new one finds the present files and reports only the
genuinely absent one.
Also match case-insensitively. That is how the installed tree behaves - the
uninstaller clears both $INSTDIR\plugins and $INSTDIR\Plugins - so a
Plugins vs plugins difference in the archive is not a missing file.
* [CI] Print both sides of the installer path comparison, and strip its prefixes
The check still reported all 14 files missing after the CR fix, so the CR was
not the cause and the real difference between 7-Zip's output and the manifest
is still unknown. Inferring it again is what produced two wrong fixes in a row,
so on failure the check now prints the required list and the first 40 paths
7-Zip lists. A bare "missing" line cannot distinguish a missing file from a
misread listing, and twice it was the latter.
Also strip $INSTDIR/, $OUTDIR/, $PLUGINSDIR/ and ./ prefixes from the listed
paths. Written as separate -e expressions rather than one alternation: with |
as the sed delimiter an escaped \| is a literal pipe, so the alternation
version silently matched nothing at all.
Verified against a 7z archive fed through seven path shapes - plain, Windows
backslashes with CRLF, each prefix, lowercase plugins, and all of them combined
- each reporting only the file genuinely absent from the archive.
* [CI] Strip CR from the manifest side of the installer path check
The dump added last run settled it: 7-Zip listed 209 paths including Qt6Gui.dll,
Qt6Multimedia.dll and Plugins/platforms/qwindows.dll verbatim, as plain relative
paths with no prefix - so the archive side was already clean and the previous
fix was right to strip CR there but not sufficient.
The trailing CR was on the other side. CMake writes required-qt-runtime.txt
with the platform line ending, so on Windows $entry arrived as
"Qt6Gui.dll\r", which cannot whole-line match a clean "Qt6Gui.dll". Every
entry then read as missing while the listing held it, which is precisely what
the two sides of the dump showed.
Reproduced locally with a CRLF manifest against a 7z archive: without the strip
every entry is reported missing, with it only the genuinely absent file is.
Checked across plain paths, Windows backslashes with CRLF, $INSTDIR, $OUTDIR,
$PLUGINSDIR, ./ and / prefixes.
The literal $INSTDIR-style prefixes are written as [$] bracket expressions so
shellcheck does not read them as unexpanded variables; that clears the SC2016
that failed the format job, without a disable directive.
* [CI] Reword the bracket-expression comment so the linter stops parsing it
A comment whose first word is shellcheck is read as a directive, so
"# shellcheck does not read these as unexpanded shell variables" was parsed as
a directive named "does" and failed with SC1073 and SC1072 - which cascaded into
the SC1009 on the else clause above it. The SC2016 it was meant to document is
already gone now that the prefixes are written as [$] bracket expressions, so
the note only had to stop looking like a directive.
* [CI] Indent the diagnostic path lists without sed, and undo comment drift
SC2001 asks for parameter expansion instead of sed 's/^/ /'. Echoing
${manifest//$'\n'/$'\n '} prepends the two spaces directly and keeps the
diagnostic block readable. Both occurrences are converted; only the one reading
from a here-string was reported, but the piped one has the same shape.
Also restore master's wording of the build-tree artifact comment. An earlier
edit inserted "in " ("not the install)" became "not in the install)"). It has no
functional effect, but it was an unintended change to a line this branch has no
reason to touch, and .ci/compile.sh now differs from master only by additions.
* [CI] Ship the Qt module runtimes the Windows package check requires
The installer check reports Qt6Concurrent.dll and Qt6ShaderTools.dll missing
from the Windows package. Both are genuinely absent, not another comparison
artifact - the manifest now matches 12 of 14 entries and the dual-list
diagnostic shows the other two are not in the archive at all.
Nothing deploys them. fixup_bundle copies only the DLLs it finds in
Cockatrice.exe's import table, and the *.dll glob only sees what was already
copied next to the executable. Qt6::Concurrent is linked but never imported,
because QtConcurrent::run and QFutureWatcher are header-only in Qt 6, so the
linker drops the import; Qt6::ShaderTools is skipped the same way. The package
has shipped without them and the client still starts, but that is luck rather
than intent, and required-qt-runtime.txt asks for both.
Install the runtime of every linked module explicitly so the installer contains
what the check demands. An unused DLL costs a few hundred kilobytes; a missing
one is what stops the client starting after an update, and stays invisible until
then.
Derive both the manifest and the install list from the imported target's TYPE,
so a statically linked Qt module - which ships no runtime at all - is skipped by
each and the two can never disagree. Scope the manifest to the client rather
than the union across server, client and Oracle: only the client is packaged on
Windows, and the other two module lists are subsets of the client's, so this
changes nothing today.
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* [Build] Enable ccache by default when it is installed
ccache is a near free win for both clean and incremental rebuilds and
has no effect on systems where it is not installed (find_program
guards the whole block). Aligns the CMake default with the documented
behavior; users can still arch with -DUSE_CCACHE=OFF.
* [Build] Disable ccache auto-engage on Windows (MSVC)
* [Build] Report ccache skip on Windows explicitly
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* [Build] Add precompiled headers for Qt-backed executables
Reparsing QtCore/QtGui/QtWidgets/QtNetwork in ~460 client translation
units is the dominant compilation cost. Precompile the two common layers:
- qtcore_pch.h (Qt Core only; safe even for headless Servatrice)
- qtwidgets_pch.h (adds Gui/Widgets/Network; used by Cockatrice and Oracle)
target_precompile_headers() requires CMake 3.16, now the project minimum.
Estimated 30-50% faster client rebuilds.
* [Build] Format qtwidgets precompiled header
clang-format include regrouping and a missing trailing newline.
* [Build] Add PCH-aware ccache sloppiness config; format cmake/pch headers
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* [Security] Use a CSPRNG for salts, tokens, and RNG seeding
Password salts and activation tokens were generated with the global SFMT
RNG, which was seeded from a 32-bit timestamp, making registration
salts and activation tokens predictable. The game RNG used the same
timestamp seed across restarts.
Add CryptoUtil backed by OpenSSL RAND_bytes and use it for salt/token
generation and to seed RNG_SFMT with a 64-bit CSPRNG value in both the
client and server. Link libcockatrice_utility against OpenSSL::Crypto.
Took 30 seconds
Took 25 minutes
* Lint.
Took 4 minutes
Took 36 seconds
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
Pass -pipe so GCC/Clang transfer intermediate representation between
compiler stages over pipes instead of temporary files, reducing build I/O.
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
3.16 is already required by Qt6 (and enforced at find_package time).
This unlocks native target_precompile_headers(), better AUTOMOC/AUTORCC
handling, and qt6_finalize_project() without a version guard.
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* [CI] Remove Qt5
Took 10 minutes
Took 9 minutes
* Revert CI failure and fix up comments
Took 4 minutes
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* fix(cmake): guard filter_string_test behind WITH_ORACLE or WITH_CLIENT. filter_string_test links against libcockatrice_filters, which is only built when WITH_ORACLE or WITH_CLIENT is enabled. Without this guard, test-only builds fail at configure time because the target doesn't exist. The guard condition mirrors the one in the root CMakeLists.txt that controls whether libcockatrice_filters is built.
* fix(cmake): centralize TEST_QT_MODULES in FindQtRuntime.cmake Each test CMakeLists.txt was independently defining TEST_QT_MODULES with its own subset of Qt modules. This duplicated knowledge that already lives in FindQtRuntime.cmake (which handles module discovery for all other targets: SERVATRICE, COCKATRICE, ORACLE). Consolidate into a single definition using the union of all test requirements (Concurrent Network Svg Widgets), matching the existing pattern for application-target modules. This ensures test-only builds (-DTEST=ON without application targets) discover all necessary Qt components.
* fix(cmake): guard libcockatrice_network behind application targets. libcockatrice_network is only needed by the client, server, and oracle targets. Other application-specific libraries (settings, models, filters) already have similar guards. This was an oversight that caused test-only builds to fail when network dependencies weren't available.
* attempts to fix the problem
* add test to show the problem
* fix workflow
* move logger to cockatrice
* more attempts
* undo stuff
* mark different libraries as gui
* fix SC2145
* rename servatrice only build
* Split filters into libraries where applicable.
Took 23 minutes
Took 2 minutes
* Include filter string.
Took 5 minutes
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* Have CardDatabase::getPreferredPrintingInfo respect card provider ID overrides (pinned printings)
Took 13 minutes
Took 37 seconds
Took 10 seconds
Took 10 seconds
# Commit time for manual adjustment:
# Took 30 seconds
Took 15 seconds
Took 8 minutes
Took 21 seconds
* Move settings cache and settings card preference provider out of libcockatrice_settings and into cockatrice
Took 52 minutes
Took 9 minutes
Took 1 minute
* Temp cache.
Took 16 minutes
* Dependency Injection for SettingsCache
* Turn SettingsCache into a QSharedPointer.
* Implement interfaces for settings that need it
Took 2 hours 38 minutes
* Adjust oracle.
Took 5 minutes
* Move abstract/noop interfaces to libcockatrice_interfaces so they can be linked against independently.
Took 52 minutes
* Clean up some links.
Took 3 minutes
* Cleanup two includes.
Took 3 minutes
* More fixes.
Took 7 minutes
* More includes that slipped past.
Took 3 minutes
* Stop mocking and start injecting for tests.
Took 15 minutes
* I don't know why remote_client was including main.
Took 4 minutes
* Include.
Took 3 minutes
* Lint.
Took 2 minutes
* Don't use Qt pointers.
Took 1 hour 7 minutes
* Make parser use CardSettingsInterface
Took 13 minutes
* Also adjust constructor lol.
Took 8 minutes
* Lint.
Took 32 minutes
* Revert "Lint."
This reverts commit ecb596c39e.
Took 3 minutes
* Test.
Took 3 minutes
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* Move models to own library.
Took 35 minutes
Took 22 minutes
* Adjust CMakeLists
Took 20 seconds
* Reformat CMakeLists.
Took 2 minutes
* Revert "Reformat CMakeLists."
This reverts commit db5982ad1c.
Took 55 seconds
* Lint an include
Took 17 minutes
Took 9 seconds
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* build: use vcpkg for most dependencies on macos
* factor out common params
* refactor: factor out common parameters in macOS matrix
* use env vars instead of matrix
* add comment about jianmingyong/ccache-action
* Remove unused 'qt_tools' param
* Use system python
* Let ccache caches be handled by ccache-action
* Add comment about why we use install-qt-action
* set unique ccache key
* nit
* fix cache prefix
* pass gh-token
* Revert "pass gh-token"
This reverts commit cadfa253c6.
* Reapply "pass gh-token"
This reverts commit bd15e96e18.
* do not cache qt on macos
