* [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>
The A/B run measured Ninja against the Visual Studio generator across
ten builds: Ninja medians sit at ~17 min against ~21 for MSBuild, and
more importantly Ninja already matches the Linux and macOS legs, runs
the compiler itself and parallelizes the whole graph instead of one
MSBuild node at a time. Single runs on shared 4-core hosts swing by
minutes either way (VS and Ninja legs even ran in different Azure
regions inside one matrix), so this switches on medians and
engineering coherence, not on any one timing.
The Ninja leg is now the release leg: it keeps the -Win10 suffix and
the vcpkg scope (named after the vs2026 toolchain, whose cl.exe is
unchanged, so the warm archives keep restoring), uploads PDBs from the
single-config layout and ships installers to releases. The experiment
scaffolding goes away with the generator it measured.
The Windows job spends about 28 minutes inside one "Build project"
group, with nothing in it that says where the time goes: MSBuild runs
single-node because no --parallel is passed, so projects build one
after another and it is not visible which projects, tasks or phases are
responsible.
Add a --profile flag that makes the Windows build write
-flp:PerformanceSummary and -bl, repeat the summary in the job log so
the numbers need no download, and report the core count, which decides
how much parallelism is worth asking for. The binary log is uploaded as
an artifact only on the runs that asked for it.
The workflow drives it from a build_profile matrix key that is empty by
default, so no build changes behaviour until someone sets it.
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* [CI] Key the vcpkg binary cache per ABI and let pull requests heal it
The cache had no prefix, and TAServers/vcpkg-cache restores every entry it
finds under the prefix rather than only the hashes vcpkg asks for. With all
legs sharing the single vcpkg/ namespace, each of the five of them downloads
all 66 archives of all five on every run. Each restore also refreshes the
entry's last access date, so a dead ABI generation stays fresh forever and
GitHub's least recently used eviction can never reach it.
Name the scope in the key after the platform, target and toolchain it belongs
to, so a leg restores only its own archives. macOS 14, 15 and 26 on the same
Xcode build identical binaries when they share the lowest deployment target,
so point all three at 14 and let them share one scope instead of three.
A pull request cannot write the shared cache either. Its entries are scoped to
refs/pull/<n>/merge, so nothing it saves is ever readable again, and letting
it write anyway only burns the repository's quota, which is what the previous
attempt at this did. Keep the build read-only and hand the archives it had to
build itself to a new workflow running in the default branch scope, which
saves them where master can restore them. Forks stay read-only, they are the
one case whose archives must not be trusted.
Because the prefix changes every key, add a job that deletes the entries of
retired scopes, which is the one-time clean up of the current flat namespace,
and the per-run ones pull requests leave behind, which age based eviction
also never reaches.
Also pin the Linux ccache cache to its own subdirectory, since the directory
is shared with the docker build context, and drop the clang-format install
from the build images, which format nothing at runtime.
* [CI] Create the vcpkg cache directory when a scope is still empty
* [CI] Let a pull request write the vcpkg archives it builds
* [CI] Upload the vcpkg handover as one archive instead of loose files
* [CI] Hand the vcpkg cache over from one leg per scope
* [CI] Gate vcpkg cache writes to master pushes
---------
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>
Add a workflow_dispatch trigger that builds an existing release tag with the current workflow and publishes it to GHCR under that tag name (optionally also as latest). This allows republishing a release whose image failed to publish, since re-running the original run reuses the workflow file from the release tag.
Dispatched runs check out the given tag, publish like stable releases, and use their own concurrency group so pushes to master can't cancel them.
Co-authored-by: Michael Hogg <3495076+jedinerd@users.noreply.github.com>
With archive: false, actions/upload-artifact ignores the name input and names the artifact after the uploaded file, so the digest artifacts were named after the raw digest instead of digest-<label>. The index job's download-artifact pattern digest-* then matched nothing, leaving no digests to build the image index from. Drop archive: false so the digest-<label> names apply.
Co-authored-by: Michael Hogg <3495076+jedinerd@users.noreply.github.com>
The inline comment after type=ref,event=tag was inside the tags block scalar, so it was passed to docker/metadata-action as part of the tag spec, failing with "Invalid event". Move the comment above the block.
Co-authored-by: Michael Hogg <3495076+jedinerd@users.noreply.github.com>
* [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>
* Direct wildcard resolution in action + cache with version key
* add back space
* Delete .ci/resolve_latest_aqt_qt_version.sh
* Disable Qt slimming and manual caching (use build-in fat caching)
* cleanup
* Re-add resolve_latest_aqt_qt_version.sh
* [App] Add onboarding wizard
Took 10 minutes
Took 3 minutes
Took 7 minutes
Took 9 minutes
Took 2 minutes
Took 1 minute
Took 7 minutes
* Adjust CI
Took 14 minutes
Took 56 seconds
Took 2 seconds
Took 3 seconds
* Adjust CI again
Took 14 minutes
Took 2 seconds
* Comments and fixes
Took 9 seconds
Took 1 minute
* Rebase.
Took 5 minutes
Took 50 seconds
Took 15 seconds
* Comments.
Took 7 minutes
* CI lol
Took 3 minutes
* CI again lol
Took 4 minutes
* Drop some settings, add some new ones.
Took 19 minutes
* Resize when expanding section
Took 4 minutes
Took 3 minutes
Took 7 minutes
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* native builds + merge
* naming and ordering
* use ninja and cmake build
* add ccache and cache mounts
* formatting
* Update servatrice.cpp
* Revert "Update servatrice.cpp"
This reverts commit 3acc684135c6db9baa17d00deaceecf8f7079721.
* remove ccache again
cache mounts are not part of GHA caches from docker action
* comments and cleanup
Use buildx provided in runner, see https://github.com/actions/runner-images/blob/main/images/ubuntu-slim/ubuntu-slim-Readme.md
* more explicit
* comments, first pass
* ${{ runner.temp }}
* $(printf "$GHCR_IMAGE@sha256:%s " *)
* follow docker docs for latest and extract short semver from our tags
* not so pretty, but allows the easy inspect at the end
* add Servatrice name
* add links to runner images
* comments, second pass
* cleanup
The webclient has been extracted to https://github.com/seavor/Webatrice
and the Playwright e2e suite has moved to Sockatrice. Cockatrice keeps
no copy.
Deleted:
- webclient/ (entire tree, 349 files)
- .github/workflows/web-build.yml, web-lint.yml
- .husky/pre-commit (the only tracked husky hook; managed entirely
from webclient's package.json which no longer exists)
Edited:
- .tx/config: dropped the webclient resource block; Webatrice/.tx/config
now manages its own translations
- .github/workflows/translations-pull.yml: removed webclient locales
from add-paths
- .github/workflows/desktop-build.yml, desktop-lint.yml: removed dead
'!webclient/**' and '!.husky/**' path-filter exclusions
- .github/dependabot.yml: removed commented-out npm/webclient block
- Doxyfile: removed webclient/ from EXCLUDE list
- .ci/release_template.md: dropped Webatrice section (Webatrice now
cuts its own releases)
- README.md: dropped 'first work on a webclient' line, added Webatrice
to Related Repositories, updated translation paragraph and build
badges
History preserved: every webclient commit remains recoverable via
git log on master before this commit.
* Use Qt 6.10.*
Took 3 hours 1 minute
* Remove workaround.
Took 10 minutes
* Pin to 6.11.0 for now.
Took 3 minutes
* Revert "Remove workaround."
This reverts commit 71584d1e50.
Took 4 seconds
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
