Add CMake Install Support for Windows #4300

Nerixyz · 2023-01-11T22:40:44Z

Pull request checklist:

CHANGELOG.md was updated, if applicable

Description

Chatterino had only rudimentary support for <build-tool> install on Windows, because it only copied the chatterino.exe.

There were two main issues:

No shared libraries such as openssl were copied to CMAKE_INSTALL_PREFIX.
windeployqt never ran, because the command silently failed (cmake's set joins multiple arguments as a semicolon-separated list).

This PR addresses these issues:

The RUNTIME_DEPENDENCIES are included. Windows DLLs are excluded, as done here.
windeployqt is run and aborts in case of any error (COMMAND_ERROR_IS_FATAL ANY)

Furthermore, it reverts #4297 and adds awareness for CMAKE_BUILD_TYPE.

How to build and install

Using conan:

cmake .. -DUSE_CONAN=On -DCMAKE_PREFIX_PATH=<qt-path>\msvc2019_64\lib\cmake\Qt5 -DCMAKE_BUILD_TYPE=Debug -DCMAKE_INSTALL_PREFIX=<install-dir> -GNinja
ninja
ninja install

Known issues

For vcpkg, -DX_VCPKG_APPLOCAL_DEPS_INSTALL is required.

Documentation

I didn't add any documentation since this feature was never used. I want to rewrite the documentation for Windows (to include VS Code, Visual Studio, and vcpkg) so I'll add it there.

kornes · 2023-01-12T14:08:06Z

src/CMakeLists.txt

@@ -637,17 +637,32 @@ if (BUILD_APP)
        )

    if (MSVC)
-        if (NOT VCPKG_INSTALLED_DIR)


none of that should run when vcpkg toolchain is used, so VCPKG_INSTALLED_DIR check should be kept.

Why? For me, it just leaves the folder empty.

take a look on this
https://github.com/microsoft/vcpkg/blob/036769f5d6da667391c9240bc7ca56e84cacd5b1/scripts/buildsystems/vcpkg.cmake#L48
and these 2 scripts, called in post-build stage
https://github.com/microsoft/vcpkg/blob/master/scripts/buildsystems/msbuild/applocal.ps1
https://github.com/microsoft/vcpkg/blob/master/ports/qt5-base/qtdeploy.ps1

maybe some problem with running powershell on your side causes it to fail

called in post-build stage

This PR is not about building chatterino, it's about installing chatterino. When building, the deployment script is run, but it needs to run at installation as well.

oh, i blame my lack of cmake knowledge then

it seems there is experimental switch for cmake install support, maybe it will help with your PR: https://github.com/microsoft/vcpkg/blob/036769f5d6da667391c9240bc7ca56e84cacd5b1/scripts/buildsystems/vcpkg.cmake#L52

anyways, my issue is resolved if it wont affect building

Thanks for pointing that out. It almost works without any effort. The only issue is that it places the qt plugins inside a plugins folder. I'm taking care of that here, but I feel like this might be a bug.

Not sure how this works on Windows, but on Linux plugins are stored inside a plugins directory
e.g. if I install extra/qt5-imageformats it installs a directory under /usr/lib/qt/plugins/ like this:

/usr/lib/qt/plugins ├── imageformats │ ├── libqgif.so │ ├── libqicns.so │ ├── libqico.so │ ├── libqjp2.so │ ├── libqjpeg.so │ ├── libqmng.so │ ├── libqpdf.so │ ├── libqsvg.so │ ├── libqtga.so │ ├── libqtiff.so │ ├── libqwbmp.so │ └── libqwebp.so

when building with vcpkg it also places all plugins inside /plugins. When Qt search for plugins it checks root executable folder and also Plugins path from qt.conf file, which defaults to /plugins when not set (like in chatterino case).
Imo to keep it consistent with vcpkg build, dont move plugins at install in this PR

This seems like the problem described here microsoft/vcpkg#7461 (comment). The linked fixes mostly manipulate qt.conf, which didn't really work for me.

pajlada · 2023-01-21T20:47:25Z

Is this in a state both @Nerixyz and @kornes are happy with?

kornes · 2023-01-21T21:21:53Z

i would revert moving plugins part from bfdbf1c, and if there is need for it (personally i dont mind /plugins path) address it in another PR for both building and install
other than that lgtm 👍

Nerixyz · 2023-01-21T21:43:39Z

and if there is need for it (personally i dont mind /plugins path) address it in another PR for both building and install

The reason I added this was because chatterino wouldn't launch without it. Does it launch for you after installing in a fresh directory?

pajlada · 2023-01-21T22:06:07Z

I would expect an install step to make the binary run without further modification as long as the extra step isn't destructive (i.e. it may override system files or w/e).

Normal CMake install:

Install executable file to C:/forsen/chatterino.exe
Install plugins to C:/forsen/plugins/...

Your extra step:
3. Move plugins from C:/forsen/plugins/... to C:/forsen/... right?

Nerixyz · 2023-01-21T22:20:55Z

I would expect an install step to make the binary run without further modification as long as the extra step isn't destructive (i.e. it may override system files or w/e).

Normal CMake install:

Install executable file to C:/forsen/chatterino.exe

Install plugins to C:/forsen/plugins/...

Your extra step:
3. Move plugins from C:/forsen/plugins/... to C:/forsen/... right?

Yes that's the planned procedure.

kornes · 2023-01-24T17:50:18Z

Does it launch for you after installing in a fresh directory?

chatterino ability to find qt plugins on this PR and master (win + vcpkg):

plugins path	install (release)	install (debug)	build (release)	build (debug)
`./`	✅	✅	✅	✅
`./plugins`	✅	⛔️	✅	⛔️

so it's only debug vcpkg build acting up and not finding plugins in ./plugins folder (default hardcoded destination).
Suggested workaround is to specify path in resources/qt.conf, by adding:

[Paths]
Plugins = ./plugins

table then looks like this:

plugins path	install (release)	install (debug)	build (release)	build (debug)
`./`	✅	✅	✅	✅
`./plugins`	✅	✅	✅	✅

so in theory this change shouldn't break other build systems (since ./ is checked first) and fix both install and build on vcpkg.
But it will for sure break on other platforms, so qt.conf edits should be limited to win32 with some cmake magic

pajlada

This breaks non-install builds on Linux

Nerixyz · 2023-01-28T13:14:35Z

This breaks non-install builds on Linux

Specifically, the changes in qt.conf break builds (or rather runs). My approach would be to revert the change to qt.conf, since it only affects vcpkg debug builds (that you wouldn't want to install anyway).

* Update qtkeychain to `e5b070831cf1ea3cb98c95f97fcb7439f8d79bd6` (Chatterino#4250) * Update qtkeychain to `e5b070831cf1ea3cb98c95f97fcb7439f8d79bd6` * FreeBSD/Cirrus - use system qtkeychain - this ensures we don't have to install pkg-config to get libsecret built * Set BUILD_SHARED_LIBS to OFF globally - this resolves the libcommuni build issue * Add Thread Guard for debugging simple threading issues (Chatterino#4254) * Add ThreadGuard class * Use ThreadGuard when accessing a ChannelView's messageSnapshot * Remove protocol from organization domain (Chatterino#4256) * remove protocol from organization domain * Add changelog entry * update changelog entry Co-authored-by: Sam Heybey <sam@heybey.org> * Migrate to C++ 20 & switch to websocketpp develop branch (Chatterino#4252) * feat: c++ 20 * fix: c++ 20 deprecations * fix(msvc): warnings * chore: add changelog entry * fix: formatting * Update websocketpp to the `develop` branch * Specify other template type in FlagsEnum != operator * Remove the user of simple template ids in our websocketpp template class Also standardizes the file a bit by using nested namespaces, using pragma once * fix: turn `MAGIC_MESSAGE_SUFFIX` into a `QString` * hacky unhacky hacky const char hack Co-authored-by: Rasmus Karlsson <rasmus.karlsson@pajlada.com> * Remove another implicit this-capture for C++20 migration (Chatterino#4257) * Fix another usage of an implicit this capture * Add changelog entry * Enable LTO support for Chatterino builds (Chatterino#4258) This is disabled by default, and can be enabled by passing `-DCHATTERINO_LTO=On` to your cmake invocation. * Disable LTO by default (Chatterino#4260) * SplitContainer refactor (Chatterino#4261) * Remove unused include util/Helpers.hpp * SplitContainer::setTag fix parameter naming * autofy/constify where possible * More const auto ptr magicifying * Make SplitNode::Type an enum class * Move QuickSwitcherPopup includes from header to source file * Remove unused DropRegion code * use empty() instead of size() == 0 * Add curly braces everywhere * Remove useless reinterpret_cast It was casting Node* to Node* * Clarify that the connect is QObject::connect * SplitContainer::setSelected fix parameter naming * Rename function variables to remove unneccesary underscore Also move addSpacing parameter out of the layout function * emplace_back where possible * Name parameters * Remove ineffective const from return type * Make node getters const * Flatten Node::releaseSplit * Rename in-function variable to match code style * [ACTUAL CODE CHANGE/MOVE] Move clamp logic to its own function * name params * applyFromDescriptorRecursively: rename node param to baseNode * [ACTUAL CODE CHANGE/MOVE] Remove the many overloads for append/insertSplit This utilizes the C++20 designed initializers aggregate initialization feature * Remove unused includes * [ACTUAL CODE CHANGE] Clean up dragging logic There's no need to keep a pointer around to which split is being dragged, it's already stored in the QDropEvent source() * UNRELATED .clang-tidy: Only suggest UPPER_CASE for constant global variables * Remove unused SplitContainer::getSplitCount function * Use std::max in Node's clamp function * Remove test code * DraggedSplit.hpp: remove unused include * Split `setDraggingSplit` into two functions, `startDraggingSplit` and `stopDraggingSplit` * Only try to extract images if the image uploader is enabled (Chatterino#4246) * Only try to extract images if the image uploader is enabled * Add changelog entry * Add `qt5-imageformats` to Fedora dependency list (Chatterino#4265) * Fix text cursor in open channel dialog (Chatterino#4263) * Fix text cursor not blinking in open channel dialog * Update CHANGELOG.md Co-authored-by: pajlada <rasmus.karlsson@pajlada.com> * fix: Remove Unused Include Directives (Chatterino#4266) * fix: remove unused includes * fix: bad includes * fix: top include * fix: streamer mode includes * fix: missing include * fix: remove `#else` Co-authored-by: pajlada <rasmus.karlsson@pajlada.com> * Remove unused operators in `Image` (Chatterino#4267) * ref: merge TooltipPreviewImage and TooltipWidget (Chatterino#4268) * merge TooltipPreviewImage and TooltipWidget * changelog * add empty line before return * fix signalholder include * add changelog for bugfix * fix custom scaling issue Co-authored-by: pajlada <rasmus.karlsson@pajlada.com> * Fix crash that would occur when performing certain actions after removing all tabs (Chatterino#4271) * Ensure we can deselect notebooks * Add changelog entry * Use dynamic_cast instead of raw cast * Bump cmake/sanitizers-cmake from `99e159e` to `a6748f4` (Chatterino#4274) Bumps [cmake/sanitizers-cmake](https://github.com/arsenm/sanitizers-cmake) from `99e159e` to `a6748f4`. - [Release notes](https://github.com/arsenm/sanitizers-cmake/releases) - [Commits](arsenm/sanitizers-cmake@99e159e...a6748f4) --- updated-dependencies: - dependency-name: cmake/sanitizers-cmake dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Refactor SplitHeader class (Chatterino#4276) * Flatten static functions in anonymous namespace * SplitHeader ctor: Rename param * Header: Remove unnecessary `virtual`s * auto ptr where possible * Add curly braces * Comment twitch room modes * Treat roomModes->slowMode as an integer * Remove unused `this` from lambdas * Add `unsigned int` overload for localizeNumbers * Move thumbnail max age to a constexpr static & set explicit types * Explicitly use `QObject::connect` * Use `empty()` instead of `size()` * Name unused parameters * Move moderation action refreshing logic from SplitHeader to Split * Fix crash that could occur when closing down splits (Chatterino#4277) * Fix crash that could occur when closing down splits Specifically, if a split was closed after the request for a thumbnail had been made, but before the request had finished, we'd run into a use-after-free issue * Add changelog entry * Fix crash that could occur when closing down the last of a channel when reloading emotes (Chatterino#4278) * Fix crash that could occur when a channel is closed Ensure we copy the QString in case the channel goes out of scope and we're referring to nothing * Add changelog entry * Remove Unnecessary Includes in Headers (Chatterino#4275) * refactor: remove unnecessary includes in headers * fix: formatting * chore: changelog * fix: scrollbar * fix: suggestions and old appbase remains * fix: suggestion * fix: missing Qt forward declarations * fix: another qt include * fix: includes for precompiled-headers=off * Add missing `<memory>` includes * Add missing `#pragma once` * Fix tests Co-authored-by: Rasmus Karlsson <rasmus.karlsson@pajlada.com> * Flatten util/IncognitoBrowser.{h,c}pp (Chatterino#4280) * Remove Deprecated `QDesktopWidget` (Chatterino#4287) * fix: remove deprecated `QDesktopWidget` * chore: add changelog entry * Fix scrollbar highlight colors when changing message history limit (Chatterino#4288) * Use correct messagesLimit for Scrollbar * Update CHANGELOG.md Co-authored-by: pajlada <rasmus.karlsson@pajlada.com> * Fix link to the "connecting to the Twitch IRC server" guide (Chatterino#4293) * Massage includes some more (Chatterino#4294) * Add `<functional>` include to QStringHash.hpp This ensures the base `std::hash` template is declared before this specialization * Add missing includes to `src/providers/twitch/TwitchAccountManager.hpp` * Move explicit HelixChatters constructor to the source file * Remove unused includes & add used includes to NicknamesModel.hpp * NicknamesModel.hpp: Remove `virtual` when `override` is used * Add missing QStringHash include to TwitchEmotes.cpp * Add missing includes to various files * Print Qt version in cmake step Technically unrelated, but I'm sneaking it in * Add changelog entry * Bump Cirrus CI FreeBSD image from 12.1 to 13.1 (Chatterino#4295) * Cirrus CI, print gcc version & print compile commands * upgrade freebsd from 12.1 to 12.3 * Add changelog entry * Bump to 13.1 * Update changelog entry * Fix various small warnings (Chatterino#4296) * Fix `inconsistent-missing-override` warnings * Fix `final-dtor-non-final-class` warnings * Fix `ambiguous-reversed-operator` warnings * Fix: dont run windeployqt with VCPKG (Chatterino#4297) * Add feature to select channels to log (Chatterino#4302) * Add checkbox for custom logging and table with channels to log on Logs page * Add checkbox to enable and disable logging per channel * Return from addMessage before logging if custom logging enabled and channel does not have logging enabled * Use clang-format to fix formatting * Add CHANGELOG.md entry * Resolve PR comments * Remove toggle for channels so any channel listed will be logged * Move Only log channels listed below checkbox to just above table * Fix formatting * Re-order changelog * ChannelLog constructor: Copy & move instead of const ref & copy * ChannelLog::createEmpty: Curly brace initialize instead of repeating name * ChannelLog toString & createEmpty: nodiscard * Use COUNT paradigm in model column * Remove ChanneLoggingModel source file comments * Use Column::Channel in getRowFromItem * Rename `getItemFromRow` parameter and mark it as unused * Curly brace initialize ChannelLog * private & friend class the model * Filter out channels to log using a set instead of iterating over a vector every time a message comes in * Rename `ChannelLog::channel` member to `ChannelLog::channelName` Also made it private * mini comment on ChannelLog Co-authored-by: Felanbird <41973452+Felanbird@users.noreply.github.com> Co-authored-by: Rasmus Karlsson <rasmus.karlsson@pajlada.com> * Change the highlight order to prioritize Message highlights over User highlights (Chatterino#4303) * Format YAML and JSON Files With Prettier (Chatterino#4304) * ci: format yaml and json files with prettier * chore: add changelog entry * fix: format everything * ci: run pretter on all files * ci: rename prettier step * Add Felanbird to contributors list (Chatterino#4308) * Mark big strong contributors as Collaborators (Chatterino#4311) * Attempt to fix certain ping sound issues on Arch Linux (Chatterino#4285) * Add Wissididom to contributors list (Chatterino#4312) * Update `contributors.txt` instructions (Chatterino#4313) * Fix slash to backslash conversions in url hashes for opening links in incognito (Chatterino#4307) * Pass link as argument instead of in command string when opening incognito links * Update CHANGELOG.md * Make changelog message more user facing * Remove now unused argument for getCommand * Update `actionsx/prettier` from `v2` to `e90ec54` (Chatterino#4318) This finishes the migrations of all workflows that used node12 * Fix the split "Search" menu action not opening the correct search window (Chatterino#4305) * Fix split menu "Search" action It now opens a split-specific search instead of a global one * Add changelog entry * feat: Add Live Emote Updates for BTTV (Chatterino#4147) This feature is enabled by default and can be disabled in settings with the "Enable BTTV live emotes updates" setting. Co-authored-by: Felanbird <41973452+Felanbird@users.noreply.github.com> Co-authored-by: pajlada <rasmus.karlsson@pajlada.com> * Bump libcommuni version (Chatterino#4320) This includes a Qt6-related fix that Nerixyz contributed to the main repo but that hasn't been merged in yet. * Fix version checking (Chatterino#4329) https://github.com/Neargye/semver * Use semver library for version downgrade checking * Add test validating our current version is valid semver * Clean up clang-tidy action checks (Chatterino#4331) * clang-tidy action: Exclude lib & tests dir for clang-tidy CI checks The lib dir is obvious, but the tests dir is disabled because gtest has a million different thing that clang-tidy doesn't like, while they're in reality perfectly reasonable * clang-tidy-action: Disable the LGTM comment * Change sound backend from Qt to miniaudio (Chatterino#4334) Thanks Greenlandicsmiley, Nerixyz, Yoitsu, and helmak for helping debug & test this * Remove QMediaPlayer includes * Prefer local path when generating the sound path * Update changelog entry number * Disable pitch & spatialization control * Add CMake Install Support for Windows (Chatterino#4300) * fix: windows installation with cmake * fix: support install for all win32 compilers * chore: add changelog entry * fix: support `X_VCPKG_APPLOCAL_DEPS_INSTALL` * chore: document cmake min version * fix: vcpkg * fix: plugin path * fix: remove flattening * revert: `qt.conf` changes --------- Co-authored-by: pajlada <rasmus.karlsson@pajlada.com> * Disable translation building (Chatterino#4336) * Use `CMakeDeps` and `CMakeToolchain` as Generators on Conan (Chatterino#4335) * deps(conan): use `CMakeDeps` as generator * chore: add changelog entry * deps(conan): add `CMakeToolchain` generator * fix: use generated toolchain file * docs: mention toolchain as well * fix: spelling * fix: formatting * revert: use nmake * docs: fix documentation * Remove Qt::Multimedia as a dependency from CMakeLists.txt (Chatterino#4339) * Remove Multimedia required in find_package * Remove Multimedia from src/CMakeLists.txt * Suppress Output of Git Command in CMake (Chatterino#4340) --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: pajlada <rasmus.karlsson@pajlada.com> Co-authored-by: Sam Heybey <sam@heybey.org> Co-authored-by: Felanbird <41973452+Felanbird@users.noreply.github.com> Co-authored-by: Douglas <douglas.carvalho@edu.unipar.br> Co-authored-by: kornes <28986062+kornes@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Daniel Sage <24928223+dnsge@users.noreply.github.com> Co-authored-by: askepticaldreamer <106888785+askepticaldreamer@users.noreply.github.com> Co-authored-by: Thomas Petersen <67438283+Greenlandicsmiley@users.noreply.github.com> Co-authored-by: Wissididom <30803034+Wissididom@users.noreply.github.com>

kornes reviewed Jan 12, 2023

View reviewed changes

Nerixyz marked this pull request as ready for review January 12, 2023 15:49

Nerixyz force-pushed the fix/windows-install branch from bfdbf1c to 7624773 Compare January 24, 2023 19:42

Nerixyz added 8 commits January 24, 2023 20:44

fix: windows installation with cmake

edccd07

fix: support install for all win32 compilers

5177b5e

chore: add changelog entry

2b070fc

fix: support X_VCPKG_APPLOCAL_DEPS_INSTALL

5aa542c

chore: document cmake min version

45db493

fix: vcpkg

f485534

fix: plugin path

4caa9a6

fix: remove flattening

257c9f2

Nerixyz force-pushed the fix/windows-install branch from 7624773 to 257c9f2 Compare January 24, 2023 19:45

pajlada requested changes Jan 28, 2023

View reviewed changes

Nerixyz added 2 commits January 28, 2023 15:01

revert: qt.conf changes

a912281

Merge remote-tracking branch 'upstream/master' into fix/windows-install

22ddddb

pajlada approved these changes Jan 29, 2023

View reviewed changes

Merge branch 'master' into fix/windows-install

1048691

pajlada enabled auto-merge (squash) January 29, 2023 10:33

pajlada merged commit 633b775 into Chatterino:master Jan 29, 2023

Nerixyz deleted the fix/windows-install branch January 29, 2023 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CMake Install Support for Windows #4300

Add CMake Install Support for Windows #4300

Nerixyz commented Jan 11, 2023 •

edited

kornes Jan 12, 2023

Nerixyz Jan 12, 2023

kornes Jan 12, 2023

Nerixyz Jan 12, 2023

kornes Jan 12, 2023

Nerixyz Jan 12, 2023

pajlada Jan 12, 2023

kornes Jan 12, 2023 •

edited

Nerixyz Jan 12, 2023

pajlada commented Jan 21, 2023

kornes commented Jan 21, 2023 •

edited

Nerixyz commented Jan 21, 2023

pajlada commented Jan 21, 2023 •

edited

Nerixyz commented Jan 21, 2023

kornes commented Jan 24, 2023 •

edited

pajlada left a comment

Nerixyz commented Jan 28, 2023

Add CMake Install Support for Windows #4300

Add CMake Install Support for Windows #4300

Conversation

Nerixyz commented Jan 11, 2023 • edited

Description

How to build and install

Known issues

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kornes Jan 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pajlada commented Jan 21, 2023

kornes commented Jan 21, 2023 • edited

Nerixyz commented Jan 21, 2023

pajlada commented Jan 21, 2023 • edited

Nerixyz commented Jan 21, 2023

kornes commented Jan 24, 2023 • edited

pajlada left a comment

Choose a reason for hiding this comment

Nerixyz commented Jan 28, 2023

Nerixyz commented Jan 11, 2023 •

edited

kornes Jan 12, 2023 •

edited

kornes commented Jan 21, 2023 •

edited

pajlada commented Jan 21, 2023 •

edited

kornes commented Jan 24, 2023 •

edited