Skip to content

Some improvements for testing#1

Closed
FranciscoPombal wants to merge 5 commits into
glassez:qt6from
FranciscoPombal:glassez_qt6
Closed

Some improvements for testing#1
FranciscoPombal wants to merge 5 commits into
glassez:qt6from
FranciscoPombal:glassez_qt6

Conversation

@FranciscoPombal
Copy link
Copy Markdown

@FranciscoPombal FranciscoPombal commented May 9, 2021

Further explanation about this PR at the bottom. Commit summary:

Subject: [PATCH 1/2] Multiple fixes for building with Qt6 on Windows

This was tested with the experimental vcpkg Qt6 port:
microsoft/vcpkg#14333
as well as with the official Qt6 MSVC binary distribution

Working with upstream on the vcpkg port of Qt6 is the priority since
that is what will be used in CI.

What works:

- Dynamic and static Debug and Release builds on MSVC with vcpkg.
In all 4 cases, installation works correctly, as well as running from the
build directory right after the build.

- Dynamic Release and Debug builds on MSVC without vcpkg (using zlib,
  boost and libtorrent compiled from source + qt and openssl precompiled
  binaries from the official MSVC Qt distribution). Again, running from
  the build directory and installing works.

Current issues when building with the vcpkg Qt6 port:

- The problem with CMAKE_FIND_PACKAGE_PREFER_CONFIG (marked with a TODO)
has been reported to Qt: https://bugreports.qt.io/browse/QTBUG-93604
The workaround is trivial (just commented out a single line) and
harmless for now.

What was not tested/touched:

- Building with MinGW
- Static builds without vcpkg

Minor changes:

- Bumped minimum Qt version to 6.1.0
- Bumped minimum CMake version to 3.20 due to usage of cmake_path()
- The Qt6 Tools component is required for the Qt6::windeployqt target

Subject: [PATCH 2/2] add experimental workflow for Qt6 builds

also tests installation on Windows with dynamic and static builds



This is not really intended to be merged. I just posted this for convenience, so that you are also able to test more easily and so that I could test the CI runs.

I will probably try to backport some of these improvements to the current Qt5 build (namely the installation and handling of dependent DLLs for dynamic builds). In progress: qbittorrent#14959

It's easy to test with vcpkg if you want to. As for building with the official Qt6 MSVC binary distribution + other dependencies compiled from source, here are the steps I preformed, if you want to reproduce.

This can be a bit simplified by installing most/all binaries under a common prefix and then just setting CMAKE_PREFIX_PATH appropriately; it was just more convenient for me at the time to install everything to separate prefixes. One notable annoyance is that Qt6 requires setting more variables than Qt5 in order to work, but it's not too bad - if you miss any, CMake will tell you exactly what it needs.

Boost 1.76.0 (builds both Release and Debug artifacts):

.\bootstrap.bat --prefix=installed
.\b2.exe address-model=64 --build-type=complete --build-dir=build install --prefix=installed -a -q -j4

zlib (Release):

cd "C:\Users\User\source\deps_msvc\zlib-1.2.11"
cmake -S . -B build -G "Ninja" -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX="C:\Users\User\source\msvc\zlib-1.2.11\build\installed"
cmake --build build
cmake --install build

zlib (Debug):

cd "C:\Users\User\source\deps_msvc\zlib-1.2.11"
cmake -S . -B build_dbg -G "Ninja" -DCMAKE_BUILD_TYPE=Debug -DCMAKE_INSTALL_PREFIX="C:\Users\User\source\deps_msvc\zlib-1.2.11\installed_dbg"
cmake --build build_dbg
cmake --install build_dbg

Everything else below uses pre-compiled Qt and OpenSSL binaries from the official Qt binary distribution, and are assumed to be installed under C:\Qt.

libtorrent (Release):

cd "C:\Users\User\source\deps_msvc\libtorrent"
git checkout RC_1_2
mkdir cmake-build-dir
mkdir RC_1_2

cmake -S . -B cmake-build-dir\RC_1_2 -G "Ninja" -DCMAKE_BUILD_TYPE=Release -DOPENSSL_ROOT_DIR="C:\Qt\Tools\OpenSSL\Win_x64" -DBoost_DIR="C:\Users\User\source\deps_msvc\boost_1_76_0\installed\lib\cmake\Boost-1.76.0" -Ddeprecated-functions=OFF -Dbuild_examples=ON -Dbuild_tools=ON -DCMAKE_INSTALL_PREFIX="C:\Users\User\source\deps_msvc\libtorrent\cmake-build-dir\RC_1_2_installed"

cmake --build cmake-build-dir\RC_1_2
cmake --install cmake-build-dir\RC_1_2

libtorrent (Debug):

cd "C:\Users\User\source\deps_msvc\libtorrent"
git checkout RC_1_2
mkdir cmake-build-dir
mkdir RC_1_2_dbg

cmake -S . -B cmake-build-dir\RC_1_2_dbg -G "Ninja" -DCMAKE_BUILD_TYPE=Release -DOPENSSL_ROOT_DIR="C:\Qt\Tools\OpenSSL\Win_x64" -DBoost_DIR="C:\Users\User\source\deps_msvc\boost_1_76_0\installed\lib\cmake\Boost-1.76.0" -Ddeprecated-functions=OFF -Dbuild_examples=ON -Dbuild_tools=ON -DCMAKE_INSTALL_PREFIX="C:\Users\User\source\deps_msvc\libtorrent\cmake-build-dir\RC_1_2_installed_dbg"

cmake --build cmake-build-dir\RC_1_2_dbg
cmake --install cmake-build-dir\RC_1_2_dbg

qBittorrent with Qt6 (Release):

cd "C:\Users\User\source\qt6-temp\qBittorrent"

cmake -S . -B build -G "Ninja" -DCMAKE_BUILD_TYPE="Release" -DZLIB_INCLUDE_DIR="C:\Users\User\source\deps_msvc\zlib-1.2.11\installed\include" -DZLIB_LIBRARY="C:\Users\User\source\deps_msvc\zlib-1.2.11\installed\lib\zlib.lib" -DOPENSSL_ROOT_DIR="C:\Qt\Tools\OpenSSL\Win_x64" -DBoost_DIR="C:\Users\User\source\deps_msvc\boost_1_76_0\installed\lib\cmake\Boost-1.76.0" -DLibtorrentRasterbar_DIR="C:\Users\User\source\deps_msvc\libtorrent\cmake-build-dir\RC_1_2_installed\lib\cmake\LibtorrentRasterbar" -DQt6_DIR="C:\Qt\6.1.0\msvc2019_64\lib\cmake\Qt6" -DQt6CoreTools_DIR="C:\Qt\6.1.0\msvc2019_64\lib\cmake\Qt6CoreTools" -DQt6ToolsTools_DIR="C:\Qt\6.1.0\msvc2019_64\lib\cmake\Qt6ToolsTools" -DQt6WidgetsTools_DIR="C:\Qt\6.1.0\msvc2019_64\lib\cmake\Qt6WidgetsTools" -DQt6GuiTools_DIR="C:\Qt\6.1.0\msvc2019_64\lib\cmake\Qt6GuiTools" -DCMAKE_INSTALL_PREFIX="C:\Users\User\source\qt6-temp\qBittorrent\install_dir"

cmake --build build
cmake --install build

qBittorrent with Qt6 (Debug):

cd "C:\Users\User\source\qt6-temp\qBittorrent"

cmake -B build -G "Ninja" -DCMAKE_BUILD_TYPE="Debug" -DZLIB_INCLUDE_DIR="C:\Users\User\source\deps_msvc\zlib-1.2.11\installed_dbg\include" -DZLIB_LIBRARY="C:\Users\User\source\deps_msvc\zlib-1.2.11\installed_dbg\lib\zlibd.lib" -DOPENSSL_ROOT_DIR="C:\Qt\Tools\OpenSSL\Win_x64" -DBoost_DIR="C:\Users\User\source\deps_msvc\boost_1_76_0\installed\lib\cmake\Boost-1.76.0" -DLibtorrentRasterbar_DIR="C:\Users\User\source\deps_msvc\libtorrent\cmake-build-dir\RC_1_2_installed_dbg\lib\cmake\LibtorrentRasterbar" -DQt6_DIR="C:\Qt\6.1.0\msvc2019_64\lib\cmake\Qt6" -DQt6CoreTools_DIR="C:\Qt\6.1.0\msvc2019_64\lib\cmake\Qt6CoreTools" -DQt6ToolsTools_DIR="C:\Qt\6.1.0\msvc2019_64\lib\cmake\Qt6ToolsTools" -DQt6WidgetsTools_DIR="C:\Qt\6.1.0\msvc2019_64\lib\cmake\Qt6WidgetsTools" -DQt6GuiTools_DIR="C:\Qt\6.1.0\msvc2019_64\lib\cmake\Qt6GuiTools" -DCMAKE_INSTALL_PREFIX="C:\Users\User\source\qt6-temp\qBittorrent\install_dir_dbg"

cmake --build build
cmake --install build --prefix install_dir

@FranciscoPombal

This comment has been minimized.

@FranciscoPombal

This comment has been minimized.

@FranciscoPombal

This comment has been minimized.

@FranciscoPombal
Copy link
Copy Markdown
Author

@glassez

OK, after some more cleanup, here is the final result:

https://github.com/FranciscoPombal/qBittorrent/actions/runs/826503179

@FranciscoPombal
Copy link
Copy Markdown
Author

@glassez

The 2nd issue with the vcpkg build (There is a (harmless?) warning about a missing file when running windeployqt. This is expected to be fixed in the port.) has now been fixed upstream.

@FranciscoPombal
Copy link
Copy Markdown
Author

@glassez

Ignore the conflicts, they are not important now. Refreshed the patch and update the ports to the latest commit from the Qt6 vcpkg PR. The PR has made it out of draft, and no significant changes are expected, especially to the things that already work, including the dynamic debug builds.

@FranciscoPombal FranciscoPombal force-pushed the glassez_qt6 branch 2 times, most recently from 1b24918 to 72a9e76 Compare May 13, 2021 16:58
Comment thread src/app/CMakeLists.txt Outdated
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems you should check TYPE property for each dependency lib instead.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glassez

Hmm, do we really want to support/endorse the use case of arbitrary mixes of static/dynamic versions of the dependent libraries? Does that even work properly in all cases? I understand supporting dynamic runtime + static libraries (in addition to both dynamic), but this seems a bit excessive.

With that in mind, I propose testing just Qt (for example), and assuming that its type is the same for all other dependencies. What do you think?

Copy link
Copy Markdown
Author

@FranciscoPombal FranciscoPombal May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glassez

Another relevant point to consider: https://docs.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-warning-lnk4098?view=msvc-160.

You should compile all source files to use the same run-time library.

Although it is technically possible and there are documented workarounds to mix different run-time library types, thanks to this recommendation in the official docs it seems to me we can simply tell the user they can either do fully static or fully dynamic, but not a mix of both. Then we could add (use this when building with dynamic libraries) to the description of the MSVC_RUNTIME_DYNAMIC option (and, if we do that, then we should probably rename that option to MSVC_DYNAMIC_BUILD or something).

Thoughts?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, there is nothing bad, incorrect, or unacceptable about mixing static and dynamic libraries (i.e., linking an executable file to multiple libraries of different linking types). Even indirect linking to different versions of the same library is quite acceptable (for example, this happens when you link qBittorrent with dynamic Qt and zlib, while the Qt itself is statically linked with zlib). We are only talking about some specific cases, for example, MSVC runtime library. So I'm totally against intentionally limiting it at project level.

With that in mind, I propose testing just Qt (for example), and assuming that its type is the same for all other dependencies. What do you think?

On the contrary, it may be a good idea to link with it dynamically (due to its large size), but statically with other (smaller) libraries. In any case, quite a lot of valid reasons for such mixing can be imagined.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glassez

So I'm totally against intentionally limiting it at project level.

OK, you've convinced me that we should not limit it (or even suggest that it should be better limited) - after all, there is no reason to make it unnecessarily difficult for others to add support for potentially useful workflows for them. But how about just considering it out-of-scope for now? If someone wants better support for that, they can implement it themselves later on (or I may even end up doing it myself at a later time).

Comment thread src/app/CMakeLists.txt Outdated
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure CMake knows the exact name of the .dll file, so you can avoid this kind of guessing.

Copy link
Copy Markdown
Author

@FranciscoPombal FranciscoPombal May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glassez

Unfortunately, I don't think that's the case. FindZLIB doesn't give you much to work with. You can experiment and see the variables/properties it sets. I couldn't find any other robust way other than copying the "name guessing" that FindZLIB itself does. This a problem unique to zlib, which for historical reasons can have all these different names. You can see that there is no such problem with OpenSSL, since the DLL name is "standard" (unless you also allow really old versions that might have the old eay names).

Let me know if you figure out a better way.

Also, rebased to fix conflicts.

@FranciscoPombal FranciscoPombal force-pushed the glassez_qt6 branch 2 times, most recently from 1f55c0f to beb176c Compare May 14, 2021 11:50
glassez pushed a commit that referenced this pull request Jun 7, 2021
* Update automatedrssdownloader.ui

* Update rssDownloader.html (#2)
@FranciscoPombal
Copy link
Copy Markdown
Author

@glassez rebased and removed the temporary Qt6 ports. The CMake changes commit is still basically an adaptation of qbittorrent#14959, so once that is merged upstream this patch can be slimmed down yet so more.

This was tested with the experimental vcpkg Qt6 port:
microsoft/vcpkg#14333
as well as with the official Qt6 MSVC binary distribution

Working with upstream on the vcpkg port of Qt6 is the priority since
that is what will be used in CI.

What works:

- Dynamic and static Debug and Release builds on MSVC with vcpkg.
In all 4 cases, installation works correctly, as well as running from the
build directory right after the build.

- Dynamic Release and Debug builds on MSVC without vcpkg (using zlib,
  boost and libtorrent compiled from source + qt and openssl precompiled
  binaries from the official MSVC Qt distribution). Again, running from
  the build directory and installing works.

Current issues when building with the vcpkg Qt6 port:

- The problem with CMAKE_FIND_PACKAGE_PREFER_CONFIG (marked with a TODO)
has been reported to Qt: https://bugreports.qt.io/browse/QTBUG-93604
The workaround is trivial (just commented out a single line) and
harmless for now.

What was not tested/touched:

- Building with MinGW
- Static builds without vcpkg

Minor changes:

- Bumped minimum Qt version to 6.1.0
- Bumped minimum CMake version to 3.20 due to usage of cmake_path()
- The Qt6 Tools component is required for the Qt6::windeployqt target
@FranciscoPombal FranciscoPombal force-pushed the glassez_qt6 branch 5 times, most recently from 6d2f155 to b6c64cf Compare July 5, 2021 21:08
also tests installation on Windows with dynamic and static builds
@FranciscoPombal
Copy link
Copy Markdown
Author

success, all Qt6 builds are passing

@FranciscoPombal
Copy link
Copy Markdown
Author

@glassez

I see you approved the runs. As you can see, Windows and macOS Qt6 builds are passing. All the others are from the other workflow which is expected to fail right now.

@glassez
Copy link
Copy Markdown
Owner

glassez commented Jul 6, 2021

@glassez

I see you approved the runs. As you can see, Windows and macOS Qt6 builds are passing. All the others are from the other workflow which is expected to fail right now.

Can you add PR that is really switch environment to Qt6? I.e. drop all Qt5 stuff. And I would expect it to not include unrelated changes, e.g. variable name changes, adding quotes etc. Then I can add your commits in upstream PR.

@FranciscoPombal
Copy link
Copy Markdown
Author

@glassez

Sure, but as I've said before, it would be best to merge qbittorrent#14959 first. There are common things from qbittorrent#14959 that also need to be adjusted for Qt6, so it is more convenient if that one goes in first so that I can rebase this one on top more easily.

@glassez glassez force-pushed the qt6 branch 4 times, most recently from c6362ad to 9b4d6c4 Compare July 24, 2021 13:26
@glassez glassez force-pushed the qt6 branch 4 times, most recently from b44bd35 to 95293d2 Compare July 31, 2021 11:26
@glassez glassez force-pushed the qt6 branch 6 times, most recently from 40bdcb2 to add75fb Compare October 2, 2021 06:23
@glassez glassez deleted the branch glassez:qt6 October 3, 2021 12:58
@glassez glassez closed this Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants