The executables precompile Qt since #7235, but the libcockatrice_*
static libraries still reparse the full Qt Core stack in every TU.
Measured over the tree after the IWYU pass: 380 MB of header parse in
the libraries, 92% of it reachable from qtcore_pch.h.
- Apply qtcore_pch.h to the 14 Qt Core static library targets
(skips libcockatrice_rng: its SFMT.c is C and cannot use a C++
precompiled header).
- Grow qtcore_pch.h with 25 QtCore headers that IWYU shows are
re-parsed across many TUs and were not already reachable from the
existing list (JSON/QUuid cluster, QSettings, item models,
threading, XmlStream, QTemporaryDir, ...).
- Grow qtwidgets_pch.h with 30 QtGui/QtWidgets headers (dialog
cluster, item views, QImageReader, ...).
Only stable Qt headers go into the PCH, so it rarely invalidates.
Test executables stay PCH-less: 1-3 TUs per target does not amortize
the extra precompile.
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>
* [Windows] Close running instances and purge stale runtime DLLs during update
* [Windows] Scope running-instance handling to install dir and fix NSIS build
- Drop !include nsExec.nsh: nsExec is a plugin DLL, not a header, so
makensis aborts before NSIS can build the installer.
- Match processes by image name and executable path under $INSTDIR via
PowerShell, then close (WM_CLOSE) and force-stop only those PIDs, so an
unrelated oracle.exe (Oracle DB) is never killed on a silent /R update.
- Gate the stale-runtime-DLL purge on $INSTDIR\cockatrice.exe existing, so
a first-time install can't recursively delete an unrelated Plugins
directory.
* [Windows] Make process detection rely on an explicit PowerShell exit code
* [Windows] Fix exit-code detection in the uninstaller process check
* [Windows] Pass the install dir to PowerShell without quoting pitfalls
* [Windows] Fail closed when a process path is unreadable
* [Windows] Only pre-check running instances for /R updates
* [Windows] Wait on a wall-clock 60 s deadline instead of a loop counter
* [Windows] Verify the force close and abort if the process survived
* [Windows] Defer silent updates instead of force-killing the client
* [Windows] Purge OpenSSL 3 DLLs on portable upgrades and broaden the stale-DLL guard
* [Windows] Replace the GetTickCount macro with a direct System::Call
* [Windows] Only close running instances whose path is readable
CloseMatchingApps reused the fail-closed detection filter, which counts a
process whose Path cannot be read (Access denied) as a match. The installer
runs elevated, so CloseMainWindow() would then be delivered to an unrelated
oracle.exe / servatrice.exe belonging to some other program, silently closing
a third party application during a Cockatrice update.
The set of processes we act on now has to be the strict one: a readable path
under $INSTDIR. Unreadable paths are simply not ours.
* [Windows] Match running instances by a readable path under the install dir
The Path read in the Where-Object filter raises "Access is denied" for
processes the installer cannot open, and both branches of the try/catch
turned that into a match. Together with mapping every non-zero nsExec result
to "running" this made the guard a hard stop rather than a fail-closed one: a
machine with an Oracle database service (or a servatrice service) running as
SYSTEM always has a process named oracle / servatrice whose path cannot be
read, so every silent update aborted after 60 s and every interactive install
re-asked for Retry indefinitely.
A process whose path cannot be read is not one of ours - the updater is
launched by the application it updates and therefore runs in the same user
session, and an interactive install is elevated and can read the paths of the
user's own processes. Both the detection and the close path now require a
readable path under $INSTDIR, which is what the comment above the helpers
already claimed.
* [Windows] Distinguish "could not determine" from "definitely running"
Mapping every non-zero nsExec result to "running" folds nsExec's "error"
return (powershell.exe could not be started at all - removed, blocked by
policy, refused by antivirus) into "Cockatrice is running". That state can
never clear: closing applications does not help, so silent installs aborted
after the full 60 s wait and interactive installs could not get past the
Retry/Cancel prompt.
IsAppRunning now reports three states (0 not running, 1 running, 2 could not
determine) and a new IsProcessLookupAvailable preflight checks that
powershell.exe can run at all. A broken PowerShell is reported to the user
once - with a message box, since a silent install would otherwise show
nothing - and the install continues instead of being blocked forever. Only a
definite "running" is now waited for.
* [Windows] Force-stop the console applications after the grace period
Servatrice and Oracle are console applications, so CloseMainWindow()
returns false and does nothing for them. Every silent update therefore
waited the full 60 s and then aborted, and every interactive install looped
on the Retry prompt, for any user running Servatrice out of the install
directory.
They now get a second shutdown path: once the grace period is over they are
force-stopped by PID with the same strict install-dir filter, since they hold
no unsaved user data. Cockatrice keeps the graceful-only treatment - a
force-kill there could destroy an unsaved deck - and still aborts instead.
* [Windows] Tell the user when a silent install or uninstall is aborted
On the /R path the installer runs with SetSilent and SetAutoClose, so the
DetailPrint before Abort went nowhere: the user clicks Help -> Check for
Client Updates, the download finishes, Cockatrice closes its window, and
nothing else happens - no update and no error, because the client launches the
installer detached and never looks at its exit code.
Every silent abort path now shows a message box telling the user which
application is still open and that they should close it and run the
installer again. The same applies to the silent uninstall, where the only
output is a log file nobody reads.
* [Windows] Treat leftover runtime files as an install marker for the purge
The guard on the stale-DLL purge only looked for the three executables and
uninstall.exe, so it skipped precisely the case the purge exists for: a
previous failed update whose uninstaller removed what it could and silently
skipped the Qt DLLs that a running instance had locked. None of the four
markers is left in that directory, and the stale DLLs then survive into the
new install.
A leftover Qt6Core.dll (what we ship today) or the qt.conf written by a
portable install now count as an install marker as well.
* [Windows] Run the section instance check before any file is deleted
Call EnsureAppsNotRunning sat after the portable-upgrade block, which had
already deleted the old cockatrice.exe, Qt*.dll and plugins directory. An
interactive install ("Portable mode" pointing at an existing portable folder)
has $ReinstallMode = 0, so the .onInit guard is skipped and locked files were
silently skipped - exactly the mixed-Qt-DLL state this change is about.
The check now runs right after SetOutPath, before the first Delete/RMDir in
the section.
* [Windows] Guard the old install before the old uninstaller deletes files
The interactive upgrade path removes the previous installation long before the
install section: componentsPagePre runs the old UninstallString on the
components page, i.e. before the directory page and before any call of ours
can check anything. Every uninstaller shipped before this change has no
un.EnsureAppsNotRunning, so for exactly the versions affected by #1576 the old
files were deleted while cockatrice.exe was still running and the new
installer had no say in it.
componentsPagePre now runs the same check before both ExecWait calls. At that
point $INSTDIR is still only the default, not the directory the old install
lives in, so the new EnsureOldInstallNotRunning guards the location recorded
in the registry instead. The helpers now work on a $GuardDir the caller
picks; the normal entry points keep seeding it with $INSTDIR.
* [Windows] Do not abort the section check past the point of no return
"nothing has been replaced yet, so aborting just defers the update" only holds
for the guard in .onInit. On the /R path AutoUninstallIfNeeded has already run
the old uninstaller, which RMDir /r's the whole install directory, and the
purge below it deletes the runtime files - so an Abort from the section would
leave the user with the old version uninstalled and the new one not installed
at all, which is strictly worse than the bug this change fixes.
The ordering now guarantees the check already passed: .onInit guards the /R
path before the uninstall, so the section only repeats it where that is not
already true, i.e. for interactive installs, where nothing of this installer
has been deleted yet and an Abort is the correct outcome.
As a safety net for whatever the purge could not remove (a still locked file,
or a check that could not run at all), HasStaleRuntimeFiles reports surviving
runtime files with a message box - also on a silent update, where there is no
other way to let the user know the install came out incomplete.
* [Windows] Only block the in-app updater with a message box
The message box on the silent abort would also fire for a plain /S install,
which is exactly the unattended case: a package manager (or a CI job) running
the installer with a leftover Cockatrice process would now hang forever on a
dialog nobody can see instead of aborting.
The box is therefore limited to /R, the path the in-app updater takes, where
the user just clicked Help -> Check for Client Updates, the client launches
the installer detached and never inspects its exit code, and the window is
about to disappear - without a message, the update would silently not happen.
Every silent abort also sets an error level now, so an unattended caller can
see that the run failed instead of having to read the log.
The silent uninstaller reports through log and exit code for the same reason:
it is run unattended by the /R updater itself (QuietUninstallString) and by
package managers. The installer that invoked it tells the user about the
incomplete result itself.
* [Windows] Treat an unset guard directory as undeterminable
$GuardDir is set by the three entry points into the check, but it is now what
decides whether a process counts as one of ours, and an empty string would
match every process that merely shares an image name - the same false positive
the strict path filter exists to avoid. Report it as "could not determine"
instead of running the check, so a future caller that forgets to set it warns
the user rather than closing or killing a third party process.
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* [Build] Keep Windows installs free of build-tree artifacts
Several Windows packaging gaps could leak Visual Studio CMake build
output into the installed application or the NSIS installer:
- The per-app DLL sweep used
${CMAKE_BINARY_DIR}/${PROJECT_NAME}/${CMAKE_BUILD_TYPE}, which is
empty on multi-config generators, collapsing the recursive
DIRECTORY install into the whole build tree (containing *.dir,
*_autogen, .qt, .qsb, x64, ...). Point it at the real per-config
output with $<TARGET_FILE_DIR:...> and exclude build artifacts.
- install(FILES ${OPENSSL_INCLUDE_DIRS} ...) tried to install OpenSSL
include directories as files. CMake refuses this
("install FILES given directory"); it only slipped through CI
because the vcpkg OpenSSL config leaves the variable empty. Remove it;
fixup_bundle already ships the OpenSSL runtime DLLs.
- The NSIS uninstaller only deleted *.exe/*.dll and a few known files,
so build-tree leftovers survived an uninstall/reinstall cycle. Wipe
the whole directory tree instead.
- Add a Windows CI gate that lists the packaged installer with 7-Zip
and fails the build if any build-tree artifact path is found.
* Update .ci/compile.sh
Co-authored-by: tooomm <tooomm@users.noreply.github.com>
* [Build] Rework Windows installer artifact exclusions per review
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
Co-authored-by: tooomm <tooomm@users.noreply.github.com>
* Add scheme-variant theme asset resolution (#7209-1)
ThemeManager::assetPath() and schemeVariantPath() resolve a theme asset to its scheme-variant file (prefix-light/dark.png) with fallback to the plain asset, and themePixmap()/loadBrush()/loadExtraBrush() use them. CSS files load style-dark.css or style-light.css when present. Home widget re-resolves its background on theme change.
Link pixel_map_generator.cpp into the oracle target, which needs Qt6::Xml for QDomDocument.
* Fix clang-format wrap of theme format probe lists
---------
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>
* [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>
* [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>
* [Application] Add single instance guard and mime types.
Took 2 hours 39 minutes
Took 18 minutes
Took 5 minutes
Took 12 seconds
Took 11 seconds
* Rework
Took 30 minutes
Took 50 seconds
* Only enforce single instance if launched with arguments.
Took 5 minutes
* Prototype intents
Took 53 minutes
Took 6 seconds
* Connect/disconnect and join game/room intents.
Took 3 hours 14 minutes
Took 2 seconds
Took 15 seconds
* Fix include.
Took 1 minute
Took 23 seconds
Took 2 seconds
* Mac handling.
Took 10 minutes
Took 12 seconds
Took 3 minutes
* Lint.
Took 3 minutes
* Rebase.
Took 3 minutes
Took 17 seconds
* Implement UrlSchemeEventFilter
Took 10 minutes
Took 7 seconds
* Qt Moc
Took 3 minutes
* Modern PList.
Took 21 minutes
Took 1 minute
* Debug output.
Took 6 minutes
Took 19 minutes
* Watch file:// prefix.
Took 15 minutes
Took 7 seconds
* Better handler.
Took 6 minutes
* Don't store reference in member
Took 5 minutes
* Move impl to cpp, fix lifetime issues.
Took 11 minutes
Took 2 minutes
* Better single-instance handoff, url intent harded
copy game link context-menu
Polish for installers
Took 35 minutes
Took 8 seconds
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* Bump minimum_required to 3.5 and GoogleTest to 1.12
GoogleTest 1.12 is the oldest version that sets cmake_minimum_required() to 3.5 in the CMakeLists files it provides
* code style changes
* Use 1.17.0
* Set minimum to 3.10 to make top-level CMakeLists
* New hash
Co-authored-by: tooomm <tooomm@users.noreply.github.com>
* Update cmake/gtest-CMakeLists.txt.in
Co-authored-by: tooomm <tooomm@users.noreply.github.com>
---------
Co-authored-by: tooomm <tooomm@users.noreply.github.com>
Co-authored-by: ebbit1q <ebbit1q@gmail.com>
* Add -R option in Windows NSIS script
Took 23 minutes
* Small fix.
Took 3 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.
* Move logger and key signals from libcockatrice_utility to Cockatrice.
Took 9 minutes
* Only link Qt::Core instead of COCKATRICE_QT_MODULES to libraries, if possible.
Took 2 minutes
---------
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
* Set some CPack variables
WIP for DMG background image
* Use a .tif for background
* Add Background image and DS_Store script
Script is from https://www.kitware.com//creating-mac-os-x-packages-with-cmake/ with cmake.app changed to cockatrice.app
* Add position for all icons
* Adjust background size and icon position
This draws dbconverter off the screen as a hack to hide it, so hopefully it doesn't break.
* Add files via upload
* Change Icon Position
Icon position is probably relative to the window edge, not the edge of the screen so the numbers should be lower.
* Adjust Icon Position and Window Boundaries
Applescript seems to use the center of the icon rather than the left corner so the coordinates need to be adjusted by 64 px.
The finder window counts the 22 px of the title bar in the window size so 22 must be added to the fourth coord of position to fit the entire image.
* Fix spelling of Servatrice
* Satifsfy Code Style
* Use vector format (eps)
* Delete background.tif
* Use vector format (eps)
* Use the right file extention in the script
* Point to svg
* Use svg
* Add svg for background image
* Delete background.eps
* Use plain SVG, adjust text
* Move applescript to /cmake
* Move background to /cmake
* Point CPack to /cmake
background and applescript now reside there
* Revert to .tif
* Use compressed .tif
* Use more specific file name
* Script expects .background:background.tif
* remove dependency on deprecated qt5 libraries for qt6
removes the use of qt6-5compat for builds
replaces use of QRegExp with QRegularExpression
fixes incorrect usage of QRegExp
removes use of QTextCodec
fixes incorrect usage of QTextCodec
sets qtlinguist as a required component for qt6
* fix anchoredPattern not existing in qt 5.11
* merge clangify and cmakify into format.sh
update desktop lint workflow to 22.04
print cmake-format version as well
um, rename things?
add extra examples to format.sh --help
add option to not run clang-format
fix version display in .ci/lint_cpp.sh
fix relative paths in format.sh
fix formatting dirs
* run ./format.sh --cmake --branch ""
* revert formatting of cmake comments
Add lock around deleting arrows for commanding cards
Add support for Qt6 w/ Backwards Qt5
Handle Qt5/6 cross compilation better
Last cleanups
caps matter
Fix serv
Prevent crash on 6.3.0 Linux & bump to 5.8 min
Prevent out of bounds indexing
Delete shutdown timer if it exists
Fixup ticket comments, remove unneeded guards
Try to add support for missing OSes
Update .ci/release_template.md
Update PR based on comments
Update XML name after done and remove Hirsute
Address local game crash
Address comments from PR (again)
Tests don't work on mac, will see if a problem on other OSes
make soundengine more consistent across qt versions
disable tests on distros that are covered by others
Fix Oracle Crash due to bad memory access
Update Oracle to use new Qt6 way of adding translations
Add support for Qt5/Qt6 compiling of Cockatrice
Remove unneeded calls to QtMath/cmath/math.h
Update how we handle bitwise comparisons for enums with Tray Icon
Change header guards to not duplicate function
Leave comment & Fix Path for GHA Qt
Update common/server.h
Update cockatrice/src/window_main.cpp
Rollback change on cmake module path for NSIS
check docker image requirements
add size limit to ccache
put variables in quotes
properly set build type on mac
avoid names used in cmake
fix up cmake module path
cmake 3.10 does not recognize prepend
Support Tests in FindQtRuntime
set ccache size on non debug builds as well
immediately return when removing non existing client
handle incTxBytes with a signal instead
don't set common link libraries in cockatrice/CMakeLists.txt
add comments
set macos qt version to 6
Try upgrading XCode versions to latest they can be supported on
Ensure Qt gets linked
add tmate so i can see what's going on
Qt6 points two directories further down than Qt5 with regard to the top lib path, so we need to account for this
Establish Plugins directory for Qt6
Establish TLS plugins for Qt6 services
Minor change for release channel network manager
Let windows build in parallel cores
Wrong symbols
Qt6 patch up for signal
add missing qt6 package on deb builds
boolean expressions are hard
negative indexes should go to the end
Intentionally fail cache
move size checks to individual zone types
Hardcode libs needed for building on Windows, as the regex was annoying
Update wording
use the --parallel option in all builds
clean up the .ci scripts some more
tweak fedora build
add os parameter to compile.sh
I don't really like this but it seems the easiest way
I'd prefer if these types of quirks would live in the main configuration
file, the yml
fixup yml
readd appended cache key to vcpkg step
fix windows 32 quirk
the json hash is already added to the key as well
remove os parameter and clean up ci files
set name_build.sh to output relative paths
set backwards compatible version of xcode and qt on mac
set QTDIR for mac builds on qt5
has no effect for qt6
export BUILD_DIR to name_build.sh
merge mac build steps
merge homebrew steps, set package suffix
link qt5
remove brew link
set qtdir to qt5 only
compile.sh vars need to be empty not 0
fix sets manager search bar on qt 5.12/15
fix oracle subprocess errors being ignored on qt 5
clean up translation loading
move en@source translation file so it will not get included in packages
NOTE: this needs to be done at transifex as well!
Use generator platform over osname
Short circuit if not Win defined
* add ubuntu 21.10 to ci
remove ubuntu 20.10
* forgot to update dockerfile
* add googletest from repos
* update downloaded gtest
ideally this should just grab the master version
* fix hash
* fix cmake issue
