Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake compilation issues #11856

Open
FranciscoPombal opened this issue Jan 10, 2020 · 7 comments
Open

CMake compilation issues #11856

FranciscoPombal opened this issue Jan 10, 2020 · 7 comments
Labels

Comments

@FranciscoPombal
Copy link
Contributor

@FranciscoPombal FranciscoPombal commented Jan 10, 2020

qBittorrent version and Operating System

Latest/recent master, Ubuntu 18.04

If on linux, libtorrent-rasterbar and Qt version

libtorrent 1.2.3, Qt 5.9.5

What is the problem

Current CMake build scripts have a few problems:

  1. Can't build qBittorrent without GUI using CMake.
    Specifying any of -DDISABLE_GUI:BOOL="1", -DGUI:BOOL="0", -DQBT_USE_GUI:BOOL="0" has no effect. Even more confusingly, the last one does not even produce a Manually-specified variables were not used by the project: warning.

  2. QBT_ADDITONAL_CXX_FLAGS and QBT_ADDITONAL_C_FLAGS unconditionally affect the default compiler options set by the main CMAKE_BUILD_TYPE option;

    • If setting -DCMAKE_BUILD_TYPE=Debug, files are compiled with -Og -g3 -pipe -g; -g negates the previous -g3.
    • If setting -DCMAKE_BUILD_TYPE=Release, files are compiled with -Og -g3 -pipe -O3 -DNDEBUG; -Og has no effect, and release builds should probably not be compiled with -g3.

    Instead of providing these options unconditionally under the QBT_ADDITIONAL_*_FLAGSdefinitions, the CMAKE_CXX_FLAGS_RELEASE, CMAKE_CXX_FLAGS_DEBUG flags should be used. In case the user manually specifies them in the command line parameters, those should take precedence. Alternatively, they can be left as default, and the relevant documentation page in the wiki should suggest the recommended values.

What is the expected behavior

Better support for building with CMake, including the possibility of building without GUI via defining -DDISABLE_GUI:BOOL="1" or similar.

Steps to reproduce

Example of the above:

mkdir build && cd build
cmake -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Debug -DDISABLE_GUI:BOOL="1" ..

Extra info(if any)

I read through #11038, so I know there are rather strong feelings about CMake adoption.

CMake is now by far the most used build system for C++ projects, and is now even going to become the default for Qt. qmake will cease (has ceased?) development and will probably be deprecated in the future.

Since CMake is more widely used than qmake+autotools, there is a bigger chance that more people can contribute to improving the build scripts. More and more resources around the web focus exclusively on modern systems like Meson and CMake. CMake has good cross platform support, including Windows (I'm not a Windows developer, but the overall consensus seems to be that CMake is the superior alternative to autotools) and niceties like an PPA for the most recent versions on Ubuntu.

As a side note, libtorrent also has good support for building with CMake.

@FranciscoPombal

This comment has been minimized.

Copy link
Contributor Author

@FranciscoPombal FranciscoPombal commented Jan 10, 2020

Part of this issue duplicates #10898, apologies for that.

NotTsunami added a commit to NotTsunami/qBittorrent that referenced this issue Jan 12, 2020
We have no reason to try and enable these flags for non-debug builds,
at the risk of overriding user provided flags.

Partial resolution to qbittorrent#11856.
NotTsunami added a commit to NotTsunami/qBittorrent that referenced this issue Jan 12, 2020
We have no reason to try and enable these flags for non-debug builds,
at the risk of overriding user provided flags. Also fixes a typo in
the strings.

Partial resolution to qbittorrent#11856.
@NotTsunami

This comment has been minimized.

Copy link
Contributor

@NotTsunami NotTsunami commented Jan 12, 2020

-DDISABLE_GUI:BOOL="1" is crazy. It can be as simple as -DDISABLE_GUI, similar to the CMake opt-in for the WebUI. I can address that in a seperate PR.

As far as the extra -g flag goes on debug builds, I'm wondering if that's the environment passing additional flags or if something else is passing those flags, because I don't see anything in our CMake files passing an extra flag.

@FranciscoPombal

This comment has been minimized.

Copy link
Contributor Author

@FranciscoPombal FranciscoPombal commented Jan 12, 2020

@NotTsunami

-DDISABLE_GUI:BOOL="1" is crazy. It can be as simple as -DDISABLE_GUI, similar to the CMake opt-in for the WebUI. I can address that in a seperate PR.

Both options are possible, but the effects are slightly different. From the cmake manpage:

-D <var>:<type>=<value>, -D <var>=<value>

    Create or update a CMake CACHE entry.

    When CMake is first run in an empty build tree, it creates a CMakeCache.txt file and populates it with customizable settings for the project. This option may be used to specify a setting that takes priority over the project’s default value. The option may be repeated for as many CACHE entries as desired.

    If the :<type> portion is given it must be one of the types specified by the set() command documentation for its CACHE signature. If the :<type> portion is omitted the entry will be created with no type if it does not exist with a type already. If a command in the project sets the type to PATH or FILEPATH then the <value> will be converted to an absolute path.

    This option may also be given as a single argument: -D<var>:<type>=<value> or -D<var>=<value>.

As far as the extra -g flag goes on debug builds, I'm wondering if that's the environment passing additional flags or if something else is passing those flags, because I don't see anything in our CMake files passing an extra flag.

QBT_ADDITONAL_CXX_FLAGS and QBT_ADDITONAL_C_FLAGS get inserted before CMAKE_CXX_FLAGS_DEBUG. By default CMAKE_CXX_FLAGS_DEBUG is -g, and since it appears last, it cancels the previous -g3 inserted by QBT_ADDITONAL_CXX_FLAGS.

You can use the CMake GUI to easily check the default values for stuff like CMAKE_CXX_FLAGS_*.

NotTsunami added a commit to NotTsunami/qBittorrent that referenced this issue Jan 14, 2020
Our previous setup lead to two unintended consequences:

* Debug flags were included in both release and debug builds instead
of just debug builds
* Clang doesn't support -gX (where X is the level of debugging
optimization), but we checked for -Og support instead

This commit avoids both of these scenarios by removing the additional
flags altogether. Partial resolution to qbittorrent#11856.
@NotTsunami

This comment has been minimized.

Copy link
Contributor

@NotTsunami NotTsunami commented Jan 18, 2020

@NotTsunami

-DDISABLE_GUI:BOOL="1" is crazy. It can be as simple as -DDISABLE_GUI, similar to the CMake opt-in for the WebUI. I can address that in a seperate PR.

Both options are possible, but the effects are slightly different. From the cmake manpage:

-D <var>:<type>=<value>, -D <var>=<value>

    Create or update a CMake CACHE entry.

    When CMake is first run in an empty build tree, it creates a CMakeCache.txt file and populates it with customizable settings for the project. This option may be used to specify a setting that takes priority over the project’s default value. The option may be repeated for as many CACHE entries as desired.

    If the :<type> portion is given it must be one of the types specified by the set() command documentation for its CACHE signature. If the :<type> portion is omitted the entry will be created with no type if it does not exist with a type already. If a command in the project sets the type to PATH or FILEPATH then the <value> will be converted to an absolute path.

    This option may also be given as a single argument: -D<var>:<type>=<value> or -D<var>=<value>.

As far as the extra -g flag goes on debug builds, I'm wondering if that's the environment passing additional flags or if something else is passing those flags, because I don't see anything in our CMake files passing an extra flag.

QBT_ADDITONAL_CXX_FLAGS and QBT_ADDITONAL_C_FLAGS get inserted before CMAKE_CXX_FLAGS_DEBUG. By default CMAKE_CXX_FLAGS_DEBUG is -g, and since it appears last, it cancels the previous -g3 inserted by QBT_ADDITONAL_CXX_FLAGS.

You can use the CMake GUI to easily check the default values for stuff like CMAKE_CXX_FLAGS_*.

I hadn't seen this comment, but good insight. I still heavily prefer the simplicity of -DDISABLE_GUI as it stays inline with -DDISABLE_WEBUI, and if you leave it enabled, you simply don't pass the flag. I'm probably going to do some more work with our build system this weekend to address that first point.

@NotTsunami

This comment has been minimized.

Copy link
Contributor

@NotTsunami NotTsunami commented Jan 18, 2020

So digging deeper into it, passing -DDISABLE_GUI will still disable some code compilation as actual source files check against that flag but our CMake configuration would likely error out if not scream at us. You can pass -DCMAKE_DISABLE_FIND_PACKAGE_Qt5Widgets=ON to explicitly disable the GUI as we do in travis, which is a little crazy for end-users and downstream. I understand the reasoning behind the revamp and swap to Qt5Widgets_FOUND, but it just makes a more confusing scenario for end-users and downstream.

If @zeule is still around, what are your thoughts on swapping back to a simple DISABLE_GUI check?

@FranciscoPombal

This comment has been minimized.

Copy link
Contributor Author

@FranciscoPombal FranciscoPombal commented Jan 18, 2020

I agree that for end users, -DDISABLE_GUI is what should be exposed. IMO, the CMake build flags that have to do with program features customization should be analogous to the autotools flags.

@zeule

This comment has been minimized.

Copy link
Contributor

@zeule zeule commented Jan 19, 2020

If @zeule is still around, what are your thoughts on swapping back to a simple DISABLE_GUI check?

The CMAKE_DISABLE_FIND_PACKAGE_XXX scheme follows that of the KDE project, which is a reference, probably, for many CMake based ones, and certainly for CMake + Qt projects. Therefore this is exactly what downstream maintainers used to work with and, I believe, is quite easy for users too: all the optional packages are listed in the CMake output with the explanation what they are required for. Users, who are aware of CMAKE_DISABLE_FIND_PACKAGE option immediately know what to do, others will read Wikis/Howtos regardless of the option name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.